Fixes ios accessibility send focus request to an unfocusable node #25738
Conversation
| // item that is currently off screen but the a11y navigation needs to know | ||
| // about. | ||
| return (([self node].flags & ~flutter::kScrollableSemanticsFlags) != 0 && | ||
| [self node].flags != static_cast<int32_t>(flutter::SemanticsFlags::kIsHidden)) || |
There was a problem hiding this comment.
I think this was a typo.
| return object; | ||
| } | ||
|
|
||
| SemanticsObject* candidate = nil; |
There was a problem hiding this comment.
I am not sure what had got into my head when i wrote this code. The pr change it to be more readable without changing the functionality
|
Ping @gaaclarke for the review please? |
|
Also cc @goderbauer. |
|
a friendly bump |
| // about. | ||
| return (([self node].flags & ~flutter::kScrollableSemanticsFlags) != 0 && | ||
| [self node].flags != static_cast<int32_t>(flutter::SemanticsFlags::kIsHidden)) || | ||
| ([self node].flags & ~static_cast<int32_t>(flutter::SemanticsFlags::kIsHidden)) != 1) || |
There was a problem hiding this comment.
This doesn't look correct, why are you checking against the 1 value? Are you trying to check to see if the kIsHidden bit is turned on? It should look like what is above in that case: ([self node].flags & flutter::kScrollableSemanticsFlags) != 0
Checking anything against 1 is going to be fragile since it would depend on which bit is kIsHidden.
There was a problem hiding this comment.
oops, I made a mistake, I surprised this still work
There was a problem hiding this comment.
Mostly looks good. The bitwise math looks a bit off. If it is right, it is fragile because of what a I mentioned in the comment. It occurred to me that maybe you wanted to check that it wasn't set? That would be like this: ([self node].flags & flutter::kScrollableSemanticsFlags) == 0
| new flutter::MockAccessibilityBridge()); | ||
| fml::WeakPtr<flutter::AccessibilityBridgeIos> bridge = factory.GetWeakPtr(); | ||
| flutter::SemanticsNode node; | ||
| node.flags = static_cast<int32_t>(flutter::SemanticsFlags::kHasImplicitScrolling) | |
There was a problem hiding this comment.
Are these casts necessary? They should already be int32_t's. (fwiw I think people use unsigned ints for flags to avoid 2's compliment weirdness)
There was a problem hiding this comment.
I got this error if i get rid of the static cast
error: invalid operands to binary expression ('flutter::SemanticsFlags' and 'flutter::SemanticsFlags')
node.flags = flutter::SemanticsFlags::kHasImplicitScrolling |
| if (nextToFocus) { | ||
| nextToFocus = FindFirstFocusable(nextToFocus); | ||
| } else if (root) { |
There was a problem hiding this comment.
Since you do these 2 checks I think it would be helpful if you pulled this out into a function like:
Object* FindFirstFocusable(Object* one, Object* two) {
if (one) {
return FindFirstFocusable(one);
} else if (two) {
return FindFirstFocusable(two);
}
return nullptr;
}This function is a monster, the more duplicate logic we can pull out the better imo.
Logic seems find though.
|
@gaaclarke Thanks for reviewing, I addressed all comments, this is ready for another look. |
chunhtai
added
the
waiting for tree to go green
4 participants
When we build a list view, we use cache extent to build some hidden element outside the view port so the voiceover can focus it and showOnScreen to move the hidden element into the screen to achieve scrolling. There is an issue if that hidden off screen element does not have any label/value/hint and thus not focusable when they are on screen. the accessibility bridge will still send the layoutchange to focus that element when it is scrolled to the viewport because it was previously focused. This PR fixes it so that when it send focus request, it will check again if that element is accessibility element and finds the first focusable child of the element if it is not. If none of the child is focusable, the layoutchange will be sent with nil target which lets the voiceover to decide what to focus next.
fixes flutter/flutter#80991
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.