Options in the CircularOptionPicker may be not operable with the keyboard

[afercia]

Description

Working on #65124 surfaced an edge case where the buttons in the color picker may end up in a state where they're not focusable because they all have a tabindex="-1" attribute.

I'm not sure what is the root cause but I see some recent work on the CircularOptionPicker and the underlying Composite components in #64833

It appears to me the UI may end up in a state where the value of the circular option picker option activeId value doesn't match any HTML id attribute in the UI. As such, none of the color buttons is considered 'active'. Worth reminding the 'actice' button is expected to be the only button that is focusable with the Tab key, while navigation to the other buttons works via arrow keys.

Cc @WordPress/gutenberg-components

Three prerequisites to reproduce the bug:

• Seems the bug can be reproduced only with themes that display more than one color palette in the panel:
  • Twenty Twenty-Three: Theme and Default palettes - can reproduce
  • Twenty Twenty-Four: Theme palette - cannot reproduce
  • Twenty Twenty-Five: Theme and Default palettes - can reproduce
• Switch to the branch from Global styles menu: Avoid visible labels and accessible names mismatch #65124 to reproduce.
• Go to the WP admin > Gutenberg > Experiments and enable the 'Color randomizer' experiment.

Aniamted GIF to illustrate; Notice that after randomizing the colors, tabbing through the UI entirely skipcs the color picker buttons:

Step-by-step reproduction instructions

• Set the prerequisites as described above.
• Test with Twenty Twenty-Four.
• Go to Site Editor > Styles > Colors > Edit palette.
• Tab through the UI and observe the color picker buttons are operable with the keyboard.
• Move focus to the Randomize colors button and press Enter.
• At this point, the color buttons in the picker re-render. They get new IDs.
• Tab backwards and forwards through the UI and observe that the color picker buttons are still operable with the keyboard.
• Test with Twenty Twenty-Three or Twenty Twenty-Five.
• Repeat the steps above.
• On the last step, observe that now none of the color picker buttons is operable with the keyboard.

Inspecting the source:

• All the buttons have a tabindex="-1" attribute.
• Expected: the 'active' button to be focusable (not sure whether tabindex should be removed or be tabindex="0").
• In my testing, dumping the value of activeId may return a value that doesn't match any ID attribute in the DOM. for example:
  • The activeId value is components-circular-option-picker-3-75.
  • None of the IDs of the 8 buttons in the screenshot below matches activeId e.g.:

components-circular-option-picker-3-43
components-circular-option-picker-3-45
components-circular-option-picker-3-83
components-circular-option-picker-3-85
components-circular-option-picker-3-87
components-circular-option-picker-3-53
components-circular-option-picker-3-89
components-circular-option-picker-3-57

Worth also noting that the ID values in the HTML are not consecutive. I would expect to be numbers progressively incremented by 1. Instead, they appear to be pretty inconsistent, which makes me think to some timing issue and the ID being genereated randomly across the multiple instances of the color picker.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

• Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

• Yes

[afercia]

Note: seems to me this isn't specific to the presence of the button of the 'Color randomizer' experiment. Rather, it appers to be something related to re-rendering of the color picker buttons which may happen in other scenarios as well.

As such, while this issue surfaced by testing an edge case, it appears to be a more general issue with generating the IDs and the logic to set the activeId in the component.

[tyxla]

Thanks for the thorough instructions @afercia, I couldn't have made it without them! 🙌

I was able to reproduce it, although very inconsistently.

I wonder if we can have a better way to reproduce, ideally in trunk. Without a clear, minimal reproduction case, it feels like a lower-priority edge case.

[mirka]

I have a feeling this is related to the changes made here (cc @ciampo). If so, Diego is working on a fix that may resolve the issue. Otherwise, I have suggested an alternative approach that is more verbose but probably more robust against cases like this.

[afercia]

Thanks for your feedback.

I was able to reproduce it, although very inconsistently.

Yes, reproducing is inconsistent for me as well. Which makes me think it's a timing issue.

[ciampo]

@diegohaz , do you have an estimate of when ariakit/ariakit#4100 may be fixed? It will inform us on whether we should patch on our end, or wait for an ariakit fix.

[diegohaz]

@diegohaz , do you have an estimate of when ariakit/ariakit#4100 may be fixed? It will inform us on whether we should patch on our end, or wait for an ariakit fix.

The fix likely includes a breaking change, so I wouldn't wait for it.

[ciampo]

I have a feeling this is related to the changes made here

After taking a look, I don't think this is the case. That code basically changes the initial active composite item in case an option is marked as selected. This allows the selected item to receive focus when the composite widget it focused.

Instead, this is what I think is happening:

• when randomizing colors, the CircularOptionPicker.Option items are removed from the react tree and replaced with new items;
• the CircularOptionPicker doesn't react to this change, and keeps the previously activeId
• which means that, if there isn't an item that matches the activeId, Composite doesn't find a composite item to focus.
• this also explains why the bug can't be reproduced when focusing the "Base" or "Contrast" palette items, which it can (at least on my machine) be consistently reproduced when focusing the "Primary" or "Secondary" colors. In fact, in ColorPalette, the items are keyed based on color and index: if "base" and "contrast" don't change their color and are always in the same positions, their key won't change, and their id will stay the same even after randomizing colors. On the other hand, primary and secondary colors will change, which causes the key to change, which causes React to create a new instance of the item, which has a new composite ID that doesn't match the stale activeId store in CircularOptionPicker

If my hypothesis is true, the fix would be to add code in CircularOptionPicker to reset the activeId when the active item is removed from its children.

[ciampo]

I put together a quick code sandbox to demonstrate the issue described above. Note that, when the active composite item is removed from the react tree, the active id can become stale, causing Composite to skip focus on its items entirely.

Kapture.2024-09-19.at.11.54.23.mp4

[ciampo]

I tested the solution proposed in the previous message in this other codesandbox, and it seems to work well:

Kapture.2024-09-19.at.12.12.50.mp4

The question is: where should this fix be?

Adding this behaviour directly to Composite could make the component a bit too opinionated.

Alternatively, we could add it to CircularOptionPicker: each CircularOptionPicker.Option would register/deregister on mount/unmount to its parent CircularOptionPicker. And if we detect that the active item is de-registering, we reset the active id to the first item.

[ciampo]

While we should probably work on a fix on our end, at least for the short term, I also opened an issue in Ariakit.