Fix semantics_tester #34368

dnfield · 2019-06-13T00:45:10Z

Description

Semantics_tester was broken. If a node had multiple children, it was checking the first and then bailing.

Fixing that has revealed other bugs.

I talked with @GaryQian and I think the test_text tests now have the correct fixes applied in paragraph.dart. Gary to review.

I'm not as confident on the other changes - DatePicker seems like it may be right now, ~~but not sure why we get all those empty nodes. /cc @goderbauer and/or @darrenaustin~~ we get them because there are empty container placeholders there.

Sliver tests I think are right - they were just missing implicit scrolling flags and not caught previously due to this bug.

Semantics traversal test seems like it may be a different bug. It's possible the 64 bit rect stuff impacted this and we didn't catch it because of the brokeness of the tester.

This regression was introduced in #16253, where a return true that used to live in a closure was moved to a for loop.

Related Issues

Fixes #34363

Tests

I added the following tests:

Added a sanity check for the tester. This test fails without these changes. This is similar to a test I was trying to write, and I was confused about why it was passing when it was so clearly wrong.

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 Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

packages/flutter/test/widgets/semantics_tester_test.dart

jonahwilliams · 2019-06-13T00:50:32Z

packages/flutter/test/widgets/semantics_traversal_test.dart

@@ -202,13 +202,13 @@ void main() {
    await tester.test(
      textDirection: TextDirection.ltr,
      children: children,
-      expectedTraversal: 'A B C D E F G H I J K L M',
+      expectedTraversal: 'A B C D E G H F I J K L M',


This is ... odd

Yes, I think something's wrong here.

Unfortunately, the regression in semantics_tester was introduced in the same commit where these tests were added. I tried checking out d354096 and modifying the tester so that it's closer to the state in this PR and those tests still pass. I'll try to figure out what's going on here, unless @yjbanov might know.

Looks like this regressed in f2442b8, which was part of v0.8.0 tag.

(And wasn't caught because the tester was broken)

Got this sorted out - changed the deflate constant from 0.1 to 1e-15. Anything bigger than 1e-15 was failing.

Pushed this back to .1. The right fix is to update the traversal logic.

dnfield · 2019-06-13T04:53:11Z

packages/flutter/test/material/search_test.dart

@@ -496,6 +496,17 @@ void main() {
                label: routeName,
                textDirection: TextDirection.ltr,
                children: <TestSemantics>[
+                  TestSemantics(


If this isn't correct, we have to put a sort key on it I think.

This seems wrong if things are dumped in traversal order (which I think they are).

I am surprised that it would need a sort key, though. The suggestions come physically after the back button and the text field.

(We should verify that talkback and voiceover do the right thing here on device)

This is actually how things are ordered today. It's not very clear to me why that's the case - even without any changes here. Filed #34432 and added a TODO.

Talked offline - will close the other bug. Traversal order test was broken and needed to be updated.

dnfield · 2019-06-13T04:53:49Z

packages/flutter/test/widgets/text_test.dart

@@ -414,7 +414,7 @@ void main() {
            TestSemantics(
              label: 'INTERRUPTION',
              textDirection: TextDirection.rtl,
-              rect: const Rect.fromLTRB(448.0, 0.0, 488.0, 80.0),
+              rect: const Rect.fromLTRB(0.0, 0.0, 40.0, 80.0),


@GaryQian is this expected?

This may actually be a bug, but I'll look at that separately. I think in practice the rects still show up in the right place after transforms, but it may be better to make sure the values are rolled into here too. I think we can go ahead with this for now in the context of your semantics fix and ill fix this when I take a deeper look.

If we don't fix this here, we should at least file an issue and link it from here.

I believe I have a fix for this that I'll be landing separately. I have a patch ready to go that will be changing this again, but don't want to fiddle with it or risk having yet another piece to worry about as this patch is blcoking the google3 roll now.

jonahwilliams

LGTM for changes to rect deflate

yjbanov · 2019-06-13T15:30:27Z

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

@@ -2230,7 +2230,7 @@ class _SemanticsSortGroup extends Comparable<_SemanticsSortGroup> {
    final List<_BoxEdge> edges = <_BoxEdge>[];
    for (SemanticsNode child in nodes) {
      // Using a small delta to shrink child rects removes overlapping cases.
-      final Rect childRect = child.rect.deflate(0.1);
+      final Rect childRect = child.rect.deflate(1e-15);


This may not be the right fix. The rects are deflated not to remove rounding errors, but to remove minor (even if visible) overlaps from consideration. Crazy idea: what if you not fix this? Instead, change the test, leave a TODO and file a bug to address this more carefully.

/cc @jonahwilliams

My thought with this is that removing a trivially small amount like this should still make them not evaluate as overlapping.

Our original thought here was that even when things are overlapping visibly, if the overlap is smallish, it shouldn't count as overlapping...

That ends up breaking traversal order :(

So what's the vote here? Add the very small deflate, or remove it entirely and add a TODO?

The canonical example is the categories on the flutter gallery home page. It should go left to right, but before it was going top to bottom.

If it still goes left to right with this change I'm okay with it. If it doesn't, we might as well remove the deflate and go back to the drawing board

The categories sample still worked with this change, but it's actually the traversal order we want to change (which was going top to bottom instead of left to right). I've updated that test to go left to right and put back the deflate to 0.1.

yjbanov · 2019-06-13T15:30:43Z

Good catch! Thanks for fixing!

yjbanov

goderbauer · 2019-06-13T16:09:18Z

packages/flutter/test/material/date_picker_test.dart

                                children: <TestSemantics>[
+                                  TestSemantics(),


Why do we have all these extra empty nodes? They really shouldn't be here...

I believe they're containers/placeholders to make the calendar day fall under the right slot.

Empty placeholders should not have a semantics node, though. I thought we filter those out...

They're from the ExcludeSemantics around the weekday header and more empty nodes for the containers for placeholders.

So 7 weekday headers + 5 placeholder containers = 12 empty semantics nodes.

I'm fine with changing that but I think if we do it should be in a separate PR.

OK, Can you add a TODO and file an issue so it doesn't get lost?

goderbauer · 2019-06-13T16:15:49Z

packages/flutter/test/material/search_test.dart

@@ -496,6 +496,17 @@ void main() {
                label: routeName,
                textDirection: TextDirection.ltr,
                children: <TestSemantics>[
+                  TestSemantics(


This seems wrong if things are dumped in traversal order (which I think they are).

I am surprised that it would need a sort key, though. The suggestions come physically after the back button and the text field.

(We should verify that talkback and voiceover do the right thing here on device)

goderbauer · 2019-06-13T16:16:46Z

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

@@ -2230,7 +2230,7 @@ class _SemanticsSortGroup extends Comparable<_SemanticsSortGroup> {
    final List<_BoxEdge> edges = <_BoxEdge>[];
    for (SemanticsNode child in nodes) {
      // Using a small delta to shrink child rects removes overlapping cases.
-      final Rect childRect = child.rect.deflate(0.1);
+      final Rect childRect = child.rect.deflate(1e-15);


Our original thought here was that even when things are overlapping visibly, if the overlap is smallish, it shouldn't count as overlapping...

goderbauer · 2019-06-13T16:17:20Z

packages/flutter/test/widgets/semantics_tester_test.dart

@@ -0,0 +1,44 @@
+// Copyright 2019 The Flutter Authors. All rights reserved.


+1 for adding these tests! Thank you!

goderbauer · 2019-06-13T16:23:06Z

we get them because there are empty container placeholders there.

What does this mean? Empty containers should not produce a semantics node?

GaryQian · 2019-06-13T17:29:59Z

Paragraph changes LGTM!

goderbauer

LGTM

Thank you!

* master: (24 commits) [flutter_tool,fuchsia] Update the install flow for packaging migration. (flutter#34447) SliverFillRemaining flag for different use cases (flutter#33627) SizedBox documentation (flutter#34424) Change API doc link to api.dart.dev (flutter#34388) 2589785 Roll src/third_party/skia 87e885038893..c3252a04b377 (3 commits) (flutter/engine#9327) (flutter#34484) ace5d59 Fix rawTypes errors in Android embedding classes (flutter/engine#9326) (flutter#34481) bf0def6 Roll src/third_party/skia 4c4945a25248..87e885038893 (1 commits) (flutter/engine#9325) (flutter#34471) Roll engine f1d821d..6f5347c (13 commits) (flutter#34466) Allow "from" hero state to survive hero animation in a push transition (flutter#32842) Roll pub dependencies (flutter#33677) Skip flaky test on Windows (flutter#34464) Allow flaky tests to pass or fail and mark web tests as flaky (flutter#34456) Dont depend on web SDK unless running tests on chrome (flutter#34457) Fix semantics_tester (flutter#34368) Include raw value in Diagnostics json for basic types (flutter#34417) Refactor Gradle plugin (flutter#34353) Allow web tests to fail in cirrus config (flutter#34436) skip bottom_sheet (flutter#34430) Remove unused flag `--target-platform` from `flutter run` (flutter#34369) Extract DiagnosticsNode serializer from WidgetInspector (flutter#34012) ...

Fix semantics_tester

4e6a539

dnfield requested review from goderbauer, GaryQian and jonahwilliams June 13, 2019 00:45

googlebot added the cla: yes label Jun 13, 2019

dnfield requested a review from yjbanov June 13, 2019 00:45

jonahwilliams reviewed Jun 13, 2019

View reviewed changes

packages/flutter/test/widgets/semantics_tester_test.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Jun 13, 2019

View reviewed changes

dnfield added 2 commits June 12, 2019 20:16

copyright

85d811b

Fix traversal order and update tests

16a3fb6

dnfield commented Jun 13, 2019

View reviewed changes

version constraint

959449e

jonahwilliams approved these changes Jun 13, 2019

View reviewed changes

typo

525016f

yjbanov reviewed Jun 13, 2019

View reviewed changes

yjbanov approved these changes Jun 13, 2019

View reviewed changes

goderbauer reviewed Jun 13, 2019

View reviewed changes

Piinks added a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Jun 13, 2019

dnfield mentioned this pull request Jun 13, 2019

Material search ends up with the wrong traversal order #34432

Closed

GaryQian mentioned this pull request Jun 13, 2019

Fix semantics offset rect for inline widgets #34433

Open

Add todos

6ef09a6

dnfield mentioned this pull request Jun 13, 2019

Semantics fixes #34434

Merged

9 tasks

fix traversal order

5fa24d8

GaryQian mentioned this pull request Jun 13, 2019

Fix semantics labeling for paragraphs #34454

Closed

goderbauer approved these changes Jun 13, 2019

View reviewed changes

dnfield merged commit ea6b758 into flutter:master Jun 13, 2019

dnfield deleted the semantic_test_fix branch June 13, 2019 23:15

kiku-jw pushed a commit to kiku-jw/flutter that referenced this pull request Jun 14, 2019

Fix semantics_tester (flutter#34368)

96be6a7

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

		@@ -0,0 +1,44 @@
		// Copyright 2019 The Flutter Authors. All rights reserved.

Fix semantics_tester #34368

Fix semantics_tester #34368

Conversation

dnfield commented Jun 13, 2019 • edited Loading

Description

Related Issues

Tests

Checklist

Breaking Change

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

Choose a reason for hiding this comment

jonahwilliams 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yjbanov commented Jun 13, 2019

yjbanov 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

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

goderbauer commented Jun 13, 2019

GaryQian commented Jun 13, 2019

goderbauer left a comment

Choose a reason for hiding this comment

dnfield commented Jun 13, 2019 •

edited

Loading