Remove inactive elements from the semantic tree #10305

goderbauer · 2017-05-24T23:52:36Z

In this PR:

OverlayEntries can be a semantic barriers meaning that everything below that entry is no longer included in the semantics tree (used for e.g. Dialogs).
When the Drawer in a Scaffold is open all elements outside of the drawer are no longer part of the semantics tree.
A new includesNodeWithLabel matcher for tests.

Hixie · 2017-05-24T23:55:31Z

packages/flutter/lib/src/material/drawer.dart

+  ///
+  /// Gets called whenever the drawer's state switches from open to closed or
+  /// vice versa.
+  final VoidCallback onStateChanged;


maybe make this a ValueChanged<bool> callback and pass the value of isClosed?

Hixie · 2017-05-24T23:55:55Z

packages/flutter/lib/src/material/scaffold.dart

  }

-  void _addIfNonNull(List<LayoutId> children, Widget child, Object childId) {
+  void _addChild(List<LayoutId> children, Widget child, Object childId, {bool excludeSemantics: false}) {


nit: spaces around the named arguments

Hixie · 2017-05-24T23:58:39Z

packages/flutter/lib/src/material/scaffold.dart

+        new DrawerController(
          key: _drawerKey,
+          onStateChanged: () {
+            // Rebuild Scaffold when drawer opens/closes to update semantics tree.


this would be slightly clearer if there was a local member field _drawerOpen which you used to determine the exclude semantics logic, and you just set it here inside the setState. it would avoid the empty callback.

if you prefer the current setup, then put this comment inside the setState callback's body.

Hixie · 2017-05-24T23:59:07Z

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

-  RenderExcludeSemantics({ RenderBox child }) : super(child);
+  RenderExcludeSemantics({
+    RenderBox child,
+    bool excluding : true,


nit: extraneous space before colon

Hixie · 2017-05-24T23:59:20Z

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

+
+  @override
+  void visitChildrenForSemantics(RenderObjectVisitor visitor) {
+    if (excluding) {


nit: no braces

Hixie · 2017-05-24T23:59:52Z

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

+  }) : assert(excluding != null),
+       super(key: key, child: child);
+
+  /// Whether this widget is included in the semantics tree.


excluded, presumably

Hixie · 2017-05-25T00:00:23Z

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

 /// call [State.setState], the user's battery will be drained unnecessarily.
 ///
+/// If this entry is declared to be a [semanticsBarrier], then entries below
+/// this one will not be included in the semantic tree, e.i. those entries will


e.i. -> i.e.

but actually, just "e.g." because it means other things too

Hixie · 2017-05-25T00:01:26Z

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

 ///
+/// If this entry is declared to be a [semanticsBarrier], then entries below
+/// this one will not be included in the semantic tree, e.i. those entries will
+/// be unreachable via accessibility means.


"via accessibility means" -> "to accessibility tools" or "via accessibility user agents" or some such.

Hixie · 2017-05-25T00:01:42Z

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

+  }) : _opaque = opaque, _maintainState = maintainState, _semanticsBarrier = semanticsBarrier {
    assert(builder != null);
    assert(opaque != null);
    assert(maintainState != null);


Hixie · 2017-05-25T00:02:19Z

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

+  /// Whether this entry is a semantics barrier for the entries below it.
+  ///
+  /// If `true`, entries below this entry will not be included in the semantic
+  /// tree. That means they are not reachable via accessibility means.


same comment here. "This means, for example, that they are not reachable using accessibility tools."

Hixie · 2017-05-25T00:02:38Z

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

+      return;
+    _semanticsBarrier = value;
+    assert(_overlay != null);
+    _overlay._didChangeEntryOpacity(); // TODO(goderbauer): rename?


yes, rename. :-)

Hixie · 2017-05-25T00:03:52Z

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

  @override
  Iterable<OverlayEntry> createOverlayEntries() sync* {
-    yield new OverlayEntry(builder: _buildModalBarrier);
+    yield new OverlayEntry(builder: _buildModalBarrier, semanticsBarrier: true);


looking at this it's sad that we can't have the modal barrier itself somehow occlude all the semantics below it rather than having it done in the overlay. but i don't see how we could sanely do that.

Hixie · 2017-05-25T00:04:55Z

packages/flutter/test/widgets/semantics_tester.dart

+}
+
+/// Asserts that a node in the semantics tree of [SemanticsTester] has [label].
+Matcher includesNodeWithLabel(String label) => new _IncludesNodeWithLabel(label);


Hixie · 2017-05-25T00:05:41Z

This is fantastic.

Hixie · 2017-05-25T00:05:51Z

goderbauer · 2017-05-25T00:55:48Z

This patch introducesExcludeSemantics widgets between Stack and its children (which can be ParentDataWidgets). This causes asserts to be thrown (see https://travis-ci.org/flutter/flutter/jobs/235859177#L549).

@abarth Do you have an idea for a clean solution to fix this? Hixie and I were out of ideas today.

abarth · 2017-05-25T06:06:16Z

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

      final OverlayEntry entry = _entries[i];
      if (onstage) {
-        onstageChildren.add(new _OverlayEntry(entry));
+        onstageChildren.add(new ExcludeSemantics(


This isn't going to work because part of the contract Overlay has with its entries is that they'll be contained in a Stack.

abarth · 2017-05-25T06:10:42Z

I'd try a different approach.

Currently, we have RenderExcludeSemantics, which hides a subtree of render tree from the semantics compiler. I'd try adding a RenderSemanticsBarrier that prevents the semantics compiler from seeing anything before it in paint order. I'd then teach ModalBarrier to build a SemanticsBarrier.

Essentially, you're trying to use a tree-order primitive to do a paint-order job. Instead, you might have more success if you invent a paint-order primitive to do a paint-order job.

Hixie · 2017-05-25T16:59:47Z

Yeah, that was the other approach we talked about, though we had dismissed it as impractical. Speaking with Adam this morning, Adam convinced me that we should take a look at that again. It certainly would be more idiomatic and would more closely match what we're really doing.

Hixie · 2017-05-26T16:42:37Z

@goderbauer I'm going to close this PR since we've concluded this approach isn't going to work. I hope that's ok. (I'm going through open PRs tidying things up.)

goderbauer added 7 commits May 24, 2017 12:30

[WIP] Prune semantics tree for overlays

ae756b6

Prune semantics tree for Scaffold's Drawer

0effb99

Added docs (scaffold)

c4adc70

Add documentation (overlay)

be8a0f8

Added tests for overlay

475683a

Added tests for scaffold

c16a644

minor fixes

e533206

goderbauer requested a review from Hixie May 24, 2017 23:52

googlebot added the cla: yes label May 24, 2017

Hixie reviewed May 24, 2017

View reviewed changes

Hixie reviewed May 25, 2017

View reviewed changes

review comments

4b43710

abarth reviewed May 25, 2017

View reviewed changes

Hixie closed this May 26, 2017

goderbauer mentioned this pull request Jun 1, 2017

Adds a widgets that blocks all semantics of widgets below it in paint order within the same container #10425

Merged

goderbauer deleted the excludeSematnics branch September 25, 2017 22:19

goderbauer added the a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) label Oct 17, 2017

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

Remove inactive elements from the semantic tree #10305

Remove inactive elements from the semantic tree #10305

Uh oh!

Conversation

goderbauer commented May 24, 2017

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!

Hixie commented May 25, 2017

Uh oh!

Hixie commented May 25, 2017

Uh oh!

goderbauer commented May 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abarth commented May 25, 2017

Uh oh!

Hixie commented May 25, 2017

Uh oh!

Hixie commented May 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants