[fuchsia] HitTesting for fuchsia a11y #15570

dnfield · 2020-01-14T01:06:02Z

This still needs a test, but working when tested manually

I'm a little torn on whether my defensive guards around cycles are overly-pessimistic. The framework should never allow this to happen, but if it does happen the engine will crash with a stack overflow - and the engine API has no asserts in place to check it otherwise.

Fixes flutter/flutter#47386

dnfield · 2020-01-14T06:48:44Z

Retry of failing Fuchsia build here: https://ci.chromium.org/p/flutter/builders/try/Linux%20Fuchsia/2150

Original failed because of infra issue on bot_update.

dnfield · 2020-01-14T17:31:42Z

https://ci.chromium.org/p/flutter/builders/try/Linux%20Fuchsia/2226

dnfield · 2020-01-15T00:04:40Z

@godofredoc - Linux Fuchsia has repeatedly failed on this due to different infra issues here...

godofredoc · 2020-01-15T00:23:11Z

https://github.com/flutter/engine/blob/master/testing/fuchsia/run_tests.sh#L37 seems like we need to add a delay after paving or a retry to the ssh connection. The test seems to be flaking because we are trying to ssh immediately after paving and the device may be still not available.

chinmaygarde · 2020-01-15T19:24:39Z

shell/platform/fuchsia/flutter/accessibility_bridge.cc

+        .rect = flutter_node.rect,
+        .transform = flutter_node.transform,
+        .children_in_hit_test_order =
+            std::vector<int32_t>(flutter_node.childrenInHitTestOrder),


Why the cast?

Fuchsia needs these at int32_t

chinmaygarde · 2020-01-15T19:31:39Z

shell/platform/fuchsia/flutter/accessibility_bridge.h

 private:
