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

[iOS] Fix focus issues with DatePicker and TimePicker #13321

Merged
merged 25 commits into from
Apr 13, 2023
Merged

Conversation

jsuarezruiz
Copy link
Contributor

Description of Change

Fix focus issues with DatePicker and TimePicker on Android and iOS.

fix-12899

To validate the changes can use the added sample in the DatePicker and TimePicker pages inside the .NET MAUI Gallery. Also included some device tests.

Issues Fixed

Fixes #12899

@jsuarezruiz jsuarezruiz added t/bug Something isn't working legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor area-controls-datetimepicker DatePicker, TimePicker labels Feb 14, 2023
@github-actions
Copy link
Contributor

⚠️ Your code has been reformatted. ⚠️

Copy link
Member

@mandel-macaque mandel-macaque left a comment

Choose a reason for hiding this comment

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

minor changes.

Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

On iOS we need a better way to unsubscribe events

src/Core/src/Handlers/DatePicker/DatePickerHandler.iOS.cs Outdated Show resolved Hide resolved
@PureWeen PureWeen requested review from rachelkang and removed request for rmarinho February 23, 2023 21:23
@PureWeen
Copy link
Member

@rachelkang can you review this PR for any accessibility concerns? I don't think it really touches anything in that area but just want to make sure.

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Apologies on this one @jsuarezruiz but my last comment was incorrect. I see now that the EditingDidBegin and EditingDidEnd that I referenced are different than the ones you subscribed to.

I tested the code here a bit and it looks like the reason subscribing to those events leaks is because we are also subscribing to those events on the UIDatePicker here

https://github.com/dotnet/maui/blob/main/src/Core/src/Platform/iOS/MauiDatePicker.cs#L35-L37

I don't know exactly why one effects the other but I'm guessing it causes some type of circular path between the handler and the input view. It doesn't really matter though.

I think when we copied this renderer over from forms we incorrectly subscribed to EditingDidBegin and EditingDidEnd on the UIDatePicker instead of the MauiDatePicker control.

I think the fix for iOS here is purely to change these two lines of code here

https://github.com/dotnet/maui/blob/main/src/Core/src/Platform/iOS/MauiDatePicker.cs#L35-L37

to

this.EditingDidBegin += OnStarted;
this.EditingDidEnd += OnEnded;

vs

picker.EditingDidBegin += OnStarted;
picker.EditingDidEnd += OnEnded;

If I do that then the IsFocused property starts to work and the allocation test passes

Copy link
Member

@rachelkang rachelkang left a comment

Choose a reason for hiding this comment

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

The focus behavior on Android, as it behaves for picker on this PR, mostly makes sense to me. Clicking on the picker will trigger Focus and clicking on something else will trigger Unfocus. Similarly, tab key navigating to the picker will trigger Focus and tav key navigating away to something else will trigger Unfocus. The one thing I'm unsure about on Android - what should happen when clicking "Done"? On this PR, focus automatically moves to a different control in order to Unfocus the current one. Previously, focus remained on the control until you navigated away from it.

From what I can tell, on iOS, the focus behavior appears to behave the same as before (buggy). Clicking on / tab key navigating to the picker is not enough to trigger Focus. Focus is only triggered once a (new) selection is made on the picker. Unfocus didn't seem to get triggered at all, even when a different control received focus.

@rachelkang
Copy link
Member

Related: #13503, #6252

@jsuarezruiz
Copy link
Contributor Author

jsuarezruiz commented Mar 20, 2023

The focus behavior on Android, as it behaves for picker on this PR, mostly makes sense to me. Clicking on the picker will trigger Focus and clicking on something else will trigger Unfocus. Similarly, tab key navigating to the picker will trigger Focus and tav key navigating away to something else will trigger Unfocus. The one thing I'm unsure about on Android - what should happen when clicking "Done"? On this PR, focus automatically moves to a different control in order to Unfocus the current one. Previously, focus remained on the control until you navigated away from it.

