Skip to content

Implicit scrolling for pageview#45598

Merged
dnfield merged 5 commits intoflutter:masterfrom
dnfield:page_view_a11y
Nov 26, 2019
Merged

Implicit scrolling for pageview#45598
dnfield merged 5 commits intoflutter:masterfrom
dnfield:page_view_a11y

Conversation

@dnfield
Copy link
Copy Markdown
Contributor

@dnfield dnfield commented Nov 26, 2019

Description

Previous attempt here: #44645

This is an improvement on that (I hope) in terms of documentation and implmentation. No longer uses a layout builder.

If we can get the iOS bridge to respect the allowImplicitScrolling flag, we could also allow for the page view to have a non-zero cache extent - but for now, on iOS, having a cache extent allows implicit a11y scrolling.

Basic use case here is to use a PageView in non-full-screen applications such as a carousel.

Related Issues

#44645
fixes #44484

Tests

I added the following tests:

Test asserting the semantics tree looking right when the new flag is set.

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.

@dnfield dnfield requested a review from goderbauer November 26, 2019 06:26
@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Nov 26, 2019
@dnfield dnfield changed the title feedback/new cache extent calc Implicit scrolling for pageview Nov 26, 2019
Copy link
Copy Markdown
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

/// scrolling.
///
/// With this flag set to false, when accessibility focus reaches the end of
/// the current page and a user performs another implicit scroll action,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the "implicit scroll action" is moving a11y focus to the next element. Maybe we should say:

With this flag set to false, when accessibility focus reaches the end of current page and the user attempts to move it to the next element, the focus will traverse to the next widget outside of the page view.

With this flag set to true, when accessibility focus reaches the end of the current page and user attempts to move it to the next element, focus will traverse to the next page in the page view.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that better, thanks - done.

viewportBuilder: (BuildContext context, ViewportOffset position) {
return Viewport(
cacheExtent: 0.0,
cacheExtent: widget.allowImplicitScrolling ? 1.0 : 0.0,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe leave a TODO here to make the cacheExtend/cacheExtendStyle configurable on the pageView once we have implicitScrolling set up correctly on iOS.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@dnfield
Copy link
Copy Markdown
Contributor Author

dnfield commented Nov 26, 2019

The commit behind this one is about to be reverted while I investigate perf regression that started with it. Will have to put this on hold.

@dnfield dnfield merged commit 2d3d220 into flutter:master Nov 26, 2019
@dnfield dnfield deleted the page_view_a11y branch November 26, 2019 22:12
@dnfield
Copy link
Copy Markdown
Contributor Author

dnfield commented Nov 26, 2019

Regression was unrelated, code is safe for now

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PageView should allow for implicit a11y scrolling

4 participants