Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix CupertinoTextSelectionToolbar clipping #138195

Merged
merged 10 commits into from Feb 2, 2024
Merged

Fix CupertinoTextSelectionToolbar clipping #138195

merged 10 commits into from Feb 2, 2024

Conversation

luccasclezar
Copy link
Contributor

@luccasclezar luccasclezar commented Nov 9, 2023 •
edited

The CupertinoTextSelectionToolbar sets the maxWidth of the whole toolbar to the width of the first page. This ends up clipping other pages from the toolbar. This PR just removes this limitation.

It was easy enough that I thought there was a catch, but I ran the tests locally and they all passed.

Before After
Simulator Screenshot - iPhone Xʀ - 2023-11-09 at 19 45 29 Simulator Screenshot - iPhone Xʀ - 2023-11-09 at 19 44 30

#138177

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
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels Nov 9, 2023
@LongCatIsLooong
Copy link
Contributor

The change makes sense to me. I tried building the same menu in UIKit and it did't limit the max width of the menu to the width of the first page. Could you add a test for this change?

@luccasclezar
Copy link
Contributor Author

@LongCatIsLooong Whew, glad this was really this easy to fix 😅 Will add a test as soon as possible.

@LongCatIsLooong
Copy link
Contributor

Hello from the triage meeting, are you planning to continue working on this PR?

@luccasclezar
Copy link
Contributor Author

@LongCatIsLooong Sorry, I couldn't find enough time yet to implement the tests, I will try to do it today.

@luccasclezar
Copy link
Contributor Author

@LongCatIsLooong Done! I reverted my previous commit locally to make sure the test failed without the fix, so everything should be in order.

LongCatIsLooong
LongCatIsLooong approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

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

The change LGTM. Thank you for contributing! The ci fails are not related, I've synced the PR branch via the github UI hopefully that will do it.

@justinmc justinmc self-requested a review January 4, 2024 23:37
justinmc
justinmc reviewed Jan 5, 2024
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

When I wrote this I deliberately kept it to the first page's width because that's what I saw native iOS doing. It looks like that's no longer the case though, so thank you for changing it!

packages/flutter/test/cupertino/text_selection_toolbar_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/cupertino/text_selection_toolbar_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/cupertino/text_selection_toolbar_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/cupertino/text_selection_toolbar_test.dart Outdated Show resolved Hide resolved
@justinmc
Copy link
Contributor

@luccasclezar Also FYI you have a merge conflict.

@justinmc
Copy link
Contributor

@luccasclezar Are you still planning to come back to this PR?

@luccasclezar
Copy link
Contributor Author

@justinmc Yep, I'm just very busy currently 😅 I will try to improve what you suggested tomorrow, and if I get any complications I will finish on the weekend.

@luccasclezar
Copy link
Contributor Author

@justinmc Okay, I did everything you suggested and resolved the conflict. Everything should be working now 😁

justinmc
justinmc approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@justinmc justinmc 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 fixes!

@justinmc
Copy link
Contributor

justinmc commented Feb 1, 2024

@luccasclezar Can you update the branch with master? I think that will fix the broken "ci.yaml validation" check...

@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 1, 2024
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Feb 1, 2024

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

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 1, 2024
@LongCatIsLooong
Copy link
Contributor 

   info • Missing type annotation • packages/flutter/test/cupertino/text_selection_toolbar_test.dart:277:5 • always_specify_types
   info • Missing type annotation • packages/flutter/test/cupertino/text_selection_toolbar_test.dart:278:5 • always_specify_types
   info • Missing type annotation • packages/flutter/test/cupertino/text_selection_toolbar_test.dart:279:5 • always_specify_types
   info • Missing type annotation • packages/flutter/test/cupertino/text_selection_toolbar_test.dart:280:5 • always_specify_types

@luccasclezar
Copy link
Contributor Author

@LongCatIsLooong I can fix these tomorrow

@luccasclezar
Copy link
Contributor Author

@LongCatIsLooong Done 👍

@justinmc
Copy link
Contributor

justinmc commented Feb 2, 2024

There are now some analyzer errors here.

