Adds the semantic node traversal order API. #14060

gspencergoog · 2018-01-12T01:21:57Z

This adds an API for defining the semantic node traversal order.

It adds a sortOrder argument to the Semantics widget, which is a class that can define a list of sort keys to sort on. The keys are sorted globally so that an order that doesn't have to do with the current widget hierarchy may be defined.

It also adds a shortcut sortKey argument to the Semantics widget that simply sets the sortOrder to just contain that key.

The platform side (flutter/engine#4540) gets an additional member in the SemanticsData object that is an integer describing where in the overall order each semantics node belongs. There is an associated engine-side change that takes this integer and uses it to order widgets for the platform's accessibility services.

goderbauer · 2018-01-12T18:18:17Z

packages/flutter/test/semantics/semantics_test.dart

If sortOrder is null can we just hide it from the printed tree? Seems like there is no value in showing it?

Good point. Fixed

goderbauer · 2018-01-12T18:35:44Z

packages/flutter/lib/src/semantics/semantics.dart

(unrelated to your PR) Looks like this one is missing a few other members as well. Filed #14072.

goderbauer · 2018-01-12T18:35:51Z

packages/flutter/lib/src/semantics/semantics.dart

goderbauer · 2018-01-12T18:39:01Z

packages/flutter/lib/src/semantics/semantics.dart

This should probably include some examples of how an app developer can use the sortOrder.

Added. Hopefully that's clear. I'm also going to be adding an example app to the catalog.

goderbauer · 2018-01-12T18:40:02Z

packages/flutter/lib/src/semantics/semantics.dart

why print just the if of child instead of the entire child node, which should contain the id plus more?

Because it was just really verbose. I reverted it (except for the English fix).

goderbauer · 2018-01-12T18:48:04Z

packages/flutter/lib/src/semantics/semantics.dart

nit: I know it's a private class, but the next person looking at this would probably appreciate a short doc comment explaining what this class is :)

goderbauer · 2018-01-12T18:58:32Z

packages/flutter/lib/src/semantics/semantics.dart

I don't really like the name default traversal. I don't have a better idea of the top of my head either. Can we maybe make this describe more what the default is, e.g. something like topLeftToBottomRightTraversal?

Also, can you add a description to the comment what traversal order would look like when sort nodes are ignored?

geometricOrder?

visualOrder?

I like geometricOrder. Changed.

goderbauer · 2018-01-12T18:59:45Z

packages/flutter/lib/src/semantics/semantics.dart

TranversalSortOrder probably needs to be renamed to its new name here?

goderbauer · 2018-01-12T19:01:21Z

packages/flutter/lib/src/semantics/semantics.dart

Not 100% sure about our style guide here, but should key and keys be in `?

goderbauer · 2018-01-12T19:03:07Z

packages/flutter/lib/src/semantics/semantics.dart

I believe this needs a new line after See also to render correctly.

tiny nit: everywhere else in this file we seem to be using * instead of -.

googlebot · 2018-01-19T18:32:54Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

goderbauer · 2018-01-19T18:21:48Z

packages/flutter/lib/src/rendering/binding.dart

change traversal here as well? Also, can you change the help text that shows up when you press 'h' during a hot run to reflect the change?

OK, I think I got that.

I believe, the changed files were maybe not added to the PR? This is the place I was talking about:

flutter/packages/flutter_tools/lib/src/resident_runner.dart

Line 825 in 1859e82

printStatus('For layers (debugDumpLayerTree), use "L"; accessibility (debugDumpSemantics), "S" (traversal order) or "U" (inverse hit test order).');

Pressing "S" no longer gives you traversal order, right?

You're correct. I forgot to upload to the PR.

goderbauer · 2018-01-19T18:36:43Z

packages/flutter/lib/src/semantics/semantics.dart

I believe this should be "## Sample code". That way, travis will run the analyzer over this code snippet and we have some kind of guarantee that it doesn't go out of date.

It still seems to show up as "Example usage" instead of "## Sample code"?

goderbauer · 2018-01-19T18:37:57Z

packages/flutter/lib/src/semantics/semantics.dart

+1 to the comprehensive documentation with example.

goderbauer · 2018-01-19T18:46:29Z

packages/flutter/lib/src/semantics/semantics.dart

I still feel a little uncomfortable about sending over an index for a global order. When you have a parent node with a 100 child nodes all specifying an order and you add a node at the very beginning, we will have to send all 100 child nodes over again, decode them on the engine side, translate them into platform-specific nodes, etc (we would even tell the OS that these 100 nodes have changed even though they didn't really).

The only thing that has changed is that after the parent node we now need to traverse to the new node instead of the old node 1.

I'd much prefer to just send that minimal diff. Or we should create a benchmark verifying that doing the ordering like this doesn't have a performance impact (the benchmark could maybe measure the performance of adding a child node to an existing list of n child nodes with and without an order set).

OK, done. I now push down the nextNodeId instead of the traversal index, and only mark those nodes where the next node has changed as dirty. I'm not sure what impact this will have on implementing it for iOS yet, however.

googlebot · 2018-01-19T19:16:08Z

CLAs look good, thanks!

goderbauer · 2018-01-29T18:26:34Z

packages/flutter/lib/src/rendering/binding.dart

debugDumpSemanticsTreeInTraversalOrder -> debugDumpSemanticsTreeInGeometricOrder

Sorry, I did that, but forgot to upload the changes...

goderbauer · 2018-01-29T18:27:22Z

packages/flutter/lib/src/widgets/basic.dart

nit: [sortOrder]

goderbauer · 2018-01-29T18:31:06Z

packages/flutter/lib/src/rendering/binding.dart

I believe, the changed files were maybe not added to the PR? This is the place I was talking about:

flutter/packages/flutter_tools/lib/src/resident_runner.dart

Line 825 in 1859e82

printStatus('For layers (debugDumpLayerTree), use "L"; accessibility (debugDumpSemantics), "S" (traversal order) or "U" (inverse hit test order).');

Pressing "S" no longer gives you traversal order, right?

goderbauer · 2018-01-29T18:31:58Z

packages/flutter/lib/src/semantics/semantics.dart

update the doc comment?

goderbauer · 2018-01-29T18:33:30Z

packages/flutter/lib/src/semantics/semantics.dart

I find this paragraph hard to understand. Maybe add an example?

Or add an "See also" entry linking to SemanticsSortOrder, which seems to explain this in more detail.

Added the See also.

goderbauer · 2018-01-29T18:35:10Z

packages/flutter/lib/src/semantics/semantics.dart

remove duplicated sentence?

goderbauer · 2018-01-29T18:37:46Z

packages/flutter/lib/src/semantics/semantics.dart

Should this include the nextNodeId if set? I imagine it might be useful when you debug traversal order. And this is what gets printed out when you hit "S" during a hot run.

Good point. Added.

goderbauer · 2018-01-29T18:39:43Z

packages/flutter/lib/src/semantics/semantics.dart

I was under the impression that the accompanying engine PR completely removed geometric comparison from the platform side? What's left to move?

And without that, what is actually now breaking ties?

As we discussed, nothing is breaking ties. The order of a tie is currently arbitrary, until I've implemented the geometric comparison again on the framework side. It doesn't help to continue to compare geometry on the engine side, since they're all assigned nextNodeIds already.

goderbauer · 2018-01-29T18:49:30Z

packages/flutter/lib/src/semantics/semantics.dart

Can we document somewhere around the setter/getter for nextNodeId (or for _updateNextNodeId) what setting this to -1 means? Especially, does it mean: Not followed by any node? Or the order of the next node has not been defined?

Done. It means not followed by another node, and is the value that the last node in the list gets.

goderbauer · 2018-01-29T18:52:32Z

packages/flutter/lib/src/semantics/semantics.dart

It still seems to show up as "Example usage" instead of "## Sample code"?

This adds support for semantics traversal ordering. It is a companion to flutter/flutter#14060, adding support for a sortIndex in the semantics data passed to the engine. Addresses flutter/flutter#12187

goderbauer

LGTM

This adds an API for defining the semantic node traversal order. It adds a sortOrder argument to the Semantics widget, which is a class that can define a list of sort keys to sort on. The keys are sorted globally so that an order that doesn't have to do with the current widget hierarchy may be defined. It also adds a shortcut sortKey argument to the Semantics widget that simply sets the sortOrder to just contain that key. The platform side (flutter/engine#4540) gets an additional member in the SemanticsData object that is an integer describing where in the overall order each semantics node belongs. There is an associated engine-side change that takes this integer and uses it to order widgets for the platform's accessibility services.

gspencergoog requested review from Hixie and goderbauer January 12, 2018 01:21

googlebot added the cla: yes label Jan 12, 2018

gspencergoog mentioned this pull request Jan 12, 2018

Adding semantics traversal order support flutter/engine#4540

Merged

gspencergoog force-pushed the traversal branch 2 times, most recently from 454428c to f51562a Compare January 12, 2018 18:24

goderbauer reviewed Jan 12, 2018

View reviewed changes

gspencergoog force-pushed the traversal branch 7 times, most recently from 8f3187a to c74ffac Compare January 19, 2018 00:14

googlebot added cla: no and removed cla: yes labels Jan 19, 2018

goderbauer reviewed Jan 19, 2018

View reviewed changes

gspencergoog force-pushed the traversal branch from 3d69c3e to 1a01692 Compare January 19, 2018 19:16

googlebot added cla: yes and removed cla: no labels Jan 19, 2018

gspencergoog force-pushed the traversal branch 2 times, most recently from ceed5a1 to 3717bd3 Compare January 26, 2018 21:50

goderbauer reviewed Jan 29, 2018

View reviewed changes

gspencergoog force-pushed the traversal branch from 8b7220b to 6e112a1 Compare January 30, 2018 18:50

gspencergoog force-pushed the traversal branch 3 times, most recently from e018478 to 245a637 Compare January 30, 2018 22:49

gspencergoog force-pushed the traversal branch 5 times, most recently from fa5f42e to 408ed84 Compare February 5, 2018 19:05

goderbauer approved these changes Feb 5, 2018

View reviewed changes

Adds the semantic node traversal API.

2848f83

gspencergoog force-pushed the traversal branch from 408ed84 to 2848f83 Compare February 5, 2018 21:45

gspencergoog merged commit 14309b9 into flutter:master Feb 6, 2018

goderbauer mentioned this pull request Feb 9, 2018

TalkBack linear navigation busted #14600

Closed

gspencergoog deleted the traversal branch February 12, 2018 18:12

engine-flutter-autoroll mentioned this pull request Nov 28, 2019

Roll engine 35937eb31240..4bdd15cd5438 (5 commits) #45741

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2021

Adds the semantic node traversal order API. #14060

Adds the semantic node traversal order API. #14060

Uh oh!

Conversation

gspencergoog commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

googlebot commented Jan 19, 2018

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!

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!

googlebot commented Jan 19, 2018

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

gspencergoog commented Jan 12, 2018 •

edited

Loading