- Notifications
You must be signed in to change notification settings - Fork30k
Adjustments to FocusHighlightMode handling#162417
Adjustments to FocusHighlightMode handling#162417auto-submit[bot] merged 5 commits intoflutter:masterfrom
Conversation
goderbauer commentedJan 30, 2025
This is currently blocked on#162472, which is causing the g3 failures. |
goderbauer commentedFeb 4, 2025
Giving this another go now that#162472 is resolved. 🤞 |
goderbauer commentedFeb 4, 2025
Arg, we have to wait for#162554 to roll into google3 to know that it fixes the issues there. I can confirm that in mylocal testing the issue appears to be fixed. Onto writing some more tests for this... |
gspencergoog left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| arguments is ByteData | ||
| ? action.copyWith(arguments: const StandardMessageCodec().decodeMessage(arguments)) | ||
| : action; | ||
| final List<ValueSetter<ui.SemanticsActionEvent>> localListeners = _semanticsActionListeners |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's not immediately obvious what this is trying to do. AFAICT, this is creating a defensive copy to make sure the iterated list is not mutated during the loop. Maybe leave a comment, or give the variable a better name e.g.defensiveCopy or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'll leave a comment. This is a pattern that we use everywhere in the framework where listeners need to be called, though.
| if (kIsWeb && | ||
| semanticsActionEvent.type == SemanticsAction.focus && | ||
| _lastInteractionWasTouch != true) { | ||
| _lastInteractionWasTouch = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is counter-intuitive that code that handlesSemanticsAction.focus sets something called_lastInteractionWasTouch to true here. Perhaps it's time we renamed_lastInteractionWasTouch to something that reflects the intent, e.g._lastInteractionRequiresNoHighlight (or if we want to remove double-negatives_lastInteractionRequiresHighlight).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Renaming this is a good idea.
goderbauer commentedFeb 5, 2025
Waiting on cl/723567941 to land. That should hopefully resolve the "google testing" issues. |
yjbanov left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Temporarily removing LGTM because my change, which this PR depends on, might have broken first-frame behavior. I'm looking into that right now.
yjbanov left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Re-LGTM. The first frame fix was fairly trivial:#162779
goderbauer commentedFeb 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Kicking off another Google testing run... 🤞 |
autosubmit label was removed for flutter/flutter/162417, because - The status or check suiteGoogle testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
Uh oh!
There was an error while loading.Please reload this page.
Fixes#158527
Adjustments:
FocusHighlightModes (this matches observed behavior on Android)FocusHighlightModes.touch, which hides the visual input focus indication from non-Textfields. The reason here is in order to give something input focus on the web it also has to have a11y focus, which is indicated separately.