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.

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

Fix ColorPicker virtual keyboard popup on mobile #97807

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

syntaxerror247
Copy link
Member

This PR fixes auto popup of virtual keyboard when colorPickerButton is pressed on mobile devices.
Fixes #88235

@syntaxerror247 syntaxerror247 requested a review from a team as a code owner October 4, 2024 10:21
@Chaosus Chaosus added this to the 4.4 milestone Oct 4, 2024
@syntaxerror247 syntaxerror247 marked this pull request as draft October 4, 2024 10:36
@syntaxerror247 syntaxerror247 force-pushed the colorPicker_kb_fix branch 3 times, most recently from c912f00 to 6f795f8 Compare October 4, 2024 11:37
@syntaxerror247 syntaxerror247 marked this pull request as ready for review October 4, 2024 11:47
@WhalesState
Copy link
Contributor

WhalesState commented Oct 4, 2024

It looks good but I don't know if we should return from TextEdit and LineEdit show_virtual_keyboard instead, to avoid showing the virtual keyboard when a physical keyboard is detected, at least we can do it for android and ios only but what if the users still wants to use the virtual keyboard ? There's no option to prefer virtual keyboard over physical one.

@syntaxerror247
Copy link
Member Author

@WhalesState Isn't this the current behaviour, virtual keyboard does not appear if physical keyboard is connected.

@WhalesState
Copy link
Contributor

@WhalesState Isn't this the current behaviour, virtual keyboard does not appear if physical keyboard is connected.

There was no way to check for physical keyboards before, it was recently merged so it always show the virtual keyboard on android and ios by default.

@WhalesState
Copy link
Contributor

That's why i have applied a hack for this, since focusing any LineEdit or TextEdit will still show the virtual keyboard even if a hardware keyboard is connected, but I don't know if there are any opened issues or discussions about this.

@syntaxerror247
Copy link
Member Author

syntaxerror247 commented Oct 4, 2024

There was no way to check for physical keyboards before, it was recently merged so it always show the virtual keyboard on android and ios by default.

No, virtual keyboard doesn't appear if physical kb is connected even before that PR . This was used internally just wasn't exposed.

@WhalesState
Copy link
Contributor

There's an option on android Physical keyboard settings Show on-screen keyboard while a physical keyboard is being used, which is true by default.

@syntaxerror247
Copy link
Member Author

syntaxerror247 commented Oct 4, 2024

There's an option on android Physical keyboard settings Show on-screen keyboard while a physical keyboard is being used, which is true by default.

I just checked in two different devices and it was turned off in Motorola and turned on in Xiaomi, both are default.
So, yeah the only way to fix this will be to not call show_virtual_keyboard() in lineEdit and textEdit.

But that is out of scope of this PR.

@AThousandShips AThousandShips changed the title Fix colorPicker virtual keyboard popup on mobile Fix ColorPicker virtual keyboard popup on mobile Oct 4, 2024
@syntaxerror247 syntaxerror247 force-pushed the colorPicker_kb_fix branch 2 times, most recently from 953c625 to abbf90a Compare October 4, 2024 16:55
@KoBeWi
Copy link
Member

KoBeWi commented Oct 5, 2024

The linked issue was closed by another PR. Is this still relevant?

@syntaxerror247
Copy link
Member Author

syntaxerror247 commented Oct 5, 2024

The linked issue was closed by another PR. Is this still relevant?

Yes, this fix is simpler and IMO better.
I think the only difference is that, fix in that PR hides virtual keyboard but still keep html_color box focused, But I think completely removing the focus would be better.

@KoBeWi It would be great if you could have a look at changes 🙂

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

This basically reverts the ColorPicker part of #97094 idk why that code was merged if the author of that PR has approved this one 🤷‍♂️

The code looks ok, but I didn't test it on mobile.

@WhalesState
Copy link
Contributor

I've approved this PR since it was submitted after my own fix and my PR was approved but not merged yet. However, I do have some questions about the approach taken here.

I'm not sure why this fix was submitted as a separate PR instead of being discussed in my original PR, where the issue was already being tracked and addressed.

Regarding the fix itself, I agree that it's simpler. However, I do have some concerns about its accuracy. Specifically, every popup should focus on at least one control.

Also to avoid potential regressions on other platforms when has_hardware_keyboard() is supported, I made sure to apply the changes only to Android and iOS.

I'm not familiar enough with the underlying mechanics to provide a definitive opinion on which fix is better. I'd be happy to let more experienced team members weigh in and provide guidance on the best approach.

@syntaxerror247
Copy link
Member Author

syntaxerror247 commented Oct 5, 2024

I'm not sure why this fix was submitted as a separate PR instead of being discussed in my original PR, where the issue was already being tracked and addressed.

That PR was already addressing multiple issues, so I thought it would be better to open a new one. ( I accept that was an error an my part)

Regarding the fix itself, I agree that it's simpler. However, I do have some concerns about its accuracy. Specifically, every popup should focus on at least one control.

If hardware keyboard is not connected, this fix won't even create a popup or automatically try to open virtual keyboard (when colorpicker button is pressed).
It will only focus the html_color field if hardware keyboard is connected OR specifically clicked on the html_color field.

Also to avoid potential regressions on other platforms when has_hardware_keyboard() is supported, I made sure to apply the changes only to Android and iOS.

This fix won't affect any other platform because has_hardware_keyboard() will always return true on any other platform. And if this func gets supported on other platforms then it should work there too.

I hope all your questions are answered :)

@fire fire requested a review from a team October 5, 2024 17:18
Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

The code and the reasoning looks good to me!

@Repiteo Repiteo merged commit 29fa4b1 into godotengine:master Oct 21, 2024
19 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 21, 2024

Thanks!

@syntaxerror247 syntaxerror247 deleted the colorPicker_kb_fix branch October 22, 2024 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Color Picker on Mobile causes keyboard to popup by default
6 participants