From what I can tell, on iOS, the focus behavior appears to behave the same as before (buggy). Clicking on / tab key navigating to the picker is not enough to trigger Focus. Focus is only triggered once a (new) selection is made on the picker. Unfocus didn't seem to get triggered at all, even when a different control received focus.

Testing on iOS, both, DatePicker and TimePicker:

  • Got the focus tapping on the control.
  • Lost the focus tapping elsewhere, for example, in another control.
  • Lost the focus tapping the Done button.
  • Moving between DatePicker/TimePicker using the physical keyboard and Tab.

timepicker-ios-focus
datepicker-ios-focus

@github-actions
Copy link
Contributor

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

Copy link
Member

@rachelkang rachelkang left a comment

Choose a reason for hiding this comment

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

Awesome!!! :)

can confirm focus and unfocus work beautifully for both DatePicker and TimePicker now.

to summarize the behavior I'm seeing, which is now the same for both DatePicker and TimePicker, and the same on both iOS and Android:

  • Keyboard tab navigation to the control will Focus
  • Keyboard tab navigation away from the control will Unfocus
  • Clicking to open the control will Focus
  • Clicking Done to close the control will Unfocus

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Hold please on this one
I'm checking a few things

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

I reviewed the Android changes a bit more here, and I'm not convinced this is the right path with Android.

Currently we are watching for focus changed at the ViewHandler level on android which causes a weird ping pong effect now when you click to show a dialog.

2023-03-30_13-31-29

I asked on the original issue #12899 (comment). I think we just keep overloading the focus behavior on the pickers too much and we need to just add APIs for "open/close" and then "opened/closed" events.

My vote would be to back out the Android changes on this PR. I think the iOS changes are staying true to the platform as they are tied to "editingbegin/editingend".

If we want to keep the Android changes, we should fix the ping pong of the focus property, but I'm inclined to just leave it how it is and add new APIs

@jsuarezruiz
Copy link
Contributor Author

I reviewed the Android changes a bit more here, and I'm not convinced this is the right path with Android.

Currently we are watching for focus changed at the ViewHandler level on android which causes a weird ping pong effect now when you click to show a dialog.

2023-03-30_13-31-29 2023-03-30_13-31-29

I asked on the original issue #12899 (comment). I think we just keep overloading the focus behavior on the pickers too much and we need to just add APIs for "open/close" and then "opened/closed" events.

My vote would be to back out the Android changes on this PR. I think the iOS changes are staying true to the platform as they are tied to "editingbegin/editingend".

If we want to keep the Android changes, we should fix the ping pong of the focus property, but I'm inclined to just leave it how it is and add new APIs

Good catch Shane!. I've been reviewing what's going on and have made changes that I think fix the problem.

fix-datepicker-focus-behavior

Could you review it again?

@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2023

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

@@ -45,7 +45,8 @@ void Initialize()
DrawableCompat.Wrap(Background);

Focusable = true;
FocusableInTouchMode = false;
FocusableInTouchMode = true;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can toggle this to true. @hartez specifically set this to false here so that the picker wouldn't open up on focus.

What do you think about removing all the android parts of this PR and creating new APIs for "opened/closed"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied changes removing all the android parts. Let's create the opened/closed APIs in another PR.

@jsuarezruiz jsuarezruiz changed the title [Android & iOS] Fix focus issues with DatePicker and TimePicker [iOS] Fix focus issues with DatePicker and TimePicker Apr 12, 2023
@jsuarezruiz jsuarezruiz merged commit 78a09db into main Apr 13, 2023
29 checks passed
@jsuarezruiz jsuarezruiz deleted the fix-12899 branch April 13, 2023 11:17
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2023
@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-datetimepicker DatePicker, TimePicker platform/iOS 🍎 t/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DatePicker Focused, UnFocused not working as expected
7 participants