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: changing month or day in post publish date closes the popover. #17201

Conversation

@jorgefilipecosta
Copy link
Member

commented Aug 26, 2019

Description

Fix: #17042

Currently, changing the month using prev/next button in the date picker closes the popover making it impossible to use and making post scheduling a very hard task.

The problem happens because when we click prev/next button a focus lose occurs. I did lots of debugging and I did not find a cause for this. Probably we will need an upstream commit on react-dates. But until then this PR uses onPrevMonthClick/onNextMonthClick events to force the focus to be inside the focusable region.

How has this been tested?

I checked that I can change the day or month in the post scheduler and the popover does not close.

@@ -69,6 +69,9 @@ export default createHigherOrderComponent(
}

queueBlurCheck( event ) {
if ( this.node && event.target && this.node.contains( event.target ) ) {

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 26, 2019

Contributor

Are you sure the referenced line is the cause? Because, the spec says that blur doesn't bubble.

In any case, it's more performant to check that event.currentTarget !== event.target. This would only not be true when the event was triggered by blurring the wrapper element, which is what we want.

As a bonus, it would fix all of the failing tests related to this.node.contains firing before the ref is ready.

I'm curious as to why we implemented this ourselves instead of relying on one of the stable "click outside" libraries. This is a very common pattern.

Copy link
Member

left a comment

When I test this PR I notice that you can no longer dismiss the date picker by clicking anywhere on the Document sidebar. There are similar problems with the inserter and the More menus.

I don't think that this fixes the root cause of the issue which is that there's a focus loss when the Next month button is clicked. It's the loss of focus which is causing withFocusOutside to (correctly) close the menu.

You can see what I mean by:

  1. Open DevTools.
  2. Open the schedule date picker.
  3. Click the Next month button.
  4. In DevTools, run: document.activeElement

document.activeElement should be the Next month button, but it is not.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/changing-month-or-day-in-post-scheduler-closes-the-popover branch from 2c4fc83 to 9ed00a8 Aug 27, 2019
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

Thank you for the reviews @noisysocks, @epiqueras. It seems the previous fix was invalid. And I was not able to find a "correct" fix. Probably we should debug the problem upstream on react dates and add a commit there that avoids the focus lose. But until then I commit a fix that essentially guarantees that when we click next/prev month, the focus is kept on the date picker focusable region.

@jorgefilipecosta jorgefilipecosta dismissed stale reviews from noisysocks and epiqueras Aug 27, 2019

Updated the fix used.

packages/components/src/date-time/date.js Outdated Show resolved Hide resolved
packages/components/src/date-time/date.js Outdated Show resolved Hide resolved
jorgefilipecosta and others added 2 commits Aug 27, 2019
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
@jorgefilipecosta jorgefilipecosta merged commit 5973603 into master Aug 27, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jorgefilipecosta jorgefilipecosta deleted the fix/changing-month-or-day-in-post-scheduler-closes-the-popover branch Aug 27, 2019
@noisysocks

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

Cool hack! Thanks @jorgefilipecosta! 👍

jorgefilipecosta added a commit that referenced this pull request Aug 28, 2019
* Fix: changing month or day in post publish date closes the popover.

* Update packages/components/src/date-time/date.js

Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>

* Update packages/components/src/date-time/date.js

Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
gziolo added a commit that referenced this pull request Aug 29, 2019
* Fix: changing month or day in post publish date closes the popover.

* Update packages/components/src/date-time/date.js

Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>

* Update packages/components/src/date-time/date.js

Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
gziolo added a commit that referenced this pull request Aug 29, 2019
* Fix: changing month or day in post publish date closes the popover.

* Update packages/components/src/date-time/date.js

Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>

* Update packages/components/src/date-time/date.js

Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
@gziolo gziolo added this to the Gutenberg 6.4 milestone Aug 30, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

This one deserves an e2e test :)

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2019

This one deserves an e2e test :)

Hi @youknowriad, I totally agree, I will look into creating a simple e2e test.

dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
…ess#17201)

* Fix: changing month or day in post publish date closes the popover.

* Update packages/components/src/date-time/date.js

Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>

* Update packages/components/src/date-time/date.js

Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.