@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 2, 2024
@auto-submit auto-submit bot merged commit fc3f4ed into flutter:master Feb 2, 2024
71 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 3, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 3, 2024
@engine-flutter-autoroll
fc3f4ed88 Fix CupertinoTextSelectionToolbar clipping (flutter/flutter…
ded8ea6 
…#138195)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 3, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 3, 2024
@engine-flutter-autoroll
fc3f4ed88 Fix CupertinoTextSelectionToolbar clipping (flutter/flutter…
f49f4cf 
…#138195)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 4, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 4, 2024
@engine-flutter-autoroll
fc3f4ed88 Fix CupertinoTextSelectionToolbar clipping (flutter/flutter…
13a31c4 
…#138195)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 4, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 4, 2024
@engine-flutter-autoroll
fc3f4ed88 Fix CupertinoTextSelectionToolbar clipping (flutter/flutter…
63140c7 
…#138195)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 5, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 5, 2024
@engine-flutter-autoroll
fc3f4ed88 Fix CupertinoTextSelectionToolbar clipping (flutter/flutter…
f42de7f 
…#138195)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 5, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 5, 2024
@engine-flutter-autoroll
fc3f4ed88 Fix CupertinoTextSelectionToolbar clipping (flutter/flutter…
f1e5dec 
…#138195)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 5, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 5, 2024
@engine-flutter-autoroll
fc3f4ed88 Fix CupertinoTextSelectionToolbar clipping (flutter/flutter…
a5975c0 
…#138195)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 5, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 5, 2024
@engine-flutter-autoroll
fc3f4ed88 Fix CupertinoTextSelectionToolbar clipping (flutter/flutter…
af2ad77 
…#138195)
tarrinneal pushed a commit to flutter/packages that referenced this pull request Feb 5, 2024
@engine-flutter-autoroll
Manual roll Flutter from e02e2079bea7 to 0b5cd5073a3b (46 revisions) (#…
d63629d 
…6053)

Manual roll Flutter from e02e2079bea7 to 0b5cd5073a3b (46 revisions)

Manual roll requested by tarrinneal@google.com

flutter/flutter@e02e207...0b5cd50

2024-02-05 124896814+BiskupMaik@users.noreply.github.com fix AppBar docs
for backgroundColor & foregroundColor (flutter/flutter#142430)
2024-02-04 98614782+auto-submit[bot]@users.noreply.github.com Reverts
"Update gradle lockfiles template" (flutter/flutter#142889)
2024-02-04 barpac02@gmail.com Update gradle lockfiles template
(flutter/flutter#140115)
2024-02-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from
20742e37e54e to f34c658b9600 (1 revision) (flutter/flutter#142876)
2024-02-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from
23763db72272 to 20742e37e54e (1 revision) (flutter/flutter#142850)
2024-02-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from
fee02145da8c to 23763db72272 (3 revisions) (flutter/flutter#142848)
2024-02-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from
9869d47a2736 to fee02145da8c (2 revisions) (flutter/flutter#142847)
2024-02-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from
78c63d3c2c68 to 9869d47a2736 (1 revision) (flutter/flutter#142842)
2024-02-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from
266d5d0b5588 to 78c63d3c2c68 (1 revision) (flutter/flutter#142836)
2024-02-02 49699333+dependabot[bot]@users.noreply.github.com Bump
github/codeql-action from 3.23.2 to 3.24.0 (flutter/flutter#142839)
2024-02-02 49699333+dependabot[bot]@users.noreply.github.com Bump
codecov/codecov-action from 3.1.6 to 4.0.1 (flutter/flutter#142838)
2024-02-02 jmccandless@google.com Update TextSelectionOverlay
(flutter/flutter#142463)
2024-02-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from
e29263212bfd to 266d5d0b5588 (5 revisions) (flutter/flutter#142832)
2024-02-02 luccas.clezar@gmail.com Fix CupertinoTextSelectionToolbar
clipping (flutter/flutter#138195)
2024-02-02 barpac02@gmail.com Reland "Add support for Gradle Kotlin DSL
(#140744)" (flutter/flutter#142752)
2024-02-02 jmccandless@google.com Support navigation during a Cupertino
back gesture (flutter/flutter#142248)
2024-02-02 chingjun@google.com Avoid depending on files from
build_system/targets other than from top level entrypoints in
flutter_tools. (flutter/flutter#142760)
2024-02-02 engine-flutter-autoroll@skia.org Roll Packages from
5b48c44 to d37fb0a (14 revisions) (flutter/flutter#142812)
2024-02-02 32242716+ricardoamador@users.noreply.github.com Add a link
the different possible Android virtual device configs
(flutter/flutter#142765)
2024-02-02 15619084+vashworth@users.noreply.github.com Allow all iOS
tests to use either iOS 16 or 17 (flutter/flutter#142714)
2024-02-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from
b35153d00b2e to e29263212bfd (2 revisions) (flutter/flutter#142799)
2024-02-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from
dd4c79a6c864 to b35153d00b2e (10 revisions) (flutter/flutter#142783)
2024-02-02 jacksongardner@google.com Wasm/JS Dual Compile with the
flutter tool (flutter/flutter#141396)
2024-02-02 hans.muller@gmail.com Reland: Added
ButtonStyle.foregroundBuilder and ButtonStyle.backgroundBuilder
(flutter/flutter#142762)
2024-02-01 32242716+ricardoamador@users.noreply.github.com Use proto
name for emulator version and show cipd package version
(flutter/flutter#142262)
2024-02-01 xilaizhang@google.com [github actions] ping actor of workflow
on cherry pick pr creation (flutter/flutter#142676)
2024-02-01 fluttergithubbot@gmail.com Marks Linux_android_emu android
views to be unflaky (flutter/flutter#142590)
2024-02-01 nathan.wilson1232@gmail.com Implement `switch` expressions in
`lib/src/material/` (flutter/flutter#142634)
2024-02-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from
9beb7e82e081 to dd4c79a6c864 (1 revision) (flutter/flutter#142749)
2024-02-01 pateltirth454@gmail.com Write Tests for API Example of
`form.0.dart` (flutter/flutter#142635)
2024-02-01 polinach@google.com Make leak_tracking bots sticked to the
left even if bot thinks they are non-flacky. (flutter/flutter#142744)
2024-02-01 15619084+vashworth@users.noreply.github.com Upload
DerivedData logs in CI (flutter/flutter#142643)
2024-02-01 magder@google.com Test codesigning xcframeworks in artifacts
(flutter/flutter#142666)
2024-02-01 davidmartos96@gmail.com Fix gen_defaults test randomness
(flutter/flutter#142743)
2024-02-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts
"Added ButtonStyle.foregroundBuilder and ButtonStyle.backgroundBuilder"
(flutter/flutter#142748)
2024-02-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from
39415c3eed42 to 9beb7e82e081 (5 revisions) (flutter/flutter#142745)
2024-02-01 magder@google.com Remove unused deprecated autoroll
mirror-remote flag (flutter/flutter#142738)
2024-02-01 polinach@google.com Fix leaks in tests.
(flutter/flutter#142677)
2024-02-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from
8c43332c6ffc to 39415c3eed42 (1 revision) (flutter/flutter#142740)
2024-02-01 magder@google.com Remove verbose-system-logs on iOS perf
tests (flutter/flutter#142739)
2024-02-01 magder@google.com Remove outdated arm64_armv7 check
(flutter/flutter#142737)
2024-02-01 62812903+sstasi95@users.noreply.github.com fix
CupertinoTabView's Android back button handling with PopScope
(flutter/flutter#141604)
2024-02-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from
68943afd62d1 to 8c43332c6ffc (8 revisions) (flutter/flutter#142726)
2024-02-01 christopherfujino@gmail.com Unpin test
(flutter/flutter#141427)
...
dumazy added a commit to dumazy/flutter that referenced this pull request Feb 7, 2024
@dumazy
Merge branch 'master' into fix/form_autovalidatemode_always
1d5f999 
* master: (45 commits)
  Reverts "Update gradle lockfiles template" (flutter#142889)
  Update gradle lockfiles template (flutter#140115)
  Roll Flutter Engine from 20742e37e54e to f34c658b9600 (1 revision) (flutter#142876)
  Roll Flutter Engine from 23763db72272 to 20742e37e54e (1 revision) (flutter#142850)
  Roll Flutter Engine from fee02145da8c to 23763db72272 (3 revisions) (flutter#142848)
  Roll Flutter Engine from 9869d47a2736 to fee02145da8c (2 revisions) (flutter#142847)
  Roll Flutter Engine from 78c63d3c2c68 to 9869d47a2736 (1 revision) (flutter#142842)
  Roll Flutter Engine from 266d5d0b5588 to 78c63d3c2c68 (1 revision) (flutter#142836)
  Bump github/codeql-action from 3.23.2 to 3.24.0 (flutter#142839)
  Bump codecov/codecov-action from 3.1.6 to 4.0.1 (flutter#142838)
  Update TextSelectionOverlay (flutter#142463)
  Roll Flutter Engine from e29263212bfd to 266d5d0b5588 (5 revisions) (flutter#142832)
  Fix CupertinoTextSelectionToolbar clipping (flutter#138195)
  Reland "Add support for Gradle Kotlin DSL (flutter#140744)" (flutter#142752)
  Support navigation during a Cupertino back gesture (flutter#142248)
  Avoid depending on files from build_system/targets other than from top level entrypoints in flutter_tools. (flutter#142760)
  Roll Packages from 5b48c44 to d37fb0a (14 revisions) (flutter#142812)
  Add a link the different possible Android virtual device configs (flutter#142765)
  Allow all iOS tests to use either iOS 16 or 17 (flutter#142714)
  Roll Flutter Engine from b35153d00b2e to e29263212bfd (2 revisions) (flutter#142799)
  ...
@HansMuller HansMuller mentioned this pull request Feb 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmc justinmc justinmc approved these changes

@LongCatIsLooong LongCatIsLooong LongCatIsLooong approved these changes

Assignees
No one assigned
Labels
a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App f: cupertino flutter/packages/flutter/cupertino repository 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.

None yet
3 participants
@luccasclezar @LongCatIsLooong @justinmc