-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Android] DatePicker Focus and Unfocus events #20547
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
[Android] DatePicker Focus and Unfocus events #20547
Conversation
|
Hey there @kubaflo! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
If we apply these changes, we should apply them to the TimePicker. |
jsuarezruiz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could replicate the same changes in the TimePicker?
|
@jsuarezruiz sure! I've added a commit with this |
8841303 to
0625df0
Compare
jonathanpeppers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM now. 👍
We should check the new test passed when CI is done.
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@PureWeen @tj-devel709 how about reviewing this? |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Hi, me and my team are migrating our Xamarin app to MAUI, and we need this feature to move forward, if anyone could take a look at this PR, thanks |
|
/azp run |
|
Hi - as I understand it this fix hasn't made it into the release due to a merge issue. Would it be possible to get this looked at as this issue is causing a headache for us. Thanks Dan |
|
Hello Maui team. Is there any way this merge can be reviewed/approved? We are trying to convert our mobile project from Xamarin and this issue is preventing us from doing so. Should be a simple approval. Thank you! |
|
This fix is needed... will it be merged soon? |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
6874961 to
6ab422d
Compare
|
@kubaflo nice! |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
6ab422d to
74b20c7
Compare
74b20c7 to
b5e571f
Compare
|
@PureWeen Just to confirm — based on this #28122 (comment), does this mean the PR won’t be moving forward? |
I think the PRs are different. The other one did this: PlatformView?.ClearFocus();It actually changed the focus on the control. This PR just updates the state in the virtual/xaml view so we get events. However, this does still mean we need to verify that it does not cause issues. @PureWeen did you have an actual physical keyboard (on the desk) or is there a "virtual physical" keyboard that we can use to test? |
mattleibow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am approving this with regards to code and logic and how things "should" work. However, it would be great to get someone with a physical keyboard on their phone to confirm that it does not crash the app or have some focus loops.
|
/rebase |
b5e571f to
344f3ef
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
bhavanesh2001
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The control isn’t a true platform input — it's a mix of AppCompatEditText and a dialog. Since nothing is actually focused at the platform level when the dialog is shown, mapping IsFocused to dialog show/dismiss is misleading.
In the case of Picker, a lot of custom wiring was done (like making EditText focusable on touch) just to make focus/unfocus work — which ended up causing subtle bugs: #29068 (comment)
Just wanted to flag this, With #29323 shaping up for .NET 10, might be worth aligning with that direction.
PureWeen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pausing based on this comment
Do we still need this PR since we are adding APIS in NET10 for opening and closing pickers?
|
Closing this in favor of APIs we're adding into .NET 10 |
Description of Change
Added DatePicker Focus and Unfocus events
Issues Fixed
Fixes #18797
Screen.Recording.2024-02-13.at.14.23.34.mov