Skip to content

Hide the text selection toolbar on mobile when orientation changes #103512

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
Merged
fluttergithubbot merged 1 commit into from
May 25, 2022
Merged

Hide the text selection toolbar on mobile when orientation changes #103512

fluttergithubbot merged 1 commit into from
May 25, 2022

Conversation

@markusaksli-nc
Copy link
Contributor

@markusaksli-nc markusaksli-nc commented May 11, 2022 •
edited
Loading

Hide the text selection toolbar on Android and iOS when orientation changes by keeping track of the previous orientation in didChangeDependencies. Hide text selection handles as well on Android to match native project behavior.

Fixes #72004

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.

@flutter-dashboard flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels May 11, 2022
@markusaksli-nc markusaksli-nc mentioned this pull request May 11, 2022
Closed
@markusaksli-nc markusaksli-nc requested a review from justinmc May 11, 2022 21:19
@Renzo-Olivares
Copy link
Contributor

Renzo-Olivares commented May 11, 2022 •
edited
Loading

This behavior is also present on Android. Tested on Pixel 6 Pro running Android 12 May Security Update in native Messages App, Google Keep, and Gmail. Other than that LGTM.

@markusaksli-nc
Copy link
Contributor Author

markusaksli-nc commented May 12, 2022 •
edited
Loading

@Renzo-Olivares Okay I wasn't able to confirm this in all native apps but when creating a native Android project it is present so I think you are right. Also on Android, it seems the handles are hidden as well, not just the toolbar.

@markusaksli-nc markusaksli-nc changed the title Hide the text selection toolbar on iOS when orientation changes Hide the text selection toolbar on mobile when orientation changes May 12, 2022
@Renzo-Olivares
Copy link
Contributor

Renzo-Olivares commented May 12, 2022

@markusaksli-nc Yeah the selection handles are also hidden, forgot to mention that.

@markusaksli-nc markusaksli-nc force-pushed the toolbar-orientation branch 2 times, most recently from 64429a9 to 8c4f0c9 Compare May 12, 2022 15:40
Renzo-Olivares
Renzo-Olivares approved these changes May 12, 2022
View reviewed changes
Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a comment

Choose a reason for hiding this comment

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

LGTM

justinmc
justinmc approved these changes May 12, 2022
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 👍. Just some nits below. Thanks for fixing this old issue!

packages/flutter/lib/src/widgets/editable_text.dart Outdated
Copy link
Contributor

@justinmc justinmc May 12, 2022

Choose a reason for hiding this comment

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

Nit: There's no need to do any of the below if the platform isn't android or iOS. You could return early here on those other platforms, but I'm not sure how much benefit you would get from that in reality. Up to you @markusaksli-nc if you want to do it.

Copy link
Contributor Author

@markusaksli-nc markusaksli-nc May 12, 2022

Choose a reason for hiding this comment

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

I thought this would make the purpose and value of _lastOrientation a bit niche and opaque but now that I think about it, it wouldn't reflect physical rotation on other platforms anyway.

packages/flutter/lib/src/widgets/editable_text.dart Outdated
Copy link
Contributor

@justinmc justinmc May 12, 2022

Choose a reason for hiding this comment

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

Nit: Maybe a switch on the platform here if you think that would be better?

Copy link
Contributor Author

@markusaksli-nc markusaksli-nc May 12, 2022

Choose a reason for hiding this comment

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

I considered it but this is more compact and I think it's just as readable.

packages/flutter/test/widgets/editable_text_test.dart Outdated
Copy link
Contributor

@justinmc justinmc May 12, 2022

Choose a reason for hiding this comment

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

Indent here.

packages/flutter/test/widgets/editable_text_test.dart Outdated
Copy link
Contributor

@justinmc justinmc May 12, 2022

Choose a reason for hiding this comment

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

Nit: 1800.0 and 2400.0

@markusaksli-nc
Copy link
Contributor Author

markusaksli-nc commented May 12, 2022 •
edited
Loading

I thought of a weird niche scenario where this logic might fail:

On my Galaxy Tab S8 Ultra I can attach a physical keyboard + touchpad and switch to Samsung's pseudo-desktop environment Samsung Dex. There you can resize app windows (making the constraints go from portrait to landscape). When compared to a basic Android native application, however, there is no difference in behavior so this PR is fine.

That being said @justinmc I didn't see this in your context menus design doc and I just found this out myself: did you know that right-clicking with a physical touchpad on Android in said basic Android native app gives this expanded huge context menu by default (at least on my Tab S8 Ultra)?

This is obviously completely different from flutter, where it just shows the regular text selection toolbar. Didn't find an issue for this either.

Also cheers for the quick reviews (and @justinmc I really enjoyed your I/O workshop on macOS flutter development)

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented May 13, 2022

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@markusaksli-nc
Copy link
Contributor Author

markusaksli-nc commented May 14, 2022

@justinmc need some help with failed google testing

@goderbauer
Copy link
Member

goderbauer commented May 18, 2022

(from triage): The failures looked unrelated. I've kicked off another test run to verify...

@markusaksli-nc
Copy link
Contributor Author

markusaksli-nc commented May 25, 2022

@goderbauer did it fail again?

@goderbauer
Copy link
Member

goderbauer commented May 25, 2022

Could you rebase this with the latest master to try again?

@fluttergithubbot fluttergithubbot merged commit b05741c into flutter:master May 25, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 25, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 25, 2022
@engine-flutter-autoroll
b05741c Hide the text selection toolbar on mobile when orientation ch…
3a36996 
…anges (flutter/flutter#103512)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 25, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 25, 2022
@engine-flutter-autoroll
b05741c Hide the text selection toolbar on mobile when orientation ch…
3dc5422 
…anges (flutter/flutter#103512)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 25, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 25, 2022
@engine-flutter-autoroll
b05741c Hide the text selection toolbar on mobile when orientation ch…
e615015 
…anges (flutter/flutter#103512)
@markusaksli-nc markusaksli-nc deleted the toolbar-orientation branch May 26, 2022 08:12
@markusaksli-nc markusaksli-nc mentioned this pull request May 27, 2022
Closed
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
@markusaksli-nc @camsim99
Hide the text selection toolbar on mobile when orientation changes (f…
5f02b38 
…lutter#103512)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
@engine-flutter-autoroll
b05741c Hide the text selection toolbar on mobile when orientation ch…
d915de6 
…anges (flutter/flutter#103512)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
@engine-flutter-autoroll
b05741c Hide the text selection toolbar on mobile when orientation ch…
bdb5eff 
…anges (flutter/flutter#103512)
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

@Renzo-Olivares Renzo-Olivares Renzo-Olivares approved these changes

Assignees

No one assigned

Labels

a: text input Entering text in a text field or keyboard related problems 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.

Hide the Text Selection Menu on rotation

5 participants

@markusaksli-nc @Renzo-Olivares @fluttergithubbot @goderbauer @justinmc