Skip to content

fix:: trigger onTapOutside only if has focus #136291

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
auto-submit merged 14 commits into from
Oct 26, 2023
Merged

fix:: trigger onTapOutside only if has focus #136291

auto-submit merged 14 commits into from
Oct 26, 2023

Conversation

@Mairramer
Copy link
Contributor

@Mairramer Mairramer commented Oct 10, 2023

Adds new feat and fix to #134341

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.

@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: material design flutter/packages/flutter/material repository. f: cupertino flutter/packages/flutter/cupertino repository labels Oct 10, 2023
@Mairramer Mairramer mentioned this pull request Oct 10, 2023
Closed
8 tasks
@Mairramer Mairramer changed the title feat:: add new option to trigger onTapOutside feat:: add the possibility of execute onTapOutside only if has focus Oct 11, 2023
@LongCatIsLooong
Copy link
Contributor

LongCatIsLooong commented Oct 12, 2023

@gspencergoog do you think we can change the default behavior of onTapOutside to execute only if it has focus?

@gspencergoog
Copy link
Contributor

gspencergoog commented Oct 12, 2023

In fact, I think it might be OK to just eliminate the extra configuration parameter and just make it always only act if the text field has focus. That would make the implementation here simpler, and I don't think it's that surprising. Is there a concrete use case for it happening all the time?

@Mairramer
Copy link
Contributor Author

Mairramer commented Oct 23, 2023

@gspencergoog Hi, I did it this way because there may be cases where developers want to use it this way. And also to avoid regression in some tests.

@LongCatIsLooong
Copy link
Contributor

LongCatIsLooong commented Oct 24, 2023

avoid regression in some tests

It should be OK to change some existing flutter/flutter tests.

there may be cases where developers want to use it this way

Once a new public API is added we're committed to maintain that API until it gets deprecated and removed. I think it would be a bit safer/less committal to try changing the default behavior first, and introduce the extra parameter once there's a concrete use case for that. WDYT?

@Mairramer
Copy link
Contributor Author

Mairramer commented Oct 24, 2023

avoid regression in some tests

It should be OK to change some existing flutter/flutter tests.

there may be cases where developers want to use it this way

Once a new public API is added we're committed to maintain that API until it gets deprecated and removed. I think it would be a bit safer/less committal to try changing the default behavior first, and introduce the extra parameter once there's a concrete use case for that. WDYT?

Ok, I agree. So as soon as possible I will roll back the code and only change what is really necessary.

@Mairramer Mairramer changed the title feat:: add the possibility of execute onTapOutside only if has focus fix:: trigger onTapOutside only if has focus Oct 24, 2023
Renzo-Olivares
Renzo-Olivares reviewed Oct 24, 2023
View reviewed changes
Renzo-Olivares
Renzo-Olivares reviewed Oct 24, 2023
View reviewed changes
Renzo-Olivares
Renzo-Olivares approved these changes Oct 24, 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

@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 26, 2023
@LongCatIsLooong
Copy link
Contributor

LongCatIsLooong commented Oct 26, 2023

LGTM, looks like this isn't a breaking change. Thanks for the fix!

@auto-submit auto-submit bot merged commit de4e3d6 into flutter:master Oct 26, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 26, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 26, 2023
@engine-flutter-autoroll
de4e3d6 fix:: trigger onTapOutside only if has focus (flutter/flutter…
be47067 
…#136291)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 26, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 26, 2023
@engine-flutter-autoroll
de4e3d6 fix:: trigger onTapOutside only if has focus (flutter/flutter…
27d8bb6 
…#136291)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 26, 2023
@engine-flutter-autoroll
Roll Flutter from 5dd2a4e to c555599 (14 revisions) (#5237)
9e4ca6b 
flutter/flutter@5dd2a4e...c555599

2023-10-26 [email protected] Roll Packages from f751ffb to fea24c5 (4 revisions) (flutter/flutter#137339)
2023-10-26 [email protected] Roll Flutter Engine from 23649a0529e5 to 61ae5ef94e9c (1 revision) (flutter/flutter#137337)
2023-10-26 [email protected] Run test with older version of Xcode (flutter/flutter#137293)
2023-10-26 [email protected] Roll Flutter Engine from f8525b9fdddf to 23649a0529e5 (1 revision) (flutter/flutter#137333)
2023-10-26 [email protected] Add ProcessTextService (flutter/flutter#137145)
2023-10-26 [email protected] Roll Flutter Engine from b6bea17e39d3 to f8525b9fdddf (1 revision) (flutter/flutter#137330)
2023-10-26 [email protected] Roll Flutter Engine from ec29ea930f58 to b6bea17e39d3 (1 revision) (flutter/flutter#137329)
2023-10-26 [email protected] Roll Flutter Engine from 2a01fae53ded to ec29ea930f58 (1 revision) (flutter/flutter#137326)
2023-10-26 [email protected] Roll Flutter Engine from 10a91eb44c5b to 2a01fae53ded (1 revision) (flutter/flutter#137320)
2023-10-26 [email protected] Roll Flutter Engine from f844ab933364 to 10a91eb44c5b (1 revision) (flutter/flutter#137317)
2023-10-26 [email protected] Roll Flutter Engine from 6428ed576841 to f844ab933364 (1 revision) (flutter/flutter#137314)
2023-10-26 [email protected] Roll Flutter Engine from 6e09ee14e244 to 6428ed576841 (38 revisions) (flutter/flutter#137310)
2023-10-26 [email protected] fix:: trigger onTapOutside only if has focus (flutter/flutter#136291)
2023-10-25 [email protected] Fix structure of pkg:integration_test (flutter/flutter#137283)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@HansMuller HansMuller mentioned this pull request Nov 1, 2023
Closed
@gspencergoog gspencergoog mentioned this pull request Nov 16, 2023
Open
2 tasks
@Renzo-Olivares Renzo-Olivares mentioned this pull request Nov 30, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Renzo-Olivares Renzo-Olivares Renzo-Olivares 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 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.

4 participants

@Mairramer @LongCatIsLooong @gspencergoog @Renzo-Olivares