Skip to content

SingleChildScrollView does not clip semantics child#114194

Merged
auto-submit[bot] merged 4 commits intoflutter:masterfrom
chunhtai:issues/113615
Oct 28, 2022
Merged

SingleChildScrollView does not clip semantics child#114194
auto-submit[bot] merged 4 commits intoflutter:masterfrom
chunhtai:issues/113615

Conversation

@chunhtai
Copy link
Contributor

@chunhtai chunhtai commented Oct 27, 2022 •
edited
Loading

Fixes #113615

I can't find a reason to clip it since it was laid out just to be thrown away during the semantics gathering stage.
The original issue is because some children are too far away from cache extent and gets throw away. the voiceover then think there is no child.

The alternative is to expose cache extent in single child scroll view, but it doesn't make sense as an API because everything will be laid out regardlessly.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Oct 27, 2022
@chunhtai chunhtai requested review from Piinks and goderbauer October 27, 2022 23:12
@goderbauer
Copy link
Member

goderbauer commented Oct 28, 2022

Looks like some tests are failing on this?

goderbauer
goderbauer reviewed Oct 28, 2022
View reviewed changes
packages/flutter/lib/src/widgets/single_child_scroll_view.dart Outdated
Copy link
Member

@goderbauer goderbauer Oct 28, 2022

Choose a reason for hiding this comment

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

Isn't this making the semantics clip larger than necessary? AFAK, "remainingOffset" is the distance from the top of the viewport to the end of the scrollable content, so the semantics clip here would be one viewport height to tall, no?

Not that it matters in practice, but may cause confusion in the future...

Copy link
Member

@goderbauer goderbauer Oct 28, 2022

Choose a reason for hiding this comment

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

Could we actually add a test for this? The height of the semanticsClip rect should be exactly the height of the scrollable content, right?

Copy link
Member

@goderbauer goderbauer Oct 28, 2022

Choose a reason for hiding this comment

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

Ah, wait: _maxScrollExtent is already accounting for the viewport width. Still, I think adding a test like I described above would be great.

goderbauer
goderbauer approved these changes Oct 28, 2022
View reviewed changes
Copy link
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

packages/flutter/lib/src/widgets/single_child_scroll_view.dart Outdated
Copy link
Member

@goderbauer goderbauer Oct 28, 2022

Choose a reason for hiding this comment

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

Ah, wait: _maxScrollExtent is already accounting for the viewport width. Still, I think adding a test like I described above would be great.

goderbauer
goderbauer approved these changes Oct 28, 2022
View reviewed changes
Copy link
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

THanks for the test!

packages/flutter/test/widgets/single_child_scroll_view_test.dart Outdated
Copy link
Member

@goderbauer goderbauer Oct 28, 2022

Choose a reason for hiding this comment

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

nit: here and below are some extra whitespaces after the "="

Copy link
Contributor Author

@chunhtai chunhtai Oct 28, 2022

Choose a reason for hiding this comment

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

Although we don't use fmt in flutter, I think should have lint or autoformat on indent at least.

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 28, 2022
Piinks
Piinks approved these changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Flutter_LGTM

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 28, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Oct 28, 2022

auto label is removed for flutter/flutter, pr: 114194, due to - The status or check suite Linux web_tests_7_last has failed. Please fix the issues identified (or deflake) before re-applying this label.

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 28, 2022
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 28, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Oct 28, 2022

auto label is removed for flutter/flutter, pr: 114194, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@chunhtai
Copy link
Contributor Author

chunhtai commented Oct 28, 2022

besides the expected scuba the other test failure doesn't seem related

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 28, 2022
@auto-submit auto-submit bot merged commit fa174b2 into flutter:master Oct 28, 2022
This was referenced Oct 29, 2022
Merged
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 29, 2022
@engine-flutter-autoroll
fa174b2 SingleChildScrollView does not clip semantics child (flutter/…
ad5d44a 
…flutter#114194)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Oct 29, 2022
@engine-flutter-autoroll
fa174b2 SingleChildScrollView does not clip semantics child (flutter/…
3eb9292 
…flutter#114194)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 29, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Oct 29, 2022
@engine-flutter-autoroll
fa174b2 SingleChildScrollView does not clip semantics child (flutter/…
41e9af9 
…flutter#114194)
CaseyHillers added a commit that referenced this pull request Nov 1, 2022
@CaseyHillers
Revert "SingleChildScrollView does not clip semantics child (#114194)"
b074ac1 
This reverts commit fa174b2.
@CaseyHillers CaseyHillers mentioned this pull request Nov 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goderbauer goderbauer goderbauer approved these changes

@Piinks Piinks Piinks approved these changes

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[iOS] [a11y] Unable to fully navigate page using talkback when using nested scroll views

3 participants

@chunhtai @goderbauer @Piinks