makes android semanticsnode to ignore hittest if it is not focusable #22205

chunhtai · 2020-10-29T19:03:12Z

Description

The semantics order is determined by the rect and its location. This can be a problem for sliver app bar because the flexablespace may have different rect when it collapses or expands, and it can change the semantics order of the flexablespace and appbar when it does so.

Therefore, we enforce semantics order for the flexablespace and appbar by adding additional semantics nodes with sortKey.
https://github.com/flutter/flutter/blob/470346f77591d38234fa2930583a6f4f582e77e7/packages/flutter/lib/src/material/app_bar.dart#L692

This however introduces an additional semantics node for both flexablespace and appbar. This become a problem if the appbar does not contribute any node to the semantics tree. In this case, we will still create an empty semantics node with sortKey for the appbar which will absorb the hittest of flexablespace, and make flexablespace unfocusable through hittest.

As far as I know, there are two ways we can fix this.

On framework side, we only add semantics node with sortKey when we detect subtree has contributed any semantics node.
On engine side, makes the android accessibility hittest to ignore nodes that does not have any semantics information.

This PR implements 2.

Related Issues

Fixes flutter/flutter#69145

Tests

I added the following tests:

No test for now until we agree this is the right approach.

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.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

flutter-dashboard · 2020-10-29T19:03:15Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chunhtai · 2020-10-29T19:07:04Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

        }
      }
-      return this;
+      return isFocusable() ? this : null;


In iOS, we ask a11y service to ignore the node entirely by setting isAccessibilityElement = NO when the condition is met.
So I "think" this is ok

Where do you see this in the iOS bridge? I'm looking at https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm#L233 and don't see focusability there. We use a few other things theree that we're not using here though.

However, we do use most of the other logic from isFocusable in the iOS version

yes, I mean iOS uses most of other logic from isFocusable except the addtional IS_FOCUSABLE flag. but i think we are trying to achieve the same thing in iOS. The isAccessibilityElement =NO is more strict than AccessibilityNodeInfo.setFocusable(false) that the former will not absorb hit test, and i think we want to same behavior here?

If not, we can go with implementing changes in framework as I mentioned in the PR description.

I think this is probably fine. Let's just add a test to cover it.

ok :) will do

I am wondering if we have any cases where we use a non-focusable semantics node to hide a node behind it from hit testing. Do we need that use case for anything?

Do we, for example rely on the app bar to block focusing elements that are scrolled behind it in the scrollable list? I think we don't, but we should check.

If we cannot come up with a case where we need this particular behavior, this change seems fine. We should add a test, though!

I think the sliver app bar does not overlay with the sliverlist, so nothing should be behind the appbar. I can't think of a use case where user might want to block something using non-focusable semantics node. It is already weird that if you have overlapping UI components.

dnfield

Can we add a test?

chunhtai · 2020-10-29T21:10:26Z

Can we add a test?

I will add a test after we agree this is the right approach

chunhtai · 2020-10-30T18:40:50Z

shell/platform/android/test/io/flutter/view/AccessibilityBridgeTest.java

+        .requestSendAccessibilityEvent(eq(mockRootView), eventCaptor.capture());
+    event = eventCaptor.getAllValues().get(2);
+    assertEquals(event.getEventType(), AccessibilityEvent.TYPE_VIEW_HOVER_ENTER);
+    assertEquals(accessibilityBridge.getHoveredObjectId(), 2);


This seems to be the only way to get the event target. The event.getSource returns null even after we call setSource.

chunhtai · 2020-10-30T18:42:31Z

@goderbauer @dnfield , Thanks for review. I added a test. can you take another look?

dnfield

LGTM

