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

DateTimePicker: Click on date causes popover to close automatically #24206

Closed
ocean90 opened this issue Jul 26, 2020 · 9 comments · Fixed by #29738
Closed

DateTimePicker: Click on date causes popover to close automatically #24206

ocean90 opened this issue Jul 26, 2020 · 9 comments · Fixed by #29738
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress

Comments

@ocean90
Copy link
Member

ocean90 commented Jul 26, 2020

Describe the bug
I'm trying to use the DateTimePicker component inside of a Dropdown component with other settings. Unfortunately, clicking on a date in the picker automatically closes the popover. This prevents the user to change other settings unless they reopen the popover.

The same happens with the default date picker to schedule a post. It might be the case that this is intentional but it only "works" as is due to the way how the DayPickerSingleDateController and Popover components are working.

To reproduce
Steps to reproduce the behavior:

  1. Create a new post
  2. Open the schedule popover by clicking on the publish date
  3. Change the date or month by using the input/dropdown menu
  4. Notice that the popover does not close
  5. Now click on a date in the picker
  6. The popover automatically closes

See also https://wordpress.slack.com/archives/C02QB2JS7/p1592578807492400 for a gif.

Expected behavior
The popover stays open so I can review the selected data in case I clicked on a wrong date. Or, when used in a custom component, DateTimePicker should have a setting to prevent this behaviour.

Additional context
This happens because when clicked on a date in DayPickerSingleDateController, document.activeElement it set to the body element.
PostSchedule with DateTimePicker is used in a Dropdown component which sets the onFocusOutside prop with a function that checks if the document.activeElement is part of the container ref. Since the body element fails that check so the popover closes.

The document.activeElement issue can also be reproduced on react-dates' storybook: https://airbnb.io/react-dates/iframe.html?id=daypickersingledatecontroller--default

@ocean90 ocean90 added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components labels Jul 26, 2020
@enriquesanchez enriquesanchez added the Good First Issue An issue that's suitable for someone looking to contribute for the first time label Aug 11, 2020
@allilevine
Copy link
Contributor

@pablinos and I can both reproduce this issue. We explored adding a check for if document.activeElement !== document.body to closeIfFocusOutside():

function closeIfFocusOutside() {
if (
! containerRef.current.contains( document.activeElement ) &&
! document.activeElement.closest( '[role="dialog"]' )
) {
close();
}
}

But it didn't seem to receive any outside events and close when it should (e.g. when editing the post title).

Since this is reproducible on react-dates storybook, it’s possible this is a bug that needs to be fixed upstream. This react-dates discussion suggests that document.activeElement should be the DateTimePicker, but it’s not.

@enriquesanchez Did you see a quick solution to this one? Are we missing something obvious?

@enriquesanchez
Copy link
Contributor

Hi @allilevine!

I didn't, sorry. I initially thought this would be a Good First Issue but it's looking like it's more complicated.

@enriquesanchez enriquesanchez removed the Good First Issue An issue that's suitable for someone looking to contribute for the first time label Sep 15, 2020
@tellthemachines
Copy link
Contributor

As noted in #27222, clicking on the AM/PM toggle also causes the popover to close.

@Ipstenu
Copy link
Contributor

Ipstenu commented Feb 10, 2021

I bet this is related -- if you click on the time and change it, it defaults to AM every time. I'm attaching a GIF of this:

am-pm-who-knows

@tomjn
Copy link
Contributor

tomjn commented Feb 23, 2021

We're encountering problems on a client site with the AM/PM toggles as well, setting the hour then changing from AM to PM changes the time selected, but when the popover containing the component is hidden it the date reverts back to AM

@allilevine
Copy link
Contributor

I bet this is related -- if you click on the time and change it, it defaults to AM every time.

We're encountering problems on a client site with the AM/PM toggles as well, setting the hour then changing from AM to PM changes the time selected, but when the popover containing the component is hidden it the date reverts back to AM

I'm taking a look at this issue again and I'm able to reproduce both these issues. It seems like it's specifically changing the hour that switches the toggle back to AM.

@tomjn
Copy link
Contributor

tomjn commented Mar 10, 2021

Do we want to move the AM/PM bug to a new ticket?

I spent 10 minutes to do a quick look these are the next avenues of investigation I would take:

@tomjn
Copy link
Contributor

tomjn commented Mar 10, 2021

I've created #29720 to track the AM/PM bug

@allilevine
Copy link
Contributor

I've created #29720 to track the AM/PM bug

Thanks!

I've opened a PR to fix the original issue (clicking on a date causes the popover to close automatically): #29738

It would be great to get your feedback @ocean90 @tomjn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants