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

Add focus check to onTapUpOutside #162939

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add focus check to onTapUpOutside #162939

wants to merge 6 commits into from
+182 −40

Conversation

Hannnes1
Copy link
Contributor

@Hannnes1 Hannnes1 commented Feb 8, 2025

Adds a focus check to onTapUpOutside of EditableText, so that it doesn't trigger if the EditableText doesn't have focus.

onTapOutside already had this check, but it was missed when onTapUpOutside was added.

Fixes #162573

Pre-launch Checklist

@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. labels Feb 8, 2025
Hannnes1
Hannnes1 commented Feb 8, 2025
View reviewed changes
packages/flutter/test/widgets/editable_text_test.dart
});

// Regression test for https://github.com/flutter/flutter/issues/162573
testWidgets('onTapUpOutside is not called upon tap up outside when field is not focused', (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The equivalent test for onTapOutside is located in text_form_field_test.dart#L741, but in my opinion it should have been located in this file instead. That's why I added this test here. Let me know if I should move it to text_form_field_test.dart instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test is good in editable_text_test.dart. If you'd like you can also transfer the equivalent onTapOutside test from text_form_field_test.dart to editable_text_test.dart, I think it fits better in here.

Renzo-Olivares
Renzo-Olivares approved these changes Mar 4, 2025
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, thank you for the contribution!

hannah-hyj
hannah-hyj approved these changes Mar 5, 2025
View reviewed changes
Copy link
Member

@hannah-hyj hannah-hyj left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions github-actions bot added the f: material design flutter/packages/flutter/material repository. label Mar 12, 2025
@Hannnes1
Copy link
Contributor Author

@Renzo-Olivares I added a _hadFocusOnTapDown that fixes the issue with onTapOutside blocking onTapUpOutside

@justinmc
Copy link
Contributor

@Hannnes1 FYI there are some test failures now.

@Hannnes1
Copy link
Contributor Author

Thanks.
Haven't figured out why yet, but will look into it

@Hannnes1
Copy link
Contributor Author

I think I'm gonna need some help with this one. I can't figure out why these tests fail

@justinmc
Copy link
Contributor

Looks like the semantics tree doesn't match any more. I see the flags SemanticsFlag.hasRequiredState and
SemanticsFlag.isRequired are missing now I think? Not sure if that's intended or not.

@Renzo-Olivares
Copy link
Contributor

Renzo-Olivares commented Mar 20, 2025 •
edited
Loading

I know there have been a lot of semantic changes recently, maybe rebasing to master will fix some of these?

@Hannnes1
Copy link
Contributor Author

I know there have been a lot of semantic changes recently, maybe rebasing to master will fix some of these?

Rebasing again worked. Thanks!

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

@hannah-hyj hannah-hyj hannah-hyj approved these changes

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

EditableText.onTapUpOutside is triggered even if the EditableText doesn't have focus
4 participants
@Hannnes1 @justinmc @Renzo-Olivares @hannah-hyj