…cusable (flutter/engine#22205)

* c597333 Roll Skia from f548a028ce70 to c21902c0d3cc (46 revisions) (flutter/engine#22224) * 37d766c Fix includes to start with shell (flutter/engine#22227) * 1ad6765 [web] Fixes canvas pixelation and overallocation due to transforms. (flutter/engine#22160) * 9f9fc1f Roll Skia from c21902c0d3cc to 9615bcf71f2a (1 revision) (flutter/engine#22226) * 0faa72e Roll Dart SDK from fed66f60a3bc to 25ef5dc559cf (1 revision) (flutter/engine#22225) * 172a393 Report image diff status for iOS scenario golden tests (flutter/engine#22230) * fddabca updating integration tests version. (flutter/engine#22235) * a36bcdc Roll Fuchsia Mac SDK from mhak7e_o6... to 8SkbMXJJ9... (flutter/engine#22231) * 6a331d3 Roll Skia from 9615bcf71f2a to d5e6368fffd0 (8 revisions) (flutter/engine#22234) * 9b34207 Fixing semantics borders on mobile web (flutter/engine#21856) * 4e9459e Refactored the FlutterEngine to make it easier to implement spawn functionality (flutter/engine#21890) * fa77e68 disable AppLifecycleTests (flutter/engine#22236) * 153775b update golden (flutter/engine#22247) * 50dbe85 [web] fix hot restart type error (flutter/engine#22248) * c8cf09a Roll Skia from d5e6368fffd0 to 7585a65ac709 (7 revisions) (flutter/engine#22237) * 68e2e46 Roll Fuchsia Mac SDK from 8SkbMXJJ9... to Pz4ZHZrUp... (flutter/engine#22246) * d3182bc Roll Dart SDK from 25ef5dc559cf to 5acb5fcf84cb (4 revisions) (flutter/engine#22243) * 9b4bb20 makes android semanticsnode to ignore hittest if it is not focusable (flutter/engine#22205) * 3c7a54e Roll Fuchsia Linux SDK from sNx8qabBn... to QqGvMWaYk... (flutter/engine#22244) * eea98b2 Roll Skia from 7585a65ac709 to dffd20efe95c (14 revisions) (flutter/engine#22250) * 46e3bba Defer Windows arrow key and delete handling (flutter/engine#22207) * 14437d6 fix _getArrayBuffer signature (flutter/engine#22251) * 8defec6 Fix nullability issue with Image.network (flutter/engine#22252) * 67d55ed Roll Dart SDK from 5acb5fcf84cb to a9d583383410 (4 revisions) (flutter/engine#22255) * 7c8f57c Report error when instantiating CanvasKit network image (flutter/engine#22159) * 9945db3 Remove the metrics task from cirrus. (flutter/engine#22240) * bd19181 Add braces on if statements to match linter style (flutter/engine#22130) * e9c62e7 do not print in _computePixelDensity (flutter/engine#22257) * 2617101 Roll Dart SDK from a9d583383410 to d2577410a501 (1 revision) (flutter/engine#22258) * 3d194fa Switch macOS embedding to proc table embedder API (flutter/engine#21811) * 31b6f0b Roll Fuchsia Mac SDK from Pz4ZHZrUp... to 6yEx5GNGG... (flutter/engine#22262) * 78a0181 Roll Fuchsia Linux SDK from QqGvMWaYk... to oLF1FW-gC... (flutter/engine#22264) * ccdb681 WeakPersistentHandle migration (flutter/engine#19843) * ce0a30c Roll Dart SDK from 52783837369d to b43baaaa477d (723 revisions) (flutter/engine#22265) * 59b01e0 [web] Fix repaint logic for cullrect,transform changes (flutter/engine#22273)

…lutter#22205)

flutter-dashboard bot added the platform-android label Oct 29, 2020

googlebot added the cla: yes label Oct 29, 2020

chunhtai requested review from dnfield and goderbauer October 29, 2020 19:03

chunhtai commented Oct 29, 2020

View reviewed changes

dnfield reviewed Oct 29, 2020

View reviewed changes

chunhtai commented Oct 30, 2020

View reviewed changes

chunhtai added 2 commits October 30, 2020 11:43

makes android semanticsnode to ignore hittest if it is not focusable

edd83ae

add test

b84e387

chunhtai force-pushed the issues/69145 branch from 6d9f6b5 to b84e387 Compare October 30, 2020 18:43

dnfield approved these changes Nov 2, 2020

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 Nov 2, 2020

fluttergithubbot merged commit 9b4bb20 into flutter:master Nov 2, 2020

engine-flutter-autoroll mentioned this pull request Nov 2, 2020

Roll Engine from 346051939814 to 14437d6c0006 (22 revisions) flutter/flutter#69599

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2020

9b4bb20 makes android semanticsnode to ignore hittest if it is not fo…

a3b807c

…cusable (flutter/engine#22205)

engine-flutter-autoroll mentioned this pull request Nov 2, 2020

Roll Engine from 346051939814 to 8defec6e8ca1 (23 revisions) flutter/flutter#69608

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2020

9b4bb20 makes android semanticsnode to ignore hittest if it is not fo…

1c6f81a

…cusable (flutter/engine#22205)

engine-flutter-autoroll mentioned this pull request Nov 2, 2020

Roll Engine from 346051939814 to 67d55ed42b25 (24 revisions) flutter/flutter#69613

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2020

9b4bb20 makes android semanticsnode to ignore hittest if it is not fo…

d20ea42

…cusable (flutter/engine#22205)

jonahwilliams mentioned this pull request Nov 2, 2020

Remove usage of --enable-experiment to analysis server flutter/flutter#69614

Merged

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 7c8f57c07d24 (25 revisions) flutter/flutter#69623

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

9b4bb20 makes android semanticsnode to ignore hittest if it is not fo…

1339818

…cusable (flutter/engine#22205)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 26171019992a (29 revisions) flutter/flutter#69637

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

9b4bb20 makes android semanticsnode to ignore hittest if it is not fo…

c7e6ddd

…cusable (flutter/engine#22205)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 31b6f0b31a48 (31 revisions) flutter/flutter#69648

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

9b4bb20 makes android semanticsnode to ignore hittest if it is not fo…

3e59d1c

…cusable (flutter/engine#22205)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 78a018174088 (32 revisions) flutter/flutter#69657

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

9b4bb20 makes android semanticsnode to ignore hittest if it is not fo…

116de8a

…cusable (flutter/engine#22205)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to ccdb6819ce53 (33 revisions) flutter/flutter#69672

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

9b4bb20 makes android semanticsnode to ignore hittest if it is not fo…

7d8a5c9

…cusable (flutter/engine#22205)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to ccdb6819ce53 (33 revisions) flutter/flutter#69692

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

9b4bb20 makes android semanticsnode to ignore hittest if it is not fo…

99aad4a

…cusable (flutter/engine#22205)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 59b01e0e5a3f (35 revisions) flutter/flutter#69709

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

9b4bb20 makes android semanticsnode to ignore hittest if it is not fo…

fc83be9

…cusable (flutter/engine#22205)

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

makes android semanticsnode to ignore hittest if it is not focusable (f…

15fd2a9

…lutter#22205)

makes android semanticsnode to ignore hittest if it is not focusable #22205

makes android semanticsnode to ignore hittest if it is not focusable #22205

Uh oh!

Conversation

chunhtai commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Oct 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Oct 29, 2020

Uh oh!

chunhtai Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Oct 30, 2020

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chunhtai commented Oct 29, 2020 •

edited

Loading

chunhtai Oct 29, 2020 •

edited

Loading

chunhtai Oct 30, 2020 •

edited

Loading