Fixes ios accessibility send focus request to an unfocusable node #25738

chunhtai · 2021-04-23T21:12:37Z

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

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.
The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

chunhtai · 2021-04-23T21:28:01Z

shell/platform/darwin/ios/framework/Source/SemanticsObject.mm

@@ -284,7 +284,7 @@ - (BOOL)isAccessibilityElement {
  // 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)) ||


I think this was a typo.

chunhtai · 2021-04-23T21:29:35Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

@@ -329,14 +333,13 @@ static bool DidFlagChange(const flutter::SemanticsNode& oldNode,
    return object;
  }

-  SemanticsObject* candidate = nil;


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

chinmaygarde · 2021-04-26T17:34:36Z

Ping @gaaclarke for the review please?

chunhtai · 2021-04-27T22:38:16Z

Also cc @goderbauer.
The current behavior will always treat offscreen element in the sliver list as focusable. Once it bring into view, it will then check its content whether this is actually focusable or not, and let the iOS to decide should it focus next. If there is a long list with unfocusable item, swipe right will scroll the list view while keep the focus at the same place. Is this acceptable?

chunhtai · 2021-04-29T20:55:52Z

a friendly bump

gaaclarke · 2021-05-03T16:37:58Z

shell/platform/darwin/ios/framework/Source/SemanticsObject.mm

@@ -284,7 +284,7 @@ - (BOOL)isAccessibilityElement {
  // 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)) ||
+          ([self node].flags & ~static_cast<int32_t>(flutter::SemanticsFlags::kIsHidden)) != 1) ||


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.

oops, I made a mistake, I surprised this still work

gaaclarke

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

gaaclarke · 2021-05-03T16:40:50Z

shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm

+      new flutter::MockAccessibilityBridge());
+  fml::WeakPtr<flutter::AccessibilityBridgeIos> bridge = factory.GetWeakPtr();
+  flutter::SemanticsNode node;
+  node.flags = static_cast<int32_t>(flutter::SemanticsFlags::kHasImplicitScrolling) |


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)

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 |

weird, thanks

gaaclarke · 2021-05-03T16:46:05Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+      if (nextToFocus) {
+        nextToFocus = FindFirstFocusable(nextToFocus);
+      } else if (root) {


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.

chunhtai · 2021-05-03T19:42:05Z

@gaaclarke Thanks for reviewing, I addressed all comments, this is ready for another look.

gaaclarke

LGTM, thanks for the fix!

…node (flutter/engine#25738)

…utter#25738)

flutter-dashboard bot added the platform-ios label Apr 23, 2021

google-cla bot added the cla: yes label Apr 23, 2021

chunhtai requested a review from gaaclarke April 23, 2021 21:26

chunhtai commented Apr 23, 2021

View reviewed changes

gaaclarke reviewed May 3, 2021

View reviewed changes

gaaclarke requested changes May 3, 2021

View reviewed changes

chunhtai added 3 commits May 3, 2021 10:56

Fixes ios accessibility send focus request to an unfocusable node

b965248

remove log

929a166

addressing comments

ca9f26e

chunhtai force-pushed the issues/80991 branch from bf0db88 to ca9f26e Compare May 3, 2021 19:37

gaaclarke approved these changes May 3, 2021

View reviewed changes

chunhtai added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label May 3, 2021

fluttergithubbot merged commit 223fb2e into flutter:master May 3, 2021

engine-flutter-autoroll mentioned this pull request May 4, 2021

Roll Engine from 378e4dbc417e to f57e986aa80c (12 revisions) flutter/flutter#81800

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 4, 2021

223fb2e Fixes ios accessibility send focus request to an unfocusable …

1cb0cf9

…node (flutter/engine#25738)

gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request May 5, 2021

Fixes ios accessibility send focus request to an unfocusable node (fl…

f3b271c

…utter#25738)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes ios accessibility send focus request to an unfocusable node #25738

Fixes ios accessibility send focus request to an unfocusable node #25738

chunhtai commented Apr 23, 2021

chunhtai Apr 23, 2021

chunhtai Apr 23, 2021 •

edited

Loading

chinmaygarde commented Apr 26, 2021

chunhtai commented Apr 27, 2021

chunhtai commented Apr 29, 2021

gaaclarke May 3, 2021

chunhtai May 3, 2021

gaaclarke left a comment

gaaclarke May 3, 2021

chunhtai May 3, 2021

gaaclarke May 3, 2021

gaaclarke May 3, 2021

chunhtai commented May 3, 2021

gaaclarke left a comment

Fixes ios accessibility send focus request to an unfocusable node #25738

Fixes ios accessibility send focus request to an unfocusable node #25738

Conversation

chunhtai commented Apr 23, 2021

Pre-launch Checklist

chunhtai Apr 23, 2021

Choose a reason for hiding this comment

chunhtai Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

chinmaygarde commented Apr 26, 2021

chunhtai commented Apr 27, 2021

chunhtai commented Apr 29, 2021

gaaclarke May 3, 2021

Choose a reason for hiding this comment

chunhtai May 3, 2021

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

gaaclarke May 3, 2021

Choose a reason for hiding this comment

chunhtai May 3, 2021

Choose a reason for hiding this comment

gaaclarke May 3, 2021

Choose a reason for hiding this comment

gaaclarke May 3, 2021

Choose a reason for hiding this comment

chunhtai commented May 3, 2021

gaaclarke left a comment

Choose a reason for hiding this comment

chunhtai Apr 23, 2021 •

edited

Loading