Avoid passing nil to IOS accessibility announcement #20700
Conversation
| // Tries to refocus the previous focused semantics object to avoid random jumps. | ||
| SemanticsObject* nextToFocus = [objects_.get() objectForKey:@(last_focused_semantics_object_id_)]; | ||
| if (!nextToFocus) | ||
| nextToFocus = first_focusable_; |
There was a problem hiding this comment.
The one thing that i am uncertain about this pr is that we giving up on letting iOS to pick which element to focus, which may be in contradiction with accessibility guide line.
|
I think the name of this PR could be more specific, "make x more robust" is pretty generic. =) |
chunhtai
changed the title
| NSMutableArray<NSNumber*>* doomed_uids) { | ||
| [doomed_uids removeObject:@(object.uid)]; | ||
| void AccessibilityBridge::UpdateFirstFocusable(SemanticsObject* object) { | ||
| if (first_focusable_ || !object.isAccessibilityElement) |
There was a problem hiding this comment.
We don't have the linter running on iOS code yet but can you please add the missing curly braces to match google style here and elsewhere.
| first_focusable_ = object; | ||
| } | ||
|
|
||
| void AccessibilityBridge::WalkAndProccessTree(SemanticsObject* object) { |
There was a problem hiding this comment.
This function isn't clear what it does from its signature. Please consider a rename or a docstring.
I wouldn't conflate the removal of objects with the calculation of first_focusable_, the first_focusable_ search doesn't even need to traverse the whole tree. The code would be more clear if the functions kept to performing one task, so keep VisitObjectsRecursivelyAndRemove and add a SemanticsObject* FindFirstFocusable(SemanticsObject* object_tree).
|
|
||
| NSMutableArray<NSNumber*>* doomed_uids = [NSMutableArray arrayWithArray:[objects_.get() allKeys]]; | ||
| doomed_uids_ = [NSMutableArray arrayWithArray:[objects_.get() allKeys]]; | ||
| first_focusable_ = nil; |
There was a problem hiding this comment.
If you take my suggestion below this line will be much more clear since there is a temporal dependency that first_focusable_ is set before the next part of the function, ex: first_focusable_ = FindFirstFocusable(objects_.get())
| } | ||
|
|
||
| NSMutableArray<NSNumber*>* doomed_uids = [NSMutableArray arrayWithArray:[objects_.get() allKeys]]; | ||
| doomed_uids_ = [NSMutableArray arrayWithArray:[objects_.get() allKeys]]; |
There was a problem hiding this comment.
Why are you making doomed_uids an ivar? This is error prone, just pass it as an argument where it is needed here. Generally you should always be looking to do as much work as you can with the least amount of data on the heap. The more stateful your system is the harder it is to reason about. Most bugs come from stale state. =)
|
I think the approach is good, I just have some feedback on the details =) I remember seeing this and seeing the TODO and thinking we should look into it, thanks =) |
|
@gaaclarke thanks for review, I addressed all review comments. This pr is ready for another look. |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Awesome, I think this looks way better =)
chunhtai
added
the
waiting for tree to go green
* b08c6b9 Fixing synthesizing keys for multiple keys pressed down on flutter web (flutter/engine#19632) * 8308b6a Avoid passing nil to IOS accessibility announcement (flutter/engine#20700) * 950b6a0 Roll Skia from ead4ca052b99 to 5da7327358e3 (1 revision) (flutter/engine#20782) * 87fd0e4 Guard recording_canvas against restore calls without ending recording (flutter/engine#20786) * 388193a Add tests for lerpDouble (flutter/engine#20778)
* b08c6b9 Fixing synthesizing keys for multiple keys pressed down on flutter web (flutter/engine#19632) * 8308b6a Avoid passing nil to IOS accessibility announcement (flutter/engine#20700) * 950b6a0 Roll Skia from ead4ca052b99 to 5da7327358e3 (1 revision) (flutter/engine#20782) * 87fd0e4 Guard recording_canvas against restore calls without ending recording (flutter/engine#20786) * 388193a Add tests for lerpDouble (flutter/engine#20778)
* b08c6b9 Fixing synthesizing keys for multiple keys pressed down on flutter web (flutter/engine#19632) * 8308b6a Avoid passing nil to IOS accessibility announcement (flutter/engine#20700) * 950b6a0 Roll Skia from ead4ca052b99 to 5da7327358e3 (1 revision) (flutter/engine#20782) * 87fd0e4 Guard recording_canvas against restore calls without ending recording (flutter/engine#20786) * 388193a Add tests for lerpDouble (flutter/engine#20778)
4 participants
Description
We passed nil to layoutchange announcement when the previous focused node changes, which is correct that the ios should focus the first element on the screen. However, it seems like ios accessiblity api sometimes may take up to 10s+ to figure out what to focus next when there are frequent screen changes such as scrolling + semantics tree changes.
This PR finds the first element on the screen to focus instead of passing nil to reduce the lag.
Related Issues
Fixes flutter/flutter#64339
Tests
I added the following tests:
See files
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.Breaking Change
Did any tests fail when you ran them? Please read handling breaking changes.