Fix AlignmentMatrixControl focus issue#27945
Conversation
diegohaz
left a comment
There was a problem hiding this comment.
Thanks, @rmorse!
With #27699, the useFocusOnMount hook — the popover uses that — has been changed so it looks for tabbable elements right away when the popover element is mounted. At this point, in the very first render, all the composite items have tabIndex="0" (in a second render, only the active item will have tabIndex="0"). That's why the first item is receiving focus even when it's not the active item.
To keep the current behavior (reflecting the change on the UI when navigating with the keyboard), I suggest we force the tabIndex value instead of using onClick.
|
@diegohaz - ah yes that makes better sense :) thanks! I've tested and works as expected, fixing the intial issue without compromising on the keyboard functionality. |
Co-authored-by: Haz <hazdiego@gmail.com>
diegohaz
left a comment
There was a problem hiding this comment.
This looks good! Thanks! The failing test is not related to this change and I think was fixed on master already.
|
@rmorse Can you please merge master into your branch? |
|
@diegohaz done :) |
Description
This fixes a bug in the AlignmentMatrixControl where simply rendering a control triggers a focus event which in turn triggers a change event / state change.
To replicate
After digging around I realised this was to do with the fact the
onChangeevent is triggered byonFocusof thecell.I think due to the way the reakit components work (I'm not too familiar), when rendering, the first element is focussed immediately/automatically - in this case
top left.Looking at their docs, I see examples of a
CompositeItemusingonClick- so I tried with that and it works.What this does "break" is:
Personally, I think this still feels natural, but I think it may need a bit of a rethink in terms of UI - perhaps, we have a "focussed" color state for when the keyboard is in the process of selecting an alignment, and when it is selected, we get the larger, black square we currently have.
How has this been tested?
By opening the control an observing behaviour to mouse + keyboard events
Screenshots
Before:
After:
Types of changes
This is a 1 line bug fix that will only have a small negative effect for keyboard users - and fixes a bug that causes a state change whenever the component is rendered.
Checklist: