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

goderbauer · 2017-06-01T01:44:35Z

Follow-up to #10305

goderbauer · 2017-06-01T01:45:26Z

@Hixie @abarth How's this approach?

abarth · 2017-06-01T03:43:51Z

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

s/left/previous

or "earlier"

also maybe s/siblings/nodes

abarth · 2017-06-01T03:45:09Z

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

Usually its a better pattern to call a method on this rather than testing this against a specific subclass.

Yeah, we don't want to do it this way, because it would prevent you from applying the same logic to e.g. RenderSector or RenderSliver subclasses. This should be something that any class can do. Maybe something you call on the SemanticsNode in your annotator?

I refactored this to use the same mechanism we use for isSemanticBoundary. No more testing this against a specific subclass and no more cyclic imports!

abarth · 2017-06-01T03:46:02Z

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

This import creates a circular dependency. Although Dart can cope with circular dependencies, we avoid them because they tend to indicate a design flaw (e.g., the test against a specific subclass below).

Hixie · 2017-06-02T16:51:55Z

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

+  }) : super(renderObjectOwner: renderObjectOwner, dropSemanticsOfPreviousSiblings: dropSemanticsOfPreviousSiblings);
+
+  @override
+  Iterable<SemanticsNode> compile({ _SemanticsGeometry geometry, SemanticsNode currentSemantics, SemanticsNode parentSemantics }) sync* {}


nit: { } for empty code blocks; {} for empty maps

Hixie · 2017-06-02T16:52:01Z

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

+class _EmptySemanticsFragment extends _SemanticsFragment {
+  _EmptySemanticsFragment({
+    @required RenderObject renderObjectOwner,
+    bool dropSemanticsOfPreviousSiblings


nit: trailing comma

Hixie · 2017-06-02T16:55:54Z

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

+  ///
+  /// If `true` is returned, the [SemanticsNode]s for all siblings and cousins
+  /// of this node, that were painted before this node, are dropped from the
+  /// semantics tree up until a semantic boundary is reached.


"earlier in a depth-first pre-order traversal"

Hixie · 2017-06-02T17:00:03Z

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

  bool get isSemanticBoundary => false;

+  /// Whether this [RenderObject] makes other [RenderObject]s previously painted
+  /// in the same semantics container unreachable for accessibility purposes.


semantics container should reference isSemanticsBoundary.
...and probably be phrased in terms of semantics boundaries rather than containers.

Hixie · 2017-06-02T17:00:59Z

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

+  ///
+  /// Paint order as established by [visitChildrenForSemantics] is used to
+  /// determine if a node is previous to this one.
+  bool get isBlockingSemanticsOfPreviouslyPaintedNodes => false;


this should define how it interacts when set on a node that is also a boundary.

I suspect the best behaviour is that it acts on the "outside" of the boundary, but I'm not sure that's what the code currently does.

Added documentation and changed the code to match that behavior.

Hixie · 2017-06-02T17:01:29Z

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

    });
+    if (isSemanticBoundary) {
+      // Don't propagate [dropSemanticsOfPreviousSiblings] past a semantic boundary.
+      dropSemanticsOfPreviousSiblings = false;


i don't understand this block.

At some point when we walk up the tree we need to stop dropping children. The semantic boundary is that point where we stop.

Oh this is about propagating up. I somehow was confused and thought it was propagating down.

I'll add the word "up" to the description.

Hixie · 2017-06-02T17:03:17Z

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

      description.add('layer: $_layer');
    if (_semantics != null)
      description.add('semantics: $_semantics');
+    description.add('isSemanticBoundary: $isSemanticBoundary');


Rather than always including this (which will make the output verbose and repetitive), I would have:

if (isBlockingSemanticsOfPreviouslyPaintedNodes) description.add('blocks semantics of earlier render objects below the common boundary'); if (isSemanticBoundary) description.add('semantic boundary');

Hixie · 2017-06-02T17:03:57Z

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


+/// A widget that drops the semantics of all widget that were painted before it
+/// in the same semantic container.
+///


please add some more depth to this specific documentation, e.g. giving an example of where you'd use this

Hixie · 2017-06-02T17:04:31Z

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

+/// See also:
+///
+/// * [BlockSemantics] which drops semantics of previously painted sibling and
+///   cousin widgets.


s/previously painted sibling and cousin widgets/widgets earlier in the tree/.

Hixie · 2017-06-02T17:05:36Z

packages/flutter/test/widgets/basic_test.dart

@@ -0,0 +1,99 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.


maybe rename basic_test to semantics_9_test?

Hixie · 2017-06-02T17:06:17Z

packages/flutter/test/material/dialog_test.dart

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.



you also need a test of what happens if a render object is both a semantics boundary and has the new flag set.

Hixie · 2017-06-02T17:07:44Z

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

+/// * [ExcludeSemantics] which drops all semantics of its descendants.
+class BlockSemantics extends SingleChildRenderObjectWidget {
+  /// Creates a widget that excludes the semantics of all widgets painted before
+  /// it in the same semantic container.


(but leave this one the same, so that the reader can see different ways to describe this same behaviour and hopefully understand one of them!)

Hixie · 2017-06-02T17:08:51Z

This seems pretty solid. Just some minor notes.

Hixie · 2017-06-02T19:59:57Z

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

+  ///
+  /// If [isSemanticBoundary] and [isBlockingSemanticsOfPreviouslyPaintedNodes]
+  /// is set on the same node, all previously painted siblings and cousins
+  /// up until the next semantic boundary are dropped.


This paragraph seems a bit ambiguous. What is the "next semantic boundary"? We're walking a multibranched tree in multiple directions...

Rephrased. PTAL.

Hixie · 2017-06-03T01:12:59Z

googlebot added the cla: yes label Jun 1, 2017

abarth reviewed Jun 1, 2017

View reviewed changes

goderbauer added 12 commits June 1, 2017 15:31

It works

02d8367

refactored

44b9310

cleanup

7caa30a

cleanup++

fb94881

Added tests

4dccbed

moar tests

e942b84

Add BlockSemantics to scaffold

b2a7c78

mark for lyout

1df732a

++

752aeda

add tests

954d579

doc

0b8c69d

Review comments and touch-ups

838f5b7

goderbauer force-pushed the blockSemantics branch from 52755cd to 838f5b7 Compare June 1, 2017 22:43

goderbauer changed the title ~~[WIP] Adds a widgets that blocks all semantics of widgets below it in paint order within the same container~~ Adds a widgets that blocks all semantics of widgets below it in paint order within the same container Jun 1, 2017

Hixie reviewed Jun 2, 2017

View reviewed changes

Review comments

3786c8b

Hixie reviewed Jun 2, 2017

View reviewed changes

rephrasing

1b09e61

goderbauer force-pushed the blockSemantics branch from 95b8396 to 1b09e61 Compare June 2, 2017 20:36

goderbauer merged commit 104725f into flutter:master Jun 3, 2017

goderbauer deleted the blockSemantics branch June 3, 2017 01:14

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 13, 2021

		@@ -0,0 +1,99 @@
		// Copyright 2017 The Chromium Authors. All rights reserved.

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

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

Uh oh!

Conversation

goderbauer commented Jun 1, 2017

Uh oh!

goderbauer commented Jun 1, 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!

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 Jun 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jun 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants