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

Components: Add onFocusOutside replacement to Popover onClickOutside #14851

Merged
merged 4 commits into from Aug 2, 2019

Conversation

@aduth
Copy link
Member

commented Apr 5, 2019

Fixes #14754, fixes #14849.

This pull request seeks to refactor popovers to avoid relying on "click outside" behaviors in popovers, deprecating the Popover component's onClickOutside prop in favor of a substitute onFocusOutside. This accounts for more reasons for focus transitions not previously reflected in onClickOutside, such as that described by the bug of #14754.

Status: While functionally complete, I'm considering this to be blocked by an apparent incompatibility with a tangentially-related bug described at #14849 . Notably, when toggling an option in the More Options menu, the menu will no longer become closed again from clicks elsewhere in the page.

Testing instructions:

Repeat Steps to Reproduce from #14754 , verifying that the menu is closed as expected.

Verify there are no changes in behavior from other Popover usage, particularly specialized cases such as:

  • Clicking a DropdownMenu toggle button should toggle itself closed when currently opened
  • Clicking a result in the link suggestions autocomplete does not dismiss the URL popover
@gziolo

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Clicking a result in the link suggestions autocomplete does not dismiss the URL popover

There seems to be a different behavior when using mouse and keyboard. Well, there might be something wrong with the URL popover and autocomplete:

  • when I use an arrow key, select one of the results and press space it doesn't get selected
  • when I use an arrow key, select one of the results and press enter it gets selected and focus goes back to RichText field
@gziolo
Copy link
Member

left a comment

I will finish this review tomorrow. It's looking great so far. I left some questions.

*/
import clickOutside from 'react-click-outside';

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 10, 2019

Member

Unrelated question to this PR. Can we also refactor Modal component to stop using react-click-outside? :)

This comment has been minimized.

Copy link
@aduth

aduth Apr 11, 2019

Author Member

Unrelated question to this PR. Can we also refactor Modal component to stop using react-click-outside? :)

I would like to, yes.

( #6261 (comment) 😞 )

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 12, 2019

Member

There is still hope then :)

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 12, 2019

Member

https://unpkg.com/react-click-outside@3.0.1/index.js

Well, it looks like react-click-outside is a simplified version of our own HOC :)

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 2, 2019

Member

I opened a follow-up to address it – #16878.

*
* @param {FocusEvent} event Focus event from onFocusOutside.
*/
onFocusOutside( event ) {

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 10, 2019

Member

How can I test this shim? By updating one of the places with usage?

This comment has been minimized.

Copy link
@aduth

aduth Apr 11, 2019

Author Member

You could locally revert one or both of 55ca76a or 3e8ffc4 to restore those components' use of onClickOutside.

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

This one was a pain to rebase, in good part because of #15053 (cc @youknowriad).

Worth noting that this will resolve some lingering errors after #11360 logged frequently in StrictMode concerning findDOMNode from react-click-outside (related: kentor/react-click-outside#45). Also related to prior conversation #14851 (comment).

@aduth aduth referenced this pull request May 22, 2019
11 of 12 tasks complete
}

render() {
return this.props.children;
}
}

export default clickOutside( PopoverDetectOutside );
export default withFocusOutside( PopoverDetectOutside );

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 23, 2019

Contributor

This could be a nice hook const useOnFocusOutside( ref, handler )

@noisysocks noisysocks force-pushed the remove/popover-on-click-outside branch from 6308529 to 6231b63 Jul 19, 2019

@noisysocks
Copy link
Member

left a comment

I rebased this PR to fix some conflicts that existed in the CHANGELOG.md files. You may want to double check that the changes to CHANGELOG.md make sense.

I followed the testing steps in the description and everything seems okay. I also played with some Popover and DropdownMenu components and couldn't see anything off. I was not able to follow the original steps in the linked issue as that issue was fixed via a different approach.

Would love to get this in as it fixes a bug that's blocking #16582.

@aduth

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

Thanks @noisysocks for the assist here. Looking again over the code and with some final testing, I think this is in good shape for merge. 👍

@aduth

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

Hmm, the end-to-end test failures may be legitimate.

@aduth

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

The end-to-end tests seem to be getting stuck when trying to switch from "Code" editor back to "Visual" in the container blocks test cases. I can't see any reason why it would be such a specific issue here.

My hunches are:

Separately, I stumbled upon this code, which was introduced more recently and seems like something we might want to update:

onClickOutside={ onClickOutside() }

@noisysocks

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

I figured out how to recreate the bug with some simple steps, but I'm no closer to figuring out what's causing it:

  1. Create a new post
  2. Open the More tools & options menu
  3. Click any menu item, e.g. enable Spotlight mode
  4. Click outside of the menu to dismiss it. The menu does not close

Any thoughts, @aduth?

@noisysocks

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Figured it out! It's because clicking on the menu item causes a focus loss. This was reported in #14849. I'll include a fix here.

MenuItem: Always use an IconButton component so as to avoid focus loss.
Switching between `Button` and `IconButton` causes React to remount the
`MenuItem` component. This causes a focus loss as well as E2E failures.

There's no need to use a `Button` for `MenuItems` that are unselected,
since we can simply pass `icon={ undefined }` to the `IconButton`.
@noisysocks

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

I pushed up 738d987 which fixes the E2E test failures and fixes #14849.

Would you mind giving this a quick test, @gziolo?


Testing instructions:

Verify that #14849 is fixed:

  1. Navigate to Posts > Add New
  2. Click or otherwise activate the "More Options" menu in the top-right
  3. Press Spacebar (assumes first menu item focused)
  4. Note focus loss

Verify there are no changes in behavior from other Popover usage, particularly specialized cases such as:

  • Clicking a DropdownMenu toggle button should toggle itself closed when currently opened
  • Clicking a result in the link suggestions autocomplete does not dismiss the URL popover
@gziolo
gziolo approved these changes Aug 1, 2019
tagName,
{
return (
<IconButton

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 1, 2019

Member

It's a bit stretched use of IconButton in the case when there is no icon 😅

However, I can't think of another simple way of handling it where this doesn't trigger re-render of the button. I noticed that this behavior is broken only because we swap IconButton with Button for no reason when only the icon gets removed/added.

This comment has been minimized.

Copy link
@noisysocks

noisysocks Aug 2, 2019

Member

Yep, exactly! Switching between the two components causes React to remount the component and lose focus. Looking at IconButton, it seemed to me that icon={} is an optional prop so I felt comfortable doing it this way.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 2, 2019

Contributor

One thing we keep discussing here and there is why not just add an icon prop to the Button block and deprecate the IconButton (or make it just a shortcut)

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 2, 2019

Member

I'm sure @drw158 @cburton4 have some good ideas on how to approach it :)

I'm fine with anything which makes it less confusing. I guess having more flexible Button and IconButton as a backward-compatible alias makes a lot of sense.

This comment has been minimized.

Copy link
@drw158

drw158 Aug 2, 2019

Contributor

We have an issue open to discuss combining IconButton and Button in #16541

We are leaning towards keeping them separate, but I'll look at this closer to understand the disadvantage.

This comment has been minimized.

Copy link
@drw158

drw158 Aug 2, 2019

Contributor

I'm assuming from the code that this is an issue because Button does not have an icon option. Switching between icon and non-icon buttons for Menu Items is causing technical problems.

Could this be solved by adding an icon option to the Button component, but still keep IconButton a separate component? That way, the unchecked and checked versions of the Menu Item could use just a single component, Button.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 3, 2019

Contributor

To be fair, we already do the opposite here. Not pass the "icon" prop to the IconButton (so it behaves like a Button). If it doesn't make sense to merge the two components into one, I feel we shouldn't do anything specific about it.

@gziolo

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

@afercia, would you mind running some sanity check for the Manage All Reusable Blocks link in the menu. I have issues confirming that it gets announced as link as expected. I outputted its HTML content and it is still an anchor with role menuitem as expected but I can't confirm it with VoiceOVer.

Aside: yes, I still keep it in mind that we should have no links in the dropdown menu :) I need to find a related issue where @mapk proposes we move it to the Manage Blocks modal and tackle it at last.

Regardless, I think we should move forward with this PR given that it makes More Menu finally usable for keyboard users. The last step to make the experience good is to stop closing this menu when opening modals 🎉

@gziolo

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

This PR will also partially resolve #15501. There is a remaining issue with focus loss when switching between Code and Visual editor when the post is empty.

@afercia

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Thanks for working on this! @gziolo I tested a bit and seems to me everything works as expected. Just one small thing: should the small horizontal "jump" of menuitems with the check icon be fixed in this PR? See animated GIF:

jump

Regarding the way screen readers announce Manage All Reusable Blocks: it has a menuitem role so it's announced as a "menu button" (or similar, depending on the browser / screen reader). ARIA roles override the native role.

However, it still creates some issues. For example: in VoiceOver, this menuitem is not listed in the "Form Controls" list while the other ones are:

Screenshot 2019-08-01 at 13 40 40

  • open the More menu
  • press Command Option U to open the VoiceOver "rotor"
  • right / left arrows to go to the "Form Controls" panel
  • down / up arrows to browse the list of form controls
  • see that Manage All Reusable Blocks is missing

For clarity, menuitems can be links. However, an ARIA menu is supposed to be either a menu of actions or a navigation menu. When it's a navigation menu, it should be wrapped in a <nav> element, see the W3C example:

Menu or Menu bar design pattern:
https://www.w3.org/TR/wai-aria-practices-1.1/#menu

Navigation Menubar Example:
https://www.w3.org/TR/wai-aria-practices-1.1/examples/menubar/menubar-1/menubar-1.html

Since the menubar presents a site navigation system, it is wrapped in a navigation region implemented with a nav element that has an aria-label that matches the label on the menubar.

The point is that an ARIA menu can't be a mix of the two things. In this case, all the menuitems perform an action in the current page except Manage All Reusable Blocks, which triggers navigation to another page. This is unexpected for users and there's no way to communicate it properly. See #13390

@gziolo

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Thanks for working on this! @gziolo I tested a bit and seems to me everything works as expected. Just one small thing: should the small horizontal "jump" of menuitems with the check icon be fixed in this PR?

Good catch, it needs to be fixed, it would be a regression.

@noisysocks

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

Thanks for testing all!

The point is that an ARIA menu can't be a mix of the two things. In this case, all the menuitems perform an action in the current page except Manage All Reusable Blocks, which triggers navigation to another page. This is unexpected for users and there's no way to communicate it properly. See #13390

Thanks for going into this. Let's address the issues with Manage All Reusable Blocks as part of #13390. I agree we should remove the menu item altogether.

Just one small thing: should the small horizontal "jump" of menuitems with the check icon be fixed in this PR?

This bug exists in master, so I'll create a separate PR to fix it. I'm keen to get this PR merged as it is blocking some other work.

@noisysocks noisysocks merged commit 5b01c97 into master Aug 2, 2019

1 of 3 checks passed

Filter merged Filter merged
Details
Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@noisysocks noisysocks deleted the remove/popover-on-click-outside branch Aug 2, 2019

@gziolo gziolo added this to the Gutenberg 6.3 milestone Aug 6, 2019

gziolo added a commit that referenced this pull request Aug 29, 2019
Components: Add onFocusOutside replacement to Popover onClickOutside (#…
…14851)

* Components: Add onFocusOutside alternative to Popover onClickOutside

* Components: Refactor Dropdown to use onFocusOutside

* Format Library: Refactor inline link to use onFocusOutside

* MenuItem: Always use an IconButton component so as to avoid focus loss.

Switching between `Button` and `IconButton` causes React to remount the
`MenuItem` component. This causes a focus loss as well as E2E failures.

There's no need to use a `Button` for `MenuItems` that are unselected,
since we can simply pass `icon={ undefined }` to the `IconButton`.
gziolo added a commit that referenced this pull request Aug 29, 2019
Components: Add onFocusOutside replacement to Popover onClickOutside (#…
…14851)

* Components: Add onFocusOutside alternative to Popover onClickOutside

* Components: Refactor Dropdown to use onFocusOutside

* Format Library: Refactor inline link to use onFocusOutside

* MenuItem: Always use an IconButton component so as to avoid focus loss.

Switching between `Button` and `IconButton` causes React to remount the
`MenuItem` component. This causes a focus loss as well as E2E failures.

There's no need to use a `Button` for `MenuItems` that are unselected,
since we can simply pass `icon={ undefined }` to the `IconButton`.
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.