+  struct SemanticsNode {


Why not just reuse flutter::SemanticsNode from //flutter/lib/ui/semantics/semantics_node.h. It may have some redundant bits but having two of these is really hard to parse.

This started as an attempt to avoid holding onto more than we need, but I agree it's less helpful now. I'll change it.

Ah. The reason I can't do this is I need the screen_rect, which flutter::SemanticsNode does not have.

Thanks for clarifying.

chinmaygarde · 2020-01-15T19:33:08Z

shell/platform/fuchsia/flutter/accessibility_bridge.cc

+        .rect = flutter_node.rect,
+        .transform = flutter_node.transform,
+        .children_in_hit_test_order =
+            std::vector<int32_t>(flutter_node.childrenInHitTestOrder),


Not sure why the cast is necessary but just reusing the flutter::SemanticsNode would negate the need for any of this.

chinmaygarde · 2020-01-15T19:42:40Z

shell/platform/fuchsia/flutter/accessibility_bridge.cc

-
+  {
+    std::unordered_set<int32_t> visited_nodes;
+    UpdateScreenRects(kRootNodeId, SkMatrix44::I(), &visited_nodes);


Let's have an UpdateSceenRects overload that doesn't take the final argument so that callers don't have to do this dance. This interface looks a bit awkward to use.

UpdateScreenRects(int32_t node_id, SkMatrix44 parent_transform, std::unordered_set<int32_t>& visited_nodes = {}); should work too I think.

The initializer doesn't work because it's a non-const lvalue reference trying to bind to a temporary.

I can make a convenience override though.

chinmaygarde · 2020-01-15T19:58:25Z

shell/platform/fuchsia/flutter/accessibility_bridge.cc

+  const auto& current_transform = parent_transform * node.transform;
+
+  const auto& rect = node.rect;
+  std::vector<SkVector4> points = {


This can all be done in a single call to SkMatrix44::map2 , then putting the resulting points in a SkRect and then calling SkRect::sort.

Nice. Done ... I think. Am I picking the right indices?

lol, I am confused too. But these look right.

chinmaygarde · 2020-01-15T20:00:14Z

shell/platform/fuchsia/flutter/accessibility_bridge.h

+                         SkMatrix44 parent_transform,
+                         std::unordered_set<int32_t>* visited_nodes);
+
+  void GetHitNode(int32_t node_id,


A bit of documentation would be nice.

How about std::optional<Hit> GetHitNode(...) for clarity?

I'm not sure if this really makes things clearer, but did it.

dnfield · 2020-01-16T00:29:14Z

@godofredoc stick-movie-plank-alien misbehaving again, didn't finish paving within 30 minutes.

https://chromium-swarm.appspot.com/task?id=49c445271c064910

chinmaygarde

Other than the missing return ending up working on an invalid iterator, lgtm.

chinmaygarde · 2020-01-22T05:57:13Z

shell/platform/fuchsia/flutter/accessibility_bridge.cc

+  const auto& current_transform = parent_transform * node.transform;
+
+  const auto& rect = node.rect;
+  std::vector<SkVector4> points = {


lol, I am confused too. But these look right.

chinmaygarde · 2020-01-22T05:57:55Z

shell/platform/fuchsia/flutter/accessibility_bridge.h

 private:
+  struct SemanticsNode {


Thanks for clarifying.

chinmaygarde · 2020-01-22T05:58:32Z

shell/platform/fuchsia/flutter/accessibility_bridge.cc

+    std::unordered_set<int32_t>* visited_nodes) {
+  auto it = nodes_.find(node_id);
+  if (it == nodes_.end()) {
+    FML_LOG(ERROR) << "UpdateScreenRects called on unknown node";


Aren't you missing an early return here?

Yes - added.

dnfield · 2020-01-22T07:25:55Z

@godofredoc - stick-movie-plank-alien failed again... https://chromium-swarm.appspot.com/task?id=49e4cbb4d21ac710

dnfield · 2020-01-22T07:27:12Z

Is that because it's unplugged but somehow still in the pool?

fluttergithubbot · 2020-01-22T07:33:02Z

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

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

flutter/engine@83a64b7...46e58b9 git log 83a64b7..46e58b9 --first-parent --oneline 2020-01-22 iska.kaushik@gmail.com [bots] remove auto-assign for web PRs (flutter/engine#15878) 2020-01-22 dnfield@google.com [fuchsia] HitTesting for fuchsia a11y (flutter/engine#15570) 2020-01-22 skia-flutter-autoroll@skia.org Roll src/third_party/skia 808f021b51df..d59053987a27 (4 commits) (flutter/engine#15882) 2020-01-22 skia-flutter-autoroll@skia.org Roll src/third_party/dart 4ad5fab95753..439cabaec02f (2 commits) (flutter/engine#15881) 2020-01-22 skia-flutter-autoroll@skia.org Roll src/third_party/skia 4277f0173657..808f021b51df (7 commits) (flutter/engine#15879) 2020-01-22 goderbauer@google.com Remove unused Imports and private method (flutter/engine#15872) 2020-01-22 skia-flutter-autoroll@skia.org Roll src/third_party/dart ef0c7f16e609..4ad5fab95753 (41 commits) (flutter/engine#15875) 2020-01-22 stuartmorgan@google.com Add missing include in GLFW and Windows embeddings (flutter/engine#15867) 2020-01-22 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from p1UDn... to v-OJE... (flutter/engine#15874) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC aaclarke@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

This reverts commit d41eb4b.

HitTesting for fuchsia a11y

1405b2b

auto-assign bot requested a review from gaaclarke January 14, 2020 01:06

googlebot added the cla: yes label Jan 14, 2020

dnfield force-pushed the fuchsia_hit branch from 93db0e6 to 3301807 Compare January 14, 2020 04:09

test attempt

3b38ae2

dnfield force-pushed the fuchsia_hit branch from 3301807 to 3b38ae2 Compare January 14, 2020 04:34

update test, avoid cycles in new method

36a8681

dnfield changed the title ~~[wip][fuchsia] HitTesting for fuchsia a11y~~ [fuchsia] HitTesting for fuchsia a11y Jan 14, 2020

dnfield requested a review from chinmaygarde January 14, 2020 06:28

uncomment

b30e145

dnfield requested a review from cbracken January 14, 2020 17:31

CI

358ae6c

Merge remote-tracking branch 'upstream/master' into fuchsia_hit

55d9b9c

dnfield requested a review from jason-simmons January 15, 2020 17:23

chinmaygarde reviewed Jan 15, 2020

View reviewed changes

map2

4648271

dnfield requested a review from chinmaygarde January 15, 2020 22:58

chinmaygarde approved these changes Jan 22, 2020

View reviewed changes

dnfield added 3 commits January 21, 2020 22:16

Merge remote-tracking branch 'upstream/master' into fuchsia_hit

a318f41

early return

b80421e

Merge branch 'fuchsia_hit' of github.com:dnfield/engine into fuchsia_hit

2d1de23

dnfield 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 Jan 22, 2020

SkScalar

c502973

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 Jan 22, 2020

ci

f541a08

dnfield merged commit c3e2c0f into flutter:master Jan 22, 2020

dnfield deleted the fuchsia_hit branch January 22, 2020 09:12

dnfield mentioned this pull request Jan 22, 2020

errant log #15891

Merged

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

c3e2c0f [fuchsia] HitTesting for fuchsia a11y (flutter/engine#15570)

06fa475

engine-flutter-autoroll mentioned this pull request Jan 22, 2020

Roll engine 83a64b7233d5..46e58b930d11 (9 commits) flutter/flutter#49306

Closed

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

c3e2c0f [fuchsia] HitTesting for fuchsia a11y (flutter/engine#15570)

5010eea

This was referenced Jan 22, 2020

Roll engine 83a64b7233d5..46e58b930d11 (9 commits) flutter/flutter#49311

Closed

Roll engine 83a64b7233d5..46e58b930d11 (9 commits) flutter/flutter#49312

Merged

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

c3e2c0f [fuchsia] HitTesting for fuchsia a11y (flutter/engine#15570)

da77bd2

NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020

[fuchsia] HitTesting for fuchsia a11y (flutter#15570)

d41eb4b

NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020

Revert "[fuchsia] HitTesting for fuchsia a11y (flutter#15570)"

fd0f543

This reverts commit d41eb4b.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fuchsia] HitTesting for fuchsia a11y #15570

[fuchsia] HitTesting for fuchsia a11y #15570

dnfield commented Jan 14, 2020 •

edited

Loading

dnfield commented Jan 14, 2020

dnfield commented Jan 14, 2020

dnfield commented Jan 15, 2020

godofredoc commented Jan 15, 2020

chinmaygarde Jan 15, 2020

dnfield Jan 15, 2020

chinmaygarde Jan 15, 2020

dnfield Jan 15, 2020

dnfield Jan 15, 2020

chinmaygarde Jan 22, 2020

chinmaygarde Jan 15, 2020

chinmaygarde Jan 15, 2020

dnfield Jan 15, 2020

chinmaygarde Jan 15, 2020

dnfield Jan 15, 2020

chinmaygarde Jan 22, 2020

chinmaygarde Jan 15, 2020

chinmaygarde Jan 15, 2020

dnfield Jan 15, 2020

dnfield commented Jan 16, 2020 •

edited

Loading

chinmaygarde left a comment

chinmaygarde Jan 22, 2020

chinmaygarde Jan 22, 2020

chinmaygarde Jan 22, 2020

dnfield Jan 22, 2020

dnfield commented Jan 22, 2020

dnfield commented Jan 22, 2020

fluttergithubbot commented Jan 22, 2020

[fuchsia] HitTesting for fuchsia a11y #15570

[fuchsia] HitTesting for fuchsia a11y #15570

Conversation

dnfield commented Jan 14, 2020 • edited Loading

dnfield commented Jan 14, 2020

dnfield commented Jan 14, 2020

dnfield commented Jan 15, 2020

godofredoc commented Jan 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented Jan 16, 2020 • edited Loading

chinmaygarde left a 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 Jan 22, 2020

dnfield commented Jan 22, 2020

fluttergithubbot commented Jan 22, 2020

dnfield commented Jan 14, 2020 •

edited

Loading

dnfield commented Jan 16, 2020 •

edited

Loading