Skip to content

closes #150048 Autocomplete don't hide options when keyboardDismissBehavior: ScrollViewKeyboardDismissBehavior.onDrag on desktop #150317

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
Closed
D-extremity wants to merge 8 commits into from
Closed

closes #150048 Autocomplete don't hide options when keyboardDismissBehavior: ScrollViewKeyboardDismissBehavior.onDrag on desktop #150317

D-extremity wants to merge 8 commits into from

Conversation

@D-extremity
Copy link
Contributor

@D-extremity D-extremity commented Jun 16, 2024 •
edited
Loading

In windowsx Linux, MacOs ScrollController().position.pixels will not recognize mouse pointer or scroll wheel of mouse if it's scrolled or not until its tapped on screen.
but for feature we just need to check if focus if there or not.

Now it is working for android,website,windows,macos and linux
closes #150048

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].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

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

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Jun 16, 2024
@D-extremity
Copy link
Contributor Author

D-extremity commented Jun 18, 2024 •
edited
Loading

@justinmc @huycozy please review it when you have time

@goderbauer goderbauer requested a review from justinmc June 18, 2024 22:03
@D-extremity
Copy link
Contributor Author

D-extremity commented Jun 25, 2024

@justinmc please review it in your feasible time.

@D-extremity
Copy link
Contributor Author

D-extremity commented Jul 8, 2024

@justinmc sir please review this PR .

justinmc
justinmc reviewed Jul 8, 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.

Sorry for the late review here. See the table below for my understanding of this behavior.

Scenario Is the field unfocused with ScrollViewKeyboardDismissBehavior.onDrag? (master) Is the field unfocused with ScrollViewKeyboardDismissBehavior.onDrag? (this PR)
MacOS scrolling with a trackpad or mouse Yes Yes
Desktop web scrolling with trackpad or mouse No Yes
Android scrolling with finger Yes Yes
iOS scrolling with finger Yes Yes

I think I agree with the change in this PR, though the name onDrag and the deliberate check for dragDetails being null makes me wonder why it was written that way in the first place. @gaaclarke Do you have any thoughts since you wrote ScrollViewKeyboardDismissBehavior? #52068

Also @adonisRodxander does this fix your problem from #150048?

@adonisRodxander
Copy link

adonisRodxander commented Jul 8, 2024

@justinmc yes, The behavior you describe in the previus table is what i'm looking for

@D-extremity D-extremity requested a review from justinmc July 10, 2024 06:22
justinmc
justinmc reviewed Jul 12, 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.

Looks like @gaaclarke is out of office for awhile, but I think we do want this change, so let's move forward with it 👍 . If you see this when you get back and have thoughts let me know @gaaclarke.

Just some small stuff to clean up.

@D-extremity D-extremity requested a review from justinmc July 20, 2024 20:33
@D-extremity
Copy link
Contributor Author

D-extremity commented Jul 22, 2024 •
edited
Loading

@justinmc please review it in your available time

@gaaclarke
Copy link
Member

gaaclarke commented Jul 23, 2024

@gaaclarke Do you have any thoughts since you wrote ScrollViewKeyboardDismissBehavior? #52068

It's been a long time since I wrote this but this sounds reasonable. My interpretation is that ScrollUpdateNotification.dragInfo being null means that the scroll has happened without a drag. That matches the intent of this PR to make it also work for things like scroll wheel events. That sounds good to me with the caveat that I don't think I've ever looked at this whole section of code in the past 4 years.

justinmc
justinmc reviewed Jul 23, 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.

I tried the test on master and it passes.

@D-extremity D-extremity requested a review from justinmc July 23, 2024 22:03
@D-extremity
Copy link
Contributor Author

D-extremity commented Jul 23, 2024

I tried the test on master and it passes.

I changed the test case to make it more meaningful for this issue. please review it in your available time.

justinmc
justinmc reviewed Jul 23, 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.

I tried the test on master again and it still passes. Can you try running it on master and make sure it breaks? That way if your change gets undone somehow, the test will fail and we'll be aware.

@goderbauer
Copy link
Member

goderbauer commented Aug 20, 2024

(triage): @D-extremity Do you still have plans to follow up on the feedback given above?

@victorsanni victorsanni mentioned this pull request Sep 24, 2024
Merged
9 tasks
@victorsanni
Copy link
Contributor

victorsanni commented Sep 24, 2024

#154675 is closer to completion. If that lands first, this PR should be closed.

@Piinks
Copy link
Contributor

Piinks commented Sep 24, 2024

Thank you for your contribution. I'm going to close this PR for now since there are outstanding comments, just to get this off our PR review queue. Please don't hesitate to re-open or submit a new PR if you have the time to address the review comments. Thanks!

@Piinks Piinks closed this Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justinmc justinmc justinmc left review comments

Assignees

No one assigned

Labels

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.

Autocomplete don't hide options when keyboardDismissBehavior: ScrollViewKeyboardDismissBehavior.onDrag on desktop

7 participants

@D-extremity @adonisRodxander @gaaclarke @goderbauer @victorsanni @Piinks @justinmc