Skip to content

Fix selection toolbar not showing on drag end #121110

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
tgucio merged 3 commits into from
Mar 26, 2023
Merged

Fix selection toolbar not showing on drag end #121110

tgucio merged 3 commits into from
Mar 26, 2023

Conversation

@jankuss
Copy link
Contributor

@jankuss jankuss commented Feb 20, 2023 •
edited
Loading

Fixes #119314

The regression was caused initially by this commit: f014c1e

Before

Before, ending the drag didn't show the toolbar, making it impossible to copy the text after adjusting the selection handles.

Screen.Recording.2023-02-20.at.23.32.51.mov

After

Now ending the drag will show the toolbar again:

Screen.Recording.2023-02-20.at.23.29.18.mov

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 f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Feb 20, 2023
@jankuss jankuss force-pushed the fix-selection-toolbar-not-showing branch 2 times, most recently from 058d8d1 to 5c017ea Compare February 21, 2023 08:22
@HansMuller HansMuller requested a review from justinmc February 22, 2023 01:23
justinmc
justinmc reviewed Feb 28, 2023
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.

Thank you for fixing this! We totally missed this when implementing the magnifier.

Just a minor comment about the use of pump in your tests, and you also need to sync with the master branch. Otherwise this should be good to go.

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

@justinmc justinmc Feb 28, 2023

Choose a reason for hiding this comment

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

Good catch, context should be provided when contextMenuBuilder is provided. Thanks!

packages/flutter/test/material/selection_area_test.dart Outdated
Copy link
Contributor

@justinmc justinmc Feb 28, 2023

Choose a reason for hiding this comment

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

Can you replace these pumps with pumpAndSettle or, if possible, a pump with no Duration? Ideally we shouldn't have any arbitrary timing things like this unless it's totally necessary.

Copy link
Contributor Author

@jankuss jankuss Mar 3, 2023

Choose a reason for hiding this comment

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

I was able to remove most of the pumps with duration, except for one location. I believe this one is necessary though, because it is also used in other tests.

For example, here:
https://github.com/flutter/flutter/blob/686266e4184a107b5e52ddc871cb5400d8caf9df/packages/flutter/test/material/selection_area_test.dart#L61

Copy link
Contributor

@justinmc justinmc Mar 3, 2023

Choose a reason for hiding this comment

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

Sounds good, it's fine like this then 👍

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares Mar 26, 2023

Choose a reason for hiding this comment

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

I think this particular use of pump is so the down event is held and detected as a long press.

@jankuss jankuss force-pushed the fix-selection-toolbar-not-showing branch from 5c017ea to 686266e Compare March 3, 2023 18:42
justinmc
justinmc approved these changes Mar 3, 2023
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍

I re-ran two tests that looked like infrastructure flakes.

@jankuss jankuss force-pushed the fix-selection-toolbar-not-showing branch from 686266e to ae81915 Compare March 4, 2023 08:39
@justinmc
Copy link
Contributor

justinmc commented Mar 6, 2023 •
edited
Loading

The test failure is something unrelated that I've seen on another PR as well. Can you push a merge commit?

Or a rerun may do it.

@justinmc justinmc mentioned this pull request Mar 6, 2023
Closed
@jankuss jankuss force-pushed the fix-selection-toolbar-not-showing branch from ae81915 to 63b67ba Compare March 23, 2023 18:07
@jankuss
Copy link
Contributor Author

jankuss commented Mar 23, 2023

@Renzo-Olivares @LongCatIsLooong could you take a look?

Renzo-Olivares
Renzo-Olivares approved these changes Mar 26, 2023
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

@tgucio tgucio merged commit 4a1002e into flutter:master Mar 26, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 27, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2023
@engine-flutter-autoroll
4a1002e Fix selection toolbar not showing on drag end (flutter/flutte…
06a9bcd 
…r#121110)
This was referenced Mar 28, 2023
Merged
Merged
@mzdm
Copy link
Contributor

mzdm commented Apr 1, 2023

Would be good cherry pick candidate IMO

@reidbaker reidbaker mentioned this pull request Apr 5, 2023
Merged
8 tasks
@justinmc
Copy link
Contributor

justinmc commented Apr 5, 2023 •
edited
Loading

A new stable release will be cut soon with this fix in it, so I think that will work out and we won't need to cherry pick.

Well CC @itsjustkevin in case. I guess it won't be released for awhile.

@devjiangzhou devjiangzhou mentioned this pull request Apr 11, 2023
Closed
6 tasks
@Renzo-Olivares Renzo-Olivares mentioned this pull request Apr 12, 2023
Closed
@danagbemava-nc danagbemava-nc mentioned this pull request Apr 24, 2023
Closed
2 tasks
@mingxin-yang
Copy link

mingxin-yang commented May 10, 2023

Hello, which flutter version fixes this function?

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
4a1002e Fix selection toolbar not showing on drag end (flutter/flutte…
f11a469 
…r#121110)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
4a1002e Fix selection toolbar not showing on drag end (flutter/flutte…
9b839cb 
…r#121110)
@justinmc
Copy link
Contributor

justinmc commented May 10, 2023

Looks like this was tagged in 3.9.0-18.0.pre.

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

@LongCatIsLooong LongCatIsLooong Awaiting requested review from LongCatIsLooong

Assignees

No one assigned

Labels

f: material design flutter/packages/flutter/material 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.

[SelectionArea] Selecting text and then resizing the selection dismisses the contextMenu

6 participants

@jankuss @justinmc @mzdm @mingxin-yang @Renzo-Olivares @tgucio