Skip to content

add PageView cacheExtent - #42107

Closed
lukepighetti wants to merge 3 commits into
flutter:masterfrom
lukepighetti:feature/page-view-extents
Closed

lukepighetti wants to merge 3 commits into
flutter:masterfrom
lukepighetti:feature/page-view-extents

Conversation

@lukepighetti

@lukepighetti lukepighetti commented Oct 7, 2019 •

Copy link
Copy Markdown
Contributor

Description

Non-breaking change that allows PageView to specify cacheExtents in whole pages. Ie, setting cacheExtent to 1 will build the current page and the one before and the one after it.

Related Issues

#31191

Tests

I added the following tests:

'cacheExtent 0'
'cacheExtent 1'
'cacheExtent 2'

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.

@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Oct 7, 2019
@lukepighetti

Copy link
Copy Markdown
Contributor Author

Did this PR on master which appears to be a little dirty, but my PR & test coverage is clean.

@chunhtai chunhtai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The analyzer seems to be unhappy :(

Comment thread packages/flutter/lib/src/widgets/page_view.dart Outdated

@goderbauer goderbauer left a comment

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.

Before this can be merged we need to double-check that it doesn't break the accessibility of the PageView especially on iOS. On iOS the engine uses the cache extent as a signal for accessibility scrolling.

With voiceover turned on, the user should not be able to scroll to the next page just by advancing the accessibility focus by swing left or right.

Technically, we have the hasImplicitScrolling flag [1] to control that, but it looks like that's currently only wired up on Android and not on iOS [2]. We probably need to change that in order to allow PageView to have a cacheExtent.

I am marking this as "request changes" until it has either been verified that this does not break accessibility on iOS, or (if it does) until that has been fixed.

[1] https://master-api.flutter.dev/flutter/dart-ui/SemanticsFlag/hasImplicitScrolling-constant.html
[2] https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

Comment thread packages/flutter/lib/src/widgets/page_view.dart Outdated
Comment thread packages/flutter/lib/src/widgets/page_view.dart Outdated
Comment thread packages/flutter/lib/src/widgets/page_view.dart
@lukepighetti

Copy link
Copy Markdown
Contributor Author

@goderbauer does the flutter_test framework have any way to lock that behavior down?

@goderbauer

Copy link
Copy Markdown
Member

flutter_test is for unit tests. This would require an integration test that verifies Flutter's interaction with iOS VoiceOver.

@lukepighetti

Copy link
Copy Markdown
Contributor Author

@goderbauer what do you need from me at this point for this to land?

@goderbauer

Copy link
Copy Markdown
Member

I just verified: Setting a cacheExtend on a pageView breaks accessibility scrolling on iOS. Before we can land this, we need to change the iOS accessibility bridge in the engine [1] to ensure that it properly respects the SemanticsFlag.hasImplicitScrolling. If that flag is set, implicit scrolling should not be enabled even if a cache extent is specified.

[1] https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

@lukepighetti

lukepighetti commented Oct 9, 2019 •

Copy link
Copy Markdown
Contributor Author

@goderbauer Thanks for the follow up. I'll assume there's nothing I can do to assist until I am notified otherwise.

@goderbauer

Copy link
Copy Markdown
Member

If you want, you could make that change to the accessibility bridge. Other than that there's not much to do.

@lukepighetti

Copy link
Copy Markdown
Contributor Author

@goderbauer It's an area I'm not comfortable with, it would need some serious code review. Is this something that is coming down the pipeline or is it only to be implemented with this PR?

@goderbauer

Copy link
Copy Markdown
Member

@lukepighetti The work for that is currently not planned, but we need to do it at some point.

/cc @darrenaustin Maybe you're interested in this?

I am going to close this PR for now as we cannot merge it without the engine side changes and nobody is currently working on those.

@aytunch

aytunch commented Apr 19, 2020

Copy link
Copy Markdown

Why is this closed? We need this pr

@rafay-tariq

Copy link
Copy Markdown

'cacheExtent 0'
'cacheExtent 1'
'cacheExtent 2'

where to add these lines ? kindly tell me. i added inside the page builder as well as in page view but not works,
_pageController = PageController(
initialPage: 0,
keepPage: true,
viewportFraction: 1.0,

);

@nocduro

nocduro commented Jun 20, 2021 •

Copy link
Copy Markdown

I just stumbled upon this and it looks like there was a recent pull request that potentially added the required accessibility support on iOS that was blocking this?: flutter-team-archive/engine#26671

#31191

#45632

@chunhtai

Copy link
Copy Markdown
Contributor

@nocduro The accessibility scroll support in the PR is tangential to the problem @goderbauer mentioned

@LuChenyang3842

Copy link
Copy Markdown

The flutter team should somehow provide the preload ability for pageview..... can someone tell me when this feature will be planned.

@Bonsai11

Copy link
Copy Markdown
Contributor

Absolutely need this aswell, as I have a form for data entry spread across several pages. I know that there is a package out there to enable this, but I always prefer to have a "native" solution. With the current PageView, after you scrolled through all the none preloaded pages and then back to the beginning, isn't the situation you are now in exactly the same as if they were preloaded from the beginning? So this simply seems to be a shortcut to get somewhere where you can get already anyway, but also enabling more flexability in how you use the PageView.

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants