Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

a11y: remove SemanticsSortOrder; sort locally only; semanticsOwner post-test check #15537

Merged
merged 4 commits into from Mar 19, 2018

Conversation

Projects
None yet
4 participants
@yjbanov
Copy link
Contributor

commented Mar 14, 2018

This PR prepares us for the upcoming a11y traversal changes. Namely:

  • Remove SemanticsSortOrder - we now rely purely on SemanticsSortKey for sorting.
  • Sorting is done only locally; we do not sort across branches.
  • Add a post-test check that all SemanticsHandles are disposed of. This prevents cross-test contamination. Ever wondered why in some tests the id starts with 1 and in others it starts with 2? Now, if you use debugResetSemanticsIdCounter before your test, all ids are guaranteed to start with 1 (the root node id remains to be hard-coded to 0).

Breaking change announcement: https://groups.google.com/forum/#!topic/flutter-dev/iCoLnW31heE

@yjbanov yjbanov requested review from goderbauer and gspencergoog Mar 14, 2018

@googlebot googlebot added the cla: yes label Mar 14, 2018

///
/// This defines how this node will be sorted with the other semantics nodes
/// This defines how this node are sorted with the other semantics nodes

This comment has been minimized.

Copy link
@goderbauer

goderbauer Mar 14, 2018

Member

mention that this is only a local sort amongst siblings?

This comment has been minimized.

Copy link
@yjbanov

yjbanov Mar 16, 2018

Author Contributor

Done.

@@ -357,13 +357,13 @@ void _defineTests() {
new TestSemantics.rootChild(
id: 1,
previousNodeId: -1,
nextNodeId: expectedId,
nextNodeId: -1,

This comment has been minimized.

Copy link
@goderbauer

goderbauer Mar 14, 2018

Member

This looks odd now. Does -1 mean "not specified"? Can we make this the default so you don't have to write it out in your tests?

This comment has been minimized.

Copy link
@yjbanov

yjbanov Mar 16, 2018

Author Contributor

Done (see b2e2aa0)

yjbanov added some commits Mar 16, 2018

update accessibility test framework
- default nextNodeId/previousNodeId to -1
- stop treating null as opt-out from value testing
- add `id`, `TestSemantics.root`, and `tags` to the suggested code in the TestSemantics failure message
- fix a small bug with raw string escaping
- update all tests accordingly
@yjbanov

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2018

Made the requested test framework updates. PTAL.

@goderbauer
Copy link
Member

left a comment

LGTM

@gspencergoog can you take a look as well since you know this code the best?

// performed irrespective of whether the owner was created via
// SemanticsTester or directly. When the test succeeds, this tear-down
// becomes a no-op.
addTearDown(dispose);

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Mar 16, 2018

Contributor

Good find! I have been meaning to check on this because it causes extra failures when a test fails, making it harder to see the real failure sometimes.

@gspencergoog

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2018

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

This looks really good, Yegor. Thanks for all of your work here.

@yjbanov yjbanov merged commit 9c49255 into flutter:master Mar 19, 2018

3 of 4 checks passed

flutter-build Flutter build is currently broken. Be careful when merging this PR.
Details
cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

aam added a commit to aam/flutter that referenced this pull request Mar 19, 2018

Add const to list literal.
This is follow-up to flutter#15537 to make dart1 coverage test happy.

@aam aam referenced this pull request Mar 19, 2018

Merged

Add const to list literal. #15693

aam added a commit that referenced this pull request Mar 19, 2018

Add const to list literal. (#15693)
This is follow-up to #15537 to make dart1 coverage test happy.

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018

a11y: remove SemanticsSortOrder; sort locally only; semanticsOwner po…
…st-test check (flutter#15537)

* a11y: remove SemanticsSortOrder; sort locally only; semanticsOwner post-test check

* update accessibility test framework

- default nextNodeId/previousNodeId to -1
- stop treating null as opt-out from value testing
- add `id`, `TestSemantics.root`, and `tags` to the suggested code in the TestSemantics failure message
- fix a small bug with raw string escaping
- update all tests accordingly

* fix sortKey doc

* prefer const over final

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018

Add const to list literal. (flutter#15693)
This is follow-up to flutter#15537 to make dart1 coverage test happy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.