Apply dpr transform to fuchsia accessibility bridge #21364

chunhtai · 2020-09-23T20:48:08Z

Description

apply dpr transform to fuchsia accessibility bridge

Related Issues

Fixes flutter/flutter#65218

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.

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.

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

chunhtai · 2020-09-24T23:59:21Z

cc @teisenbe

chunhtai · 2020-09-25T00:01:08Z

Since this pr applies the dpr transform on the root node, this requires https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=60641#c1 to be fixed in order to draw the correct highlight rect. The hittest is unaffected

dnfield · 2020-09-25T00:06:55Z

shell/platform/fuchsia/flutter/accessibility_bridge.cc

+    // view pixel ratio transformation in scenic view.
+    if (flutter_node.id == kRootNodeId) {
+      root_flutter_semantics_node_ = flutter_node;
+      continue;


Is this right? We don't want to set the ID or flags or whatever else might be present on the root node?

They will be set in GetRootNodeUpdate

dnfield · 2020-09-25T00:09:05Z

shell/platform/fuchsia/flutter/accessibility_bridge.cc

  PruneUnreachableNodes();
  UpdateScreenRects();

-  tree_ptr_->UpdateSemanticNodes(std::move(nodes));
+  tree_ptr_->UpdateSemanticNodes(std::move(root_update));


Why not just push the root node update (or, the synthetic node if we go that route) onto the updates rather than making two calls to UpdateSemanticNodes?

I don't want to go synthetic node route because we will then need to do node id conversion because the fuchsia always assume the first node id = 0.

If we want to reuse the updates list, we will need to handle size limit. I figure it may be easier to do it this way? (although test may need to be changed.)

0 should still be the root node, but we insert a child node right between the root node and the rest of the nodes coming in. Although I guess we'd have to come up with an ID the framework would not use in that case, which is unfortunate. It might be safe enough to use the maximum allowed ID value - especially since this is only expected to be a short term thing until the embedding stops inserting the transform.

and other reason i dislike synthetic node is that it may receive focus when doing swipe left or right. and it may possibly receiving hit testing, unless we make it not a a11y element. I am not sure if there is a flag for that in fuchsia.

My current intention is still go with this approach, but I am open to discussion

I think I agree that we should avoid a synthetic node, because we'd potentially be using an ID the framework wants to use.

But I still don't quite see why we need a separate update.

There's already logic that should be handling the size limit right?

Yeah, just move this logic up a bit in the method so we can append it to our nodes before we start splitting them.

shell/platform/fuchsia/flutter/accessibility_bridge.cc

dnfield · 2020-09-25T00:11:13Z

shell/platform/fuchsia/flutter/accessibility_bridge_unittest.cc

@@ -22,11 +22,13 @@ namespace flutter_runner_test {
 class AccessibilityBridgeTestDelegate
    : public flutter_runner::AccessibilityBridge::Delegate {
 public:
+  float view_pixel_ratio = 1.f;


For tests it would probably be helpful to set this to some other value so we can check the transforms are applied correctly.

Yes, I make use of this float in ApplyViewPixelRatioToRoot test

I see that it's used, but I think it should be some value other than 1.0, which doesn't result in a particularly interesting transformation right?

dnfield · 2020-09-25T00:11:49Z

shell/platform/fuchsia/flutter/platform_view_unittest.cc

+// class FuchsiaPlatformViewDelegate final : public
+// flutter_runner::PlatformView::Delegate {
+//   float GetViewPixelRatio() {
+//     return 1.f;
+//   }
+// };


Commented out code

dnfield · 2020-09-25T00:12:10Z

shell/platform/fuchsia/flutter/platform_view_unittest.cc

@@ -153,12 +160,13 @@ TEST_F(PlatformViewTests, ChangesAccessibilitySettings) {

  EXPECT_FALSE(delegate.SemanticsEnabled());
  EXPECT_EQ(delegate.SemanticsFeatures(), 0);
-
+  // FuchsiaPlatformViewDelegate fuchsia_delegate;


Commented out code

dnfield · 2020-09-25T00:12:58Z

Another benefit to not using the transform on the root widget would be that https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=60641#c1 could just be no-oped.

chunhtai · 2020-09-25T00:18:22Z

Another benefit to not using the transform on the root widget would be that https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=60641#c1 could just be no-oped.

If we do synthetic node, what will be the node id? also that node will show up in highlight rect and may have affect receive hittest

dnfield · 2020-09-25T22:21:10Z

shell/platform/fuchsia/flutter/accessibility_bridge.cc

+  // Even if the SemanticsNodeUpdates does not contain the root node, we still
+  // need to generate an update for the root node in case the view pixel ratio
+  // has changed.
+  std::vector<fuchsia::accessibility::semantics::Node> root_update;
+  root_update.push_back(GetRootNodeUpdate());


We should avoid sending an update to the root node if it has not changed (i.e. if the DPR has not changed). Sending the FIDL message is almost certainly more expensive than trackinging the state.

It's also not clear to me why we'd send a separate update instead of just including the root node in the update.

dnfield

LGTM as long as CI is passing :)

dnfield · 2020-09-25T23:31:10Z

Looks like it needs a format.

arbreng

LGTM if we get rid of "GetViewPixelRatio"

arbreng · 2020-09-26T00:16:50Z

shell/platform/fuchsia/flutter/accessibility_bridge.cc

@@ -309,6 +321,31 @@ void AccessibilityBridge::AddSemanticsNodeUpdate(
    PrintNodeSizeError(nodes.back().node_id());
  }

+  // Handles root node update.
+  float new_view_pixel_ratio = delegate_.GetViewPixelRatio();


Why not just pass this into AddSemanticsNodeUpdate directly from the PlatformView (or test)? That is the only place AddSemanticsNodeUpdate is ever called, so it avoids the extra delegate callback and associated complexity

arbreng · 2020-09-26T00:18:05Z

shell/platform/fuchsia/flutter/platform_view.h

@@ -70,6 +70,8 @@ class PlatformView final : public flutter::PlatformView,
  // |flutter_runner::AccessibilityBridge::Delegate|
  void DispatchSemanticsAction(int32_t node_id,
                               flutter::SemanticsAction action) override;
+  // |flutter_runner::AccessibilityBridge::Delegate|
+  float GetViewPixelRatio() override;


As I mentioned above, I think we can get rid of this

fluttergithubbot · 2020-09-26T00:37:02Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite build_and_test_linux_unopt_debug has failed. Please fix the issues identified (or deflake) before re-applying this label.

This reverts commit cf1fbf2.

…#21458) This reverts commit cf1fbf2.

…21364)" This reverts commit 9db9a57.

…engine#21364)

* a1db2b3 Roll Fuchsia Linux SDK from NeYDIjo58... to BFLXvCMVi... (flutter/engine#21403) * faeff0a Roll Dart SDK from eb8e6232da02 to 13b3f2d7b6ea (3 revisions) (flutter/engine#21407) * 7dfcde1 [Fix] Replaces call to deprecated method Name.name. (flutter/engine#21241) * 48ab5d1 Roll Skia from 1748c6a3b8c8 to 3b88c0772e89 (1 revision) (flutter/engine#21410) * aa8d5d4 Avoid sending a 0 DPR to framework (flutter/engine#21389) * 67fdd7e Embedder API Support for display settings (flutter/engine#21355) * 1fc87c0 Roll Skia from 3b88c0772e89 to d7ab45027877 (1 revision) (flutter/engine#21411) * de5f2b4 Revert "Revert "Adds fuchsia node roles to accessibility bridge updates. (#20385)" (#20936)" (flutter/engine#21367) * c9b40c6 Remove ASCII art from mDNS error log (flutter/engine#21397) * fed0531 Roll Fuchsia Mac SDK from xnB_uJM8T... to _e0onA6gY... (flutter/engine#21414) * d877b83 [web] enable ios safari screenshot tests (flutter/engine#21226) * a8f52e5 Roll Skia from d7ab45027877 to aeae3a58e3da (6 revisions) (flutter/engine#21415) * 8275609 Roll Skia from aeae3a58e3da to 7bd60430299f (1 revision) (flutter/engine#21417) * 65c1122 Roll Dart SDK from 13b3f2d7b6ea to 4fb134a228c7 (2 revisions) (flutter/engine#21419) * d735f2c Roll Fuchsia Linux SDK from BFLXvCMVi... to XcAUWQUZm... (flutter/engine#21420) * f1961e5 Roll Skia from 7bd60430299f to 68861e391313 (14 revisions) (flutter/engine#21421) * 08cf725 Fix getNextFrame (flutter/engine#21422) * db99912 Support dragging native platform views (flutter/engine#21396) * df83e8f Roll Skia from 68861e391313 to a05d27b170ee (1 revision) (flutter/engine#21424) * fc7d0fc [web] Respond with null for unimplemented method channels (flutter/engine#21423) * 02b8567 Roll Skia from a05d27b170ee to 5e1545fa00c8 (1 revision) (flutter/engine#21425) * 6e54e68 Roll Dart SDK from 4fb134a228c7 to db7eb2549480 (1 revision) (flutter/engine#21426) * b0cd7d1 Roll Dart SDK from db7eb2549480 to 200e8da5072a (1 revision) (flutter/engine#21427) * 0de5c0c Roll Fuchsia Linux SDK from XcAUWQUZm... to 0nW5DAxcC... (flutter/engine#21430) * 854943d [macOS] Set the display refresh rate (flutter/engine#21095) * 11aecf4 Roll Skia from 5e1545fa00c8 to 766eeb2ac325 (1 revision) (flutter/engine#21431) * b8e2b3f Roll Skia from 766eeb2ac325 to 5648572f4a94 (1 revision) (flutter/engine#21433) * 33d0bbb Roll Fuchsia Mac SDK from _e0onA6gY... to SUSVNJcX5... (flutter/engine#21434) * 51049d2 Roll Skia from 5648572f4a94 to eabce08bb2f2 (1 revision) (flutter/engine#21435) * a491533 Roll Dart SDK from 200e8da5072a to c938793e2d6f (1 revision) (flutter/engine#21437) * 06398b8 Roll Fuchsia Linux SDK from 0nW5DAxcC... to xdxm8rU8b... (flutter/engine#21439) * 4282bbc Roll Dart SDK from c938793e2d6f to fe83360d3a7c (1 revision) (flutter/engine#21440) * eba7b8e Roll Fuchsia Mac SDK from SUSVNJcX5... to v5Ko06GkT... (flutter/engine#21441) * 249bcf7 Roll Fuchsia Linux SDK from xdxm8rU8b... to ej-CkfSra... (flutter/engine#21443) * 4422ede Roll Fuchsia Mac SDK from v5Ko06GkT... to k_lSjZxIH... (flutter/engine#21445) * 35fa4bb Roll Dart SDK from fe83360d3a7c to 44e4f3958019 (1 revision) (flutter/engine#21446) * a9b5b13 Roll Fuchsia Linux SDK from ej-CkfSra... to HNNs4gfuM... (flutter/engine#21447) * 4f7ff21 Roll Skia from eabce08bb2f2 to ad6aeace6eee (2 revisions) (flutter/engine#21448) * f72613d Roll Dart SDK from 44e4f3958019 to e2a4eaba73b8 (1 revision) (flutter/engine#21451) * 2917a65 [ios] Remove unused is_valid_ from IOS Metal Context (flutter/engine#21432) * b591674 Roll Dart SDK from e2a4eaba73b8 to 13deada5b267 (1 revision) (flutter/engine#21453) * a07e0e0 Roll Fuchsia Mac SDK from k_lSjZxIH... to qyoO7f9Sk... (flutter/engine#21454) * cf1fbf2 Apply dpr transform to fuchsia accessibility bridge (flutter/engine#21364) * 9db9a57 Revert "Apply dpr transform to fuchsia accessibility bridge (#21364)" (flutter/engine#21458) * 8d165fa Revert multiple display support for embedder API (flutter/engine#21456)

* Reland "Apply dpr transform to fuchsia accessibility bridge (#21364)" This reverts commit 9db9a57. * fix test

chunhtai added the Work in progress (WIP) Not ready (yet) for review! label Sep 23, 2020

googlebot added the cla: yes label Sep 23, 2020

auto-assign bot requested a review from gw280 September 23, 2020 20:48

chunhtai force-pushed the issues/65218 branch 2 times, most recently from 08c3b30 to 4592116 Compare September 24, 2020 21:54

chunhtai requested review from dnfield and arbreng September 24, 2020 23:58

chunhtai removed the Work in progress (WIP) Not ready (yet) for review! label Sep 24, 2020

chunhtai changed the title ~~[draft] apply dpr transform to fuchsia accessibility bridge~~ Apply dpr transform to fuchsia accessibility bridge Sep 24, 2020

dnfield reviewed Sep 25, 2020

View reviewed changes

shell/platform/fuchsia/flutter/accessibility_bridge.cc Show resolved Hide resolved

dnfield reviewed Sep 25, 2020

View reviewed changes

chunhtai force-pushed the issues/65218 branch from 25fad0c to aa14377 Compare September 25, 2020 00:32

dnfield reviewed Sep 25, 2020

View reviewed changes

Apply dpr transform to fuchsia accessibility bridge

0148d66

chunhtai force-pushed the issues/65218 branch from aa14377 to 0148d66 Compare September 25, 2020 23:22

dnfield approved these changes Sep 25, 2020

View reviewed changes

fix format

a8042ca

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 Sep 26, 2020

arbreng approved these changes Sep 26, 2020

View reviewed changes

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 26, 2020

chunhtai added 2 commits September 25, 2020 18:05

addressing comment

33c6742

fix typo

767d75e

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 Sep 28, 2020

chunhtai merged commit cf1fbf2 into flutter:master Sep 28, 2020

zanderso added a commit that referenced this pull request Sep 28, 2020

Revert "Apply dpr transform to fuchsia accessibility bridge (#21364)"

d6c95de

This reverts commit cf1fbf2.

zanderso mentioned this pull request Sep 28, 2020

Revert "Apply dpr transform to fuchsia accessibility bridge" #21458

Merged

zanderso added a commit that referenced this pull request Sep 28, 2020

Revert "Apply dpr transform to fuchsia accessibility bridge (#21364)" (…

9db9a57

…#21458) This reverts commit cf1fbf2.

chunhtai added a commit to chunhtai/engine that referenced this pull request Sep 28, 2020

Reland "Apply dpr transform to fuchsia accessibility bridge (flutter#…

0f87183

…21364)" This reverts commit 9db9a57.

engine-flutter-autoroll mentioned this pull request Sep 28, 2020

Roll Engine from 3a73d073c8be to 8d165faca1ce (45 revisions) flutter/flutter#66840

Merged

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

cf1fbf2 Apply dpr transform to fuchsia accessibility bridge (flutter/…

9d18f2a

…engine#21364)

chunhtai added a commit that referenced this pull request Sep 28, 2020

Reland Apply dpr transform to fuchsia accessibility bridge (#21459)

70f7ef0

* Reland "Apply dpr transform to fuchsia accessibility bridge (#21364)" This reverts commit 9db9a57. * fix test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply dpr transform to fuchsia accessibility bridge #21364

Apply dpr transform to fuchsia accessibility bridge #21364

chunhtai commented Sep 23, 2020 •

edited

chunhtai commented Sep 24, 2020

chunhtai commented Sep 25, 2020

dnfield Sep 25, 2020

chunhtai Sep 25, 2020

dnfield Sep 25, 2020

chunhtai Sep 25, 2020

dnfield Sep 25, 2020

chunhtai Sep 25, 2020

dnfield Sep 25, 2020

dnfield Sep 25, 2020

dnfield Sep 25, 2020

chunhtai Sep 25, 2020

dnfield Sep 25, 2020

dnfield Sep 25, 2020

dnfield Sep 25, 2020

dnfield commented Sep 25, 2020

chunhtai commented Sep 25, 2020 •

edited

dnfield Sep 25, 2020

dnfield left a comment

dnfield commented Sep 25, 2020

arbreng left a comment

arbreng Sep 26, 2020

arbreng Sep 26, 2020

fluttergithubbot commented Sep 26, 2020

Apply dpr transform to fuchsia accessibility bridge #21364

Apply dpr transform to fuchsia accessibility bridge #21364

Conversation

chunhtai commented Sep 23, 2020 • edited

Description

Related Issues

Tests

Checklist

Breaking Change

chunhtai commented Sep 24, 2020

chunhtai commented Sep 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented Sep 25, 2020

chunhtai commented Sep 25, 2020 • edited

Choose a reason for hiding this comment

dnfield left a comment

Choose a reason for hiding this comment

dnfield commented Sep 25, 2020

arbreng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluttergithubbot commented Sep 26, 2020

chunhtai commented Sep 23, 2020 •

edited

chunhtai commented Sep 25, 2020 •

edited