SliverEnsureSemantics#165589
Conversation
109391c to
665bc40
Compare
|
The framework portion of this is ready for an initial review, i'm still making sure the web engine part of this is correct. |
b93cf6c to
1e38a79
Compare
855e9cf to
81f50e7
Compare
| /// to ensure a sliver is included in the semantics tree regardless of its geometry. | ||
| bool get ensureSemantics => _ensureSemantics; | ||
| bool _ensureSemantics = false; | ||
| set ensureSemantics(bool value) { |
There was a problem hiding this comment.
Is this setter used any where? it is also a bit weird to implement setter for a property that is going to be override by subclass
There was a problem hiding this comment.
Removed this setter.
| (RenderSliver sliver) => sliver.geometry!.visible || sliver.geometry!.cacheExtent > 0.0, | ||
| (RenderSliver sliver) => | ||
| sliver.geometry!.visible || | ||
| sliver.geometry!.cacheExtent > 0.0 || |
There was a problem hiding this comment.
looks like there as long as the sliver has cacheExtent they will also be preserved. can the SliverEnsureSemantics sets a cacheExtent in itself to avoid adding ensureSemantics?
There was a problem hiding this comment.
I think that may interfere with some of the logic in layoutChildSequence which uses a sliver's cacheExtent to determine the remaining cache extent.
flutter/packages/flutter/lib/src/rendering/viewport.dart
Lines 657 to 660 in 975a677
Maybe with a small amount of items and setting cacheExtent to a very small number like 0.1 or less this does not have much impact, but I can see if someone has many this may become an issue. It seems strange that these invisible items would affect the remaining cache extent. What do you think?
There was a problem hiding this comment.
I think relying on the slivers cacheExtent wouldn't work for the describeApproximatePaintClip and describeSemanticsClip case either since we wouldn't be able to differentiate between an item that is invisible but does not want to be clipped vs an item that is invisible but should be clipped, maybe if we use a magic number but not sure how stable that would be (would a normal sliver ever reach this magic number).
There was a problem hiding this comment.
agree, if it has other implication then we better off implement a new api for this
| RenderSliverToBoxAdapter({super.child}); | ||
|
|
||
| @override | ||
| Rect get semanticBounds { |
There was a problem hiding this comment.
Interesting I originally thought this was needed to avoid the node from being dropped by shouldDrop or sendSemanticsUpdate invisible checks, but that doesn't seem to be the case when I removed it now. The rect in the semantics tree is also accurate without this so I will remove it.
142f8b3 to
c0fa504
Compare
5fa67ff to
4ae2692
Compare
chunhtai
left a comment
There was a problem hiding this comment.
I think the API surface looks good to me, but i left some question about the geometry
| @@ -122,6 +90,18 @@ class SemanticScrollable extends SemanticRole { | |||
| // Scrolling is controlled by setting overflow-y/overflow-x to 'scroll`. The | |||
| // default overflow = "visible" needs to be unset. | |||
| semanticsObject.element.style.overflow = ''; | |||
There was a problem hiding this comment.
This is tangential to this pr but should the semantics scrollable be a role instead of a behavior? right now it seems hard to annotate other role like list with a scrollable. cc @yjbanov
There was a problem hiding this comment.
Could you elaborate? I imagine using a behavior would make it easier to assign a role. Scrollability seems to be an orthogonal concept to list. You can have scrollable and non-scrollable lists, and you can have scrollables that are not lists (although most often they are a list). For example, you can have scrollable tables, lists, and menus. But you can also have non-scrollable variants of those. To express that you can set the role independently of using the scrollable behavior.
There was a problem hiding this comment.
Also, right now SemanticScrollable is a SemanticRole. I'd argue that we should change that.
| (RenderSliver sliver) => sliver.geometry!.visible || sliver.geometry!.cacheExtent > 0.0, | ||
| (RenderSliver sliver) => | ||
| sliver.geometry!.visible || | ||
| sliver.geometry!.cacheExtent > 0.0 || |
There was a problem hiding this comment.
agree, if it has other implication then we better off implement a new api for this
| } | ||
|
|
||
| /// Ensures its sliver child is included in the semantics tree. | ||
| class RenderSliverEnsureSemantics extends RenderProxySliver { |
There was a problem hiding this comment.
consider moving this to widget layer and keep this private
There was a problem hiding this comment.
It has been a while since i looked at the sliver code. Can we assume this child will some how gets laid out even if it is offscreen? my understand is that to get the correct gemoetry (to calculate paint extend) this has to be laid out by the parent. If so, can you point me to the code that this happens?
There was a problem hiding this comment.
We can't necessarily assume that. For the case of my examples where a single item is wrapped with a SliverToBoxAdapter, this works because SliverToBoxAdapter always lays its child out.
The code is mainly here
flutter/packages/flutter/lib/src/rendering/viewport.dart
Lines 572 to 670 in dafbc0e
From my understanding RenderViewportBase will call layout on all its children, but its up to the child to actually lay itself out. Lazily created slivers like RenderSliverList will not layout all their children, so this approach actually doesn't work for that case, even if wrapped with an SliverEnsureSemantics because the geometry will have an empty rect. I wonder if we can leverage that RenderSliverList always lays out its first child in order to calculate the total scroll extent, and use that as the semantic bounds while the lazy sliver is off screen.
There was a problem hiding this comment.
Oops how I explained it above is how it actually works by default right now. Since the RenderSliverList always builds its first child those are its current bounds, if you wrap a SliverList with SliverEnsureSemantics, this ensures that RenderViewportBase still visits its in visitChildrenForSemantics and ensures that it doesn't get clipped out in the describeSemanticsClip or describeApproximateClipRect.
So for example
import 'package:flutter/material.dart';
import 'package:flutter/rendering.dart';
void main() {
runApp(const MyApp());
SemanticsBinding.instance.ensureSemantics();
}
class MyApp extends StatelessWidget {
const MyApp({super.key});
@override
Widget build(BuildContext context) {
return MaterialApp(
title: 'Flutter Demo',
theme: ThemeData(
colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
),
home: const MyHomePage(title: 'Flutter Demo Home Page'),
);
}
}
class MyHomePage extends StatefulWidget {
const MyHomePage({super.key, required this.title});
final String title;
@override
State<MyHomePage> createState() => _MyHomePageState();
}
class _MyHomePageState extends State<MyHomePage> {
@override
Widget build(BuildContext context) {
final ThemeData theme = Theme.of(context);
return Scaffold(
appBar: AppBar(
backgroundColor: theme.colorScheme.inversePrimary,
title: Text(widget.title),
),
body: Center(
child: CustomScrollView(
semanticChildCount: 52,
slivers: <Widget>[
SliverEnsureSemantics(
sliver: SliverToBoxAdapter(
child: IndexedSemantics(
index: 0,
child: Card(
child: Padding(
padding: const EdgeInsets.all(8.0),
child: Column(
crossAxisAlignment: CrossAxisAlignment.start,
children: <Widget>[
Semantics(
header: true,
headingLevel: 3,
child: Text(
'Steps to reproduce',
style: theme.textTheme.headlineSmall,
),
),
Text('Issue description'),
Semantics(
header: true,
headingLevel: 3,
child: Text(
'Expected Results',
style: theme.textTheme.headlineSmall,
),
),
Semantics(
header: true,
headingLevel: 3,
child: Text(
'Actual Results',
style: theme.textTheme.headlineSmall,
),
),
Semantics(
header: true,
headingLevel: 3,
child: Text(
'Code Sample',
style: theme.textTheme.headlineSmall,
),
),
Semantics(
header: true,
headingLevel: 3,
child: Text(
'Screenshots',
style: theme.textTheme.headlineSmall,
),
),
Semantics(
header: true,
headingLevel: 3,
child: Text(
'Logs',
style: theme.textTheme.headlineSmall,
),
),
],
),
),
),
),
),
),
SliverList(
delegate: SliverChildBuilderDelegate(
(BuildContext context, int index) {
return Card(
child: Padding(
padding: const EdgeInsets.all(8.0),
child: Text('Item $index'),
),
);
},
childCount: 50,
semanticIndexOffset: 1,
),
),
SliverEnsureSemantics(
sliver: SliverToBoxAdapter(
child: IndexedSemantics(
index: 52,
child: Card(
child: Padding(
padding: const EdgeInsets.all(8.0),
child: Semantics(header: true, child: Text('Footer 1')),
),
),
),
),
),
SliverEnsureSemantics(
sliver: SliverToBoxAdapter(
child: IndexedSemantics(
index: 53,
child: Card(
child: Padding(
padding: const EdgeInsets.all(8.0),
child: Semantics(header: true, child: Text('Footer 2')),
),
),
),
),
),
SliverEnsureSemantics(
sliver: SliverToBoxAdapter(
child: IndexedSemantics(
index: 54,
child: Semantics(link: true, child: Text('Link #1')),
),
),
),
SliverEnsureSemantics(
sliver: SliverToBoxAdapter(
child: IndexedSemantics(
index: 55,
child: OverflowBar(
children: <Widget>[
TextButton(onPressed: () {}, child: Text('Button 1')),
TextButton(onPressed: () {}, child: Text('Button 2')),
],
),
),
),
),
SliverEnsureSemantics(
sliver: SliverToBoxAdapter(
child: IndexedSemantics(
index: 56,
child: Semantics(link: true, child: Text('Link #2')),
),
),
),
SliverEnsureSemantics(
sliver: SliverList(
delegate: SliverChildBuilderDelegate(
(BuildContext context, int index) {
return Semantics(
role: SemanticsRole.listItem,
child: Card(
child: Padding(
padding: const EdgeInsets.all(8.0),
child: Text('Second List Item $index'),
),
),
);
},
childCount: 50,
semanticIndexOffset: 56,
),
),
),
SliverEnsureSemantics(
sliver: SliverToBoxAdapter(
child: IndexedSemantics(
index: 107,
child: Semantics(link: true, child: Text('Link #3')),
),
),
),
],
),
),
);
}
}this works because the second list still lays out its first child, so the browser has this information and can jump to it. (With the caveat that this is not possible yet without hardcoding the role in RenderSliverList to SemanticsRole.list, but I am working on this functionality in a follow up PR).
There was a problem hiding this comment.
Disregard my last comment, I had some test code that made this work while trying this, currently RenderSliverList reports empty bounds even though it lays out its first child. I still think we can leverage the first child to use as an initial anchor for semantics. What do you think?
There was a problem hiding this comment.
Talked offline, concluded it is reasonable to use the first child bounds as the semantic bounds while the sliver is off screen.
| /// to ensure a sliver is included in the semantics tree regardless of its geometry. | ||
| /// | ||
| /// [RenderSliverEnsureSemantics] overrides this value to `true` to ensure | ||
| /// its sliver child is included in the semantics tree. |
There was a problem hiding this comment.
somewhere in the doc should explain the following
- the difference between this and off screen rendersliver that was laid out due to cache extent
- A typical example why someone may want to override this getter to return true.
- The implication when return true. ie. element may be built and probably(?) laid out
| /// Whether this sliver should be included in the semantics tree. | ||
| /// | ||
| /// This value is used by [RenderViewportBase.visitChildrenForSemantics] | ||
| /// to ensure a sliver is included in the semantics tree regardless of its geometry. |
There was a problem hiding this comment.
regardless of its geometry
does this mean the geometry may not be accurate?
I was assuming the geometry must be accurate, at least the scroll extent. Otherwise, how would the engine calculate the amount it needs to scroll to review this item?
There was a problem hiding this comment.
Yeah the geometry may not be accurate for example for lazy slivers, but for lazy slivers like SliverList, we always lay out at least the first child so we can estimate the total scroll extent. The first childs rect should be enough information to move the list into view.
edit: Disregard this comment, I was running some test code when I tried this that made this work. Essentially EnsureSliverSemantics will ensure semantics at the RenderViewportbase level, but it is still up to a sliver to provide valid semanticBounds. In my testing RenderSliverToBoxAdapter does this since it always lays out its child, but RenderSliverList does not so even if wrapped with EnsureSliverSemantics it will not be included because the underlying sliver does not provide valid semantic bounds (it probably should since it always lays out its first child).
There was a problem hiding this comment.
For lazy slivers like SliverList, this PR now leverages the fact that the first child is always laid out as an anchor for semantics to bring the list into view.
This reverts commit 3fa9b38.
This reverts commit 3fa9b38.
<!-- start_original_pr_link --> Reverts: #165589 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: Renzo-Olivares <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: breaking internal tests <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: Renzo-Olivares <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {Piinks} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: Currently when using a `CustomScrollView`, screen readers cannot list or move focus to elements that are outside the current Viewport and cache extent because we do not create semantic nodes for these elements. This change introduces `SliverEnsureSemantics` which ensures its sliver child is included in the semantics tree, whether or not it is currently visible on the screen or within the cache extent. This way screen readers are aware the elements are there and can navigate to them / create accessibility traversal menus with this information. * Under the hood a new flag has been added to `RenderSliver` called `ensureSemantics`. `RenderViewportBase` uses this in its `visitChildrenForSemantics` to ensure a sliver is visited when creating the semantics tree. Previously a sliver was not visited if it was not visible or within the cache extent. `RenderViewportBase` also uses this in `describeSemanticsClip` and `describeApproximatePaintClip` to ensure a sliver child that wants to "ensure semantics" is not clipped out if it is not currently visible in the viewport or outside the cache extent. * `RenderSliverMultiBoxAdaptor.semanticBounds` now leverages its first child as an anchor for assistive technologies to be able to reach it if the Sliver is a child of `SliverEnsureSemantics`. If not it will still be dropped from the semantics tree. * `RenderProxySliver` now considers child overrides of `semanticBounds`. On the engine side we move from using a joystick method to scroll with `SemanticsAction.scrollUp` and `SemanticsAction.scrollDown` to using `SemanticsAction.scrollToOffset` completely letting the browser drive the scrolling with its current dom scroll position "scrollTop" or "scrollLeft". This is possible by calculating the total quantity of content under the scrollable and sizing the scroll element based on that. <details open><summary>Code sample</summary> ```dart // Copyright 2014 The Flutter Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. import 'package:flutter/material.dart'; import 'package:flutter/rendering.dart'; /// Flutter code sample for [SliverEnsureSemantics]. void main() => runApp(const SliverEnsureSemanticsExampleApp()); class SliverEnsureSemanticsExampleApp extends StatelessWidget { const SliverEnsureSemanticsExampleApp({super.key}); @OverRide Widget build(BuildContext context) { return const MaterialApp(home: SliverEnsureSemanticsExample()); } } class SliverEnsureSemanticsExample extends StatefulWidget { const SliverEnsureSemanticsExample({super.key}); @OverRide State<SliverEnsureSemanticsExample> createState() => _SliverEnsureSemanticsExampleState(); } class _SliverEnsureSemanticsExampleState extends State<SliverEnsureSemanticsExample> { @OverRide Widget build(BuildContext context) { final ThemeData theme = Theme.of(context); return Scaffold( appBar: AppBar( backgroundColor: theme.colorScheme.inversePrimary, title: const Text('SliverEnsureSemantics Demo'), ), body: Center( child: CustomScrollView( semanticChildCount: 106, slivers: <Widget>[ SliverEnsureSemantics( sliver: SliverToBoxAdapter( child: IndexedSemantics( index: 0, child: Card( child: Padding( padding: const EdgeInsets.all(8.0), child: Column( crossAxisAlignment: CrossAxisAlignment.start, children: <Widget>[ Semantics( header: true, headingLevel: 3, child: Text( 'Steps to reproduce', style: theme.textTheme.headlineSmall, ), ), const Text('Issue description'), Semantics( header: true, headingLevel: 3, child: Text( 'Expected Results', style: theme.textTheme.headlineSmall, ), ), Semantics( header: true, headingLevel: 3, child: Text( 'Actual Results', style: theme.textTheme.headlineSmall, ), ), Semantics( header: true, headingLevel: 3, child: Text( 'Code Sample', style: theme.textTheme.headlineSmall, ), ), Semantics( header: true, headingLevel: 3, child: Text( 'Screenshots', style: theme.textTheme.headlineSmall, ), ), Semantics( header: true, headingLevel: 3, child: Text( 'Logs', style: theme.textTheme.headlineSmall, ), ), ], ), ), ), ), ), ), SliverFixedExtentList( itemExtent: 44.0, delegate: SliverChildBuilderDelegate( (BuildContext context, int index) { return Card( child: Padding( padding: const EdgeInsets.all(8.0), child: Text('Item $index'), ), ); }, childCount: 50, semanticIndexOffset: 1, ), ), SliverEnsureSemantics( sliver: SliverToBoxAdapter( child: IndexedSemantics( index: 51, child: Card( child: Padding( padding: const EdgeInsets.all(8.0), child: Semantics( header: true, child: const Text('Footer 1'), ), ), ), ), ), ), SliverEnsureSemantics( sliver: SliverToBoxAdapter( child: IndexedSemantics( index: 52, child: Card( child: Padding( padding: const EdgeInsets.all(8.0), child: Semantics( header: true, child: const Text('Footer 2'), ), ), ), ), ), ), SliverEnsureSemantics( sliver: SliverToBoxAdapter( child: IndexedSemantics( index: 53, child: Semantics(link: true, child: const Text('Link #1')), ), ), ), SliverEnsureSemantics( sliver: SliverToBoxAdapter( child: IndexedSemantics( index: 54, child: OverflowBar( children: <Widget>[ TextButton( onPressed: () {}, child: const Text('Button 1'), ), TextButton( onPressed: () {}, child: const Text('Button 2'), ), ], ), ), ), ), SliverEnsureSemantics( sliver: SliverToBoxAdapter( child: IndexedSemantics( index: 55, child: Semantics(link: true, child: const Text('Link #2')), ), ), ), SliverEnsureSemantics( sliver: SliverSemanticsList( sliver: SliverFixedExtentList( itemExtent: 44.0, delegate: SliverChildBuilderDelegate( (BuildContext context, int index) { return Semantics( role: SemanticsRole.listItem, child: Card( child: Padding( padding: const EdgeInsets.all(8.0), child: Text('Second List Item $index'), ), ), ); }, childCount: 50, semanticIndexOffset: 56, ), ), ), ), SliverEnsureSemantics( sliver: SliverToBoxAdapter( child: IndexedSemantics( index: 107, child: Semantics(link: true, child: const Text('Link #3')), ), ), ), ], ), ), ); } } // A sliver that assigns the role of SemanticsRole.list to its sliver child. class SliverSemanticsList extends SingleChildRenderObjectWidget { const SliverSemanticsList({super.key, required Widget sliver}) : super(child: sliver); @OverRide RenderSliverSemanticsList createRenderObject(BuildContext context) => RenderSliverSemanticsList(); } class RenderSliverSemanticsList extends RenderProxySliver { @OverRide void describeSemanticsConfiguration(SemanticsConfiguration config) { super.describeSemanticsConfiguration(config); config.role = SemanticsRole.list; } } ``` </details> Fixes: #160217 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <[email protected]>
)" This reverts commit 2fc716d.
)" This reverts commit 2fc716d.
Currently when using a
CustomScrollView, screen readers cannot list or move focus to elements that are outside the current Viewport and cache extent because we do not create semantic nodes for these elements.This change introduces
SliverEnsureSemanticswhich ensures its sliver child is included in the semantics tree, whether or not it is currently visible on the screen or within the cache extent. This way screen readers are aware the elements are there and can navigate to them / create accessibility traversal menus with this information.RenderSlivercalledensureSemantics.RenderViewportBaseuses this in itsvisitChildrenForSemanticsto ensure a sliver is visited when creating the semantics tree. Previously a sliver was not visited if it was not visible or within the cache extent.RenderViewportBasealso uses this indescribeSemanticsClipanddescribeApproximatePaintClipto ensure a sliver child that wants to "ensure semantics" is not clipped out if it is not currently visible in the viewport or outside the cache extent.RenderSliverMultiBoxAdaptor.semanticBoundsnow leverages its first child as an anchor for assistive technologies to be able to reach it if the Sliver is a child ofSliverEnsureSemantics. If not it will still be dropped from the semantics tree.RenderProxySlivernow considers child overrides ofsemanticBounds.On the engine side we move from using a joystick method to scroll with
SemanticsAction.scrollUpandSemanticsAction.scrollDownto usingSemanticsAction.scrollToOffsetcompletely letting the browser drive the scrolling with its current dom scroll position "scrollTop" or "scrollLeft". This is possible by calculating the total quantity of content under the scrollable and sizing the scroll element based on that.Code sample
Fixes: #160217
Pre-launch Checklist
///).