Update a11y for SliverAppBar #144437
Update a11y for SliverAppBar #144437
Conversation
github-actions
bot
added
framework
test cache extent test
hannah-hyj
force-pushed
the
sliver-app-bar
branch
from
75b25ac to
0ec2e3b
Compare
hannah-hyj
requested review from
HansMuller and
Piinks
and removed request for
Piinks
| } | ||
| final double maxExtent = this.maxExtent; | ||
| final double paintExtent = maxExtent - constraints.scrollOffset; | ||
| final double cacheExtent = calculateCacheOffset( |
There was a problem hiding this comment.
I can't claim to understand the API doc for calculateCacheOffset (which is often used to calculate cache extents?) however this statement is troubling: This method is not useful if there is not a 1:1 relationship between consumed scroll offset and consumed cache extent. Presumably that's true for pinned persistent headers, so unconditionally specifying the cache extent here might not be recommended.
@Piinks - WDYT?
There was a problem hiding this comment.
The cacheExtent here is of RenderSliverScrollingPersistentHeader, which is a non-pinned and non-floating variant of the sliver app bar. So I think the consumed scroll offset and consumed cache extent is a 1:1 relationship.
The pinned headers you mentioned are RenderSliverPinnedPersistentHeader, it doesn't have an a11y issue because it won't be scrolled off the screen and dropped from the semantics tree.
There was a problem hiding this comment.
@Hangyujin is right, this is the scrolling render object, not the pinned one.
I think this is the second or third sliver a11y issue that was caused by the cacheExtent not being set like this. It may be worth checking the rest of the sliver library for the same issue in other slivers.
| final bool isScrolledUnder = overlapsContent || forceElevated || (pinned && shrinkOffset > maxExtent - minExtent); | ||
| final bool isPinnedWithOpacityFade = pinned && floating && bottom != null && extraToolbarHeight == 0.0; | ||
| final double toolbarOpacity = !pinned || isPinnedWithOpacityFade | ||
| final bool isAccessibilityOn = MediaQuery.of(context).accessibleNavigation; |
There was a problem hiding this comment.
Should the value of MediaQuery.of(context).accessibleNavigation be a passed parameter so it can be checked in shouldRebuild for when it changes?
There was a problem hiding this comment.
Use of MediaQuery.of or MediaQuery.accessibleNavigationOf will cause it to rebuild, is it still necessary to add a parameter to shouldRebuild?
There was a problem hiding this comment.
Can you add a test that switches accessibleNavigation off and on to verify it updates? Usually I would use didChangeDependencies, but _SliverAppBarDelegate is not a stateful widget.
There was a problem hiding this comment.
Test added. Looks like I do need to update it to shouldRebuild(), thank you!
| } | ||
| final double maxExtent = this.maxExtent; | ||
| final double paintExtent = maxExtent - constraints.scrollOffset; | ||
| final double cacheExtent = calculateCacheOffset( |
There was a problem hiding this comment.
@Hangyujin is right, this is the scrolling render object, not the pinned one.
I think this is the second or third sliver a11y issue that was caused by the cacheExtent not being set like this. It may be worth checking the rest of the sliver library for the same issue in other slivers.
|
@Piinks ping for review :) |
3 participants
fixes: iOS VoiceOver isn't able to go back to SliverAppBar from SliverSafeArea after appbar leaves screen #143437
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.