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.

Sign up for GitHub

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

Jump to bottom

Replace RovingTabIndex with reakit composite #22489

Closed
wants to merge 12 commits into from
Closed

Replace RovingTabIndex with reakit composite #22489

wants to merge 12 commits into from

Conversation

talldan
Copy link
Contributor

@talldan talldan commented May 20, 2020 •
edited
Loading

Description

Follow up to #19799, related to #22373, see comment thread #19799 (comment) for further info.

Replaces the internally used RovingTabIndex component with the Composite component from Reakit library. Composite is already used by another component to implement a grid, so it seems like a good opportunity to consolidate and rely on the more robust implementation of a library.

Props to @diegohaz for the work on Reakit and the example that this is based on:
https://codesandbox.io/s/reakit-composite-treegrid-sh6uu?file=/src/TreeGrid.js

How has this been tested?

Screenshots

Types of changes

Non-breaking refactor

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

github-actions bot commented May 20, 2020 •
edited
Loading

Size Change: -525 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/a11y/index.js 1.14 kB +2 B (0%)
build/annotations/index.js 3.62 kB +2 B (0%)
build/autop/index.js 2.83 kB -1 B
build/block-directory/index.js 6.93 kB +1 B
build/block-editor/index.js 105 kB -47 B (0%)
build/block-library/index.js 119 kB +1 B
build/block-serialization-spec-parser/index.js 3.1 kB -1 B
build/components/index.js 189 kB -416 B (0%)
build/compose/index.js 9.24 kB +1 B
build/core-data/index.js 11.4 kB -1 B
build/data/index.js 8.43 kB +4 B (0%)
build/date/index.js 5.47 kB +2 B (0%)
build/deprecated/index.js 772 B +1 B
build/edit-post/index.js 302 kB +2 B (0%)
build/editor/index.js 44.6 kB -67 B (0%)
build/element/index.js 4.65 kB -1 B
build/format-library/index.js 7.71 kB -2 B (0%)
build/i18n/index.js 3.56 kB +1 B
build/notices/index.js 1.79 kB -2 B (0%)
build/plugins/index.js 2.56 kB -1 B
build/redux-routine/index.js 2.85 kB -1 B
build/rich-text/index.js 14.8 kB +1 B
build/shortcode/index.js 1.69 kB -2 B (0%)
build/token-list/index.js 1.28 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/api-fetch/index.js 3.4 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 790 B 0 B
build/block-directory/style.css 791 B 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 7.17 kB 0 B
build/block-library/editor.css 7.17 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/style-rtl.css 17.1 kB 0 B
build/components/style.css 17.1 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/index.js 6.63 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 13.8 kB 0 B
build/edit-site/style-rtl.css 5.53 kB 0 B
build/edit-site/style.css 5.53 kB 0 B
build/edit-widgets/index.js 7.87 kB 0 B
build/edit-widgets/style-rtl.css 4.58 kB 0 B
build/edit-widgets/style.css 4.57 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/style-rtl.css 5.06 kB 0 B
build/editor/style.css 5.06 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@talldan talldan force-pushed the replace/roving-tab-index-with-reakit-composite branch from b462d7d to 6a42d58 Compare May 20, 2020 07:52
@talldan
Copy link
Contributor Author

talldan commented May 22, 2020

There are some difficult challenges here.

The block navigation has now evolved to have both RichText and a DropdownMenu components within cells, and both those components currently don't support refs or being focus managed.

DropdownMenu also has some toolbar behavior built-in (down arrow opens the menu), which is problematic in this usage.

@talldan talldan self-assigned this May 22, 2020
@talldan talldan added [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality [Status] In Progress Tracking issues with work in progress labels May 22, 2020
@gziolo gziolo requested a review from diegohaz May 23, 2020 06:38
@talldan talldan force-pushed the replace/roving-tab-index-with-reakit-composite branch from 6a42d58 to 1f474d3 Compare May 25, 2020 06:58
@diegohaz
Copy link
Member

The block navigation has now evolved to have both RichText and a DropdownMenu components within cells, and both those components currently don't support refs or being focus managed.

Is there an issue I can check to learn more about that change on the block navigation?

DropdownMenu also has some toolbar behavior built-in (down arrow opens the menu), which is problematic in this usage.

The WAI-ARIA Authoring Practices has a section about this: https://www.w3.org/TR/wai-aria-practices/#gridNav_inside. But this is really tricky indeed.

For dropdowns, I think it's easier to just disable the arrow down key on them like on this example: https://www.w3.org/TR/wai-aria-practices/examples/grid/dataGrids.html#ex2_label

@diegohaz
Copy link
Member

diegohaz commented May 25, 2020 •
edited
Loading

By the way, DropdownMenu doesn't need to accept ref to work with this. You can pass ref to the toggleProps prop and it will be passed to the toggle button. I've updated the CodeSandbox with that: https://codesandbox.io/s/reakit-composite-treegrid-sh6uu?file=/src/App.js:691-935 (it still needs to disable the arrow down key though).

@talldan
Copy link
Contributor Author

talldan commented May 27, 2020

@diegohaz Thanks for your feedback so far, that's been really helpful.

I think I'm going to park this PR for the time being as the Block Navigation UI is in some pretty rapid development at the moment and I can't keep up 😄

One thing I did spot, and I wasn't sure whether it was an issue with this PR or something upstream with Reakit, is that when re-ordering rows in the table (e.g. when moving blocks), the keyboard navigation order became incorrect. It felt like there might be a cached version of the expected order of elements held internally.

For example if I have blocks A, B, and C, arrowing through the rows works as expected, but then reordering to A, C, B, keyboard navigation would go from A to B to C still.

@diegohaz
Copy link
Member

One thing I did spot, and I wasn't sure whether it was an issue with this PR or something upstream with Reakit, is that when re-ordering rows in the table (e.g. when moving blocks), the keyboard navigation order became incorrect. It felt like there might be a cached version of the expected order of elements held internally.

For example if I have blocks A, B, and C, arrowing through the rows works as expected, but then reordering to A, C, B, keyboard navigation would go from A to B to C still.

Oh, good catch! I haven't looked into it yet, but I'm pretty sure that's a limitation on Reakit. For performance reasons, composite.items will be updated only when items mount/unmount. So, re-mounting the item could work, but it's not a good solution here as we would lose focus and internal state on the moving item. 😕

I'll make sure this is supported in the next version.

@talldan talldan added [Status] Blocked Used to indicate that a current effort isn't able to move forward and removed [Status] In Progress Tracking issues with work in progress labels Jun 4, 2020
@diegohaz
Copy link
Member

diegohaz commented Jun 4, 2020

FYI, Reakit v1.1.0 will remove the unstable_ prefix from the Composite components and introduce a composite.unstable_sort() method that can be used after composite items are re-ordered.

@diegohaz diegohaz mentioned this pull request Jul 2, 2020
Merged
Base automatically changed from master to trunk March 1, 2021 15:43
@talldan talldan closed this Jul 19, 2022
@talldan talldan deleted the replace/roving-tab-index-with-reakit-composite branch July 19, 2022 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diegohaz diegohaz Awaiting requested review from diegohaz

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra will be requested when the pull request is marked ready for review ajitbohra is a code owner

@ellatrix ellatrix Awaiting requested review from ellatrix ellatrix will be requested when the pull request is marked ready for review ellatrix is a code owner

Assignees

@talldan talldan

Labels
[Package] Components /packages/components [Status] Blocked Used to indicate that a current effort isn't able to move forward [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@talldan @diegohaz