Adjustments to FocusHighlightMode handling #162417
Conversation
d849f5a to
375bbdc
Compare
|
This is currently blocked on #162472, which is causing the g3 failures. |
375bbdc to
124ca4a
Compare
|
Giving this another go now that #162472 is resolved. 🤞 |
|
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... |
| 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.
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.
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.
It is counter-intuitive that code that handles SemanticsAction.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.
Renaming this is a good idea.
|
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.
Temporarily removing LGTM because my change, which this PR depends on, might have broken first-frame behavior. I'm looking into that right now.
65ff524 to
f3b9c49
Compare
|
Kicking off another Google testing run... 🤞 |
|
autosubmit label was removed for flutter/flutter/162417, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
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.