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

Add a useFocusOutside React hook #27549

Closed
wants to merge 3 commits into from
Closed

Conversation

youknowriad
Copy link
Contributor

Follow-up to #27502
Similar to #27544

This refactors the existing withConstrainedTabbing HoC as a React hook to avoid the extra wrapper.
The goal here is to be able to remove the PopoverWrapper component that is duplicated across three packages and reuse a single hook.

@youknowriad youknowriad added the [Type] New API New API to be used by plugin developers or package users. label Dec 7, 2020
@youknowriad youknowriad self-assigned this Dec 7, 2020
@ellatrix
Copy link
Member

ellatrix commented Dec 7, 2020

I already had a PR for this: #27050

@youknowriad
Copy link
Contributor Author

I'm sorry I missed your PR. I think though I prefer the current one for a few reasons:

  • The hook being in @wordpress/compose
  • Avoid the ref as argument pattern which doesn't allow to refresh the behavior if the container changes.

@ellatrix
Copy link
Member

ellatrix commented Dec 7, 2020

Yes, my PR wasn't ready at all, it was just an exploration. I had problems making the unit tests pass.

@github-actions
Copy link

github-actions bot commented Dec 7, 2020

Size Change: +20.4 kB (1%)

Total Size: 1.21 MB

Filename Size Change
build/a11y/index.js 1.14 kB +2 B (0%)
build/annotations/index.js 3.8 kB +5 B (0%)
build/autop/index.js 2.83 kB -1 B
build/block-directory/index.js 8.72 kB -2 B (0%)
build/block-editor/index.js 128 kB -16 B (0%)
build/block-library/index.js 149 kB -10 B (0%)
build/block-library/style-rtl.css 8.34 kB +7 B (0%)
build/block-library/style.css 8.34 kB +7 B (0%)
build/block-serialization-default-parser/index.js 1.88 kB -1 B
build/blocks/index.js 48.1 kB -2 B (0%)
build/components/index.js 171 kB -659 B (0%)
build/compose/index.js 10.4 kB +413 B (3%)
build/core-data/index.js 15.3 kB +1 B
build/data/index.js 8.97 kB -11 B (0%)
build/date/index.js 31.8 kB +20.6 kB (64%) 🆘
build/deprecated/index.js 769 B +1 B
build/dom/index.js 4.95 kB -1 B
build/edit-navigation/index.js 11.1 kB -3 B (0%)
build/edit-post/index.js 306 kB +8 B (0%)
build/edit-site/index.js 24.7 kB +6 B (0%)
build/edit-widgets/index.js 26.3 kB -12 B (0%)
build/editor/index.js 43.6 kB -12 B (0%)
build/element/index.js 4.62 kB -2 B (0%)
build/format-library/index.js 6.74 kB +2 B (0%)
build/hooks/index.js 2.27 kB +2 B (0%)
build/html-entities/index.js 622 B -1 B
build/keycodes/index.js 1.93 kB -1 B
build/list-reusable-blocks/index.js 3.1 kB +3 B (0%)
build/media-utils/index.js 5.32 kB +2 B (0%)
build/notices/index.js 1.82 kB -1 B
build/nux/index.js 3.42 kB -1 B
build/plugins/index.js 2.56 kB +3 B (0%)
build/primitives/index.js 1.43 kB +1 B
build/redux-routine/index.js 2.84 kB -1 B
build/reusable-blocks/index.js 2.92 kB -3 B (0%)
build/rich-text/index.js 13.4 kB +2 B (0%)
build/server-side-render/index.js 2.77 kB +2 B (0%)
build/shortcode/index.js 1.69 kB +1 B
build/token-list/index.js 1.27 kB +1 B
build/url/index.js 2.84 kB -1 B
build/viewport/index.js 1.86 kB -2 B (0%)
build/wordcount/index.js 1.22 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/api-fetch/index.js 3.42 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11.2 kB 0 B
build/block-editor/style.css 11.2 kB 0 B
build/block-library/editor-rtl.css 9.07 kB 0 B
build/block-library/editor.css 9.07 kB 0 B
build/block-library/theme-rtl.css 789 B 0 B
build/block-library/theme.css 790 B 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/data-controls/index.js 827 B 0 B
build/dom-ready/index.js 571 B 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/style-rtl.css 6.49 kB 0 B
build/edit-post/style.css 6.47 kB 0 B
build/edit-site/style-rtl.css 3.93 kB 0 B
build/edit-site/style.css 3.93 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.84 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/priority-queue/index.js 789 B 0 B
build/warning/index.js 1.14 kB 0 B

compressed-size-action

@talldan
Copy link
Contributor

talldan commented Dec 8, 2020

I already had a PR for this: #27050

Oops, me too 😄 #27369

I avoided DOM event handlers (event though using a ref is a nicer pattern), as I don't think that'll work for nested modals/popovers, which rely on virtual bubbling of events. But maybe there's another way to handle this. Returning the ref is definitely a nicer API.

The easiest way to test this that I've found is to use the 'Display Button Labels' option with a smaller screen size and the try accessing the nested popovers in the Tools menu (Outline or Details).

@youknowriad
Copy link
Contributor Author

I avoided DOM event handlers (event though using a ref is a nicer pattern), as I don't think that'll work for nested modals/popovers, which rely on virtual bubbling of events

I'm on the fence. Logically speaking it seems we want to avoid React virtual bubbling here because technically speaking this is an a11y behavior that rely on DOM and doesn't know anything about slots. That said it goes against "nested popovers" or "nested modals". I wonder if it's a use-case we want to support and whether it's acceptable to just close the parent.

Let's go with your approach for now.

@ellatrix
Copy link
Member

ellatrix commented Dec 8, 2020

@talldan How would you merge multiple event handlers? The implementor has to merge all the props?

Personally I think I'd prefer DOM event handlers.

@youknowriad
Copy link
Contributor Author

I'm going to close this for now, I also prefer DOM event handlers but I can't see how we can solve the nested modals/popovers issue with them.

@youknowriad youknowriad closed this Dec 8, 2020
@youknowriad youknowriad deleted the add/use-focus-outside branch December 8, 2020 08:11
@talldan
Copy link
Contributor

talldan commented Dec 8, 2020

@ellatrix There was a bit of discussion about the event handlers here with a few different ideas - #27369 (comment).

@youknowriad I do prefer returning ref in terms of the usability of the hook, but was concerned about back compat. Particularly as it's quite an intricate (at least to me 😄) little HOC.

I don't know if we could try something using context to get a similar bubbling behavior, but still use DOM event handlers. The way withFocusOutside works seems to mostly depend on cancelBlurCheck being called when a legit focus happens to a child, so perhaps that could be propagated up using context?

I'm imagining something where context consumers can register themselves and then also trigger a cancelBlurCheck. Seems a bit like an event/emitter pattern coincidentally.

@youknowriad
Copy link
Contributor Author

@talldan worth trying, let's land your first implementation as experimental and see whether we can do this separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants