[UEPR-483] Action Menu Tests#452
[UEPR-483] Action Menu Tests#452kbangelov wants to merge 23 commits intoscratchfoundation:release/UEPR-297-accessibility-improvementsfrom
Conversation
| expect(document.activeElement).toBe(mainButton); | ||
| }); | ||
|
|
||
| test('tab closes menu and focuses next element', async () => { |
There was a problem hiding this comment.
Maybe we can add a test for shift + tab as well
There was a problem hiding this comment.
Though I did add one, it kept failing if I entered the menu before shift + tab-ing and I couldn't figure out why. Perhaps it's some difference between the user testing library and the real thing. Not sure if it's ok to leave it like this.
There was a problem hiding this comment.
I see that you ended up adding a test for that. Did it start working consistently?
There was a problem hiding this comment.
The one I left, yes. If we add arrow_down to get inside the menu it doesn't perform correctly. Since there were some changes to the base branch since then, I will wait for that one to get merged first and then try again with this test (and check the other ones too).
| test('focus + arrow_down opens menu and arrow_up cycles to last', () => { | ||
| render(<ActionMenu {...defaultProps} />); | ||
| const mainButton = screen.getByRole('button', {name: 'Main Button'}); | ||
|
|
||
| act(() => { | ||
| mainButton.focus(); | ||
| fireEvent.keyDown(mainButton, {key: KEY.ARROW_DOWN}); |
There was a problem hiding this comment.
nitpick: I think the this test case in the way that it's described and written is a bit misleading. Maybe we could do something like:
- Focus opens menu and select the last element by default
- Arrow down on last element cycles to the first
- Arrow up on the first element cycles to the last
|
|
||
| act(() => { | ||
| mainButton.focus(); | ||
| fireEvent.keyDown(mainButton, {key: KEY.ARROW_DOWN}); |
There was a problem hiding this comment.
Why do we need ARROW_DOWN to open the menu? Is it just so that we'd go to the first element? I wonder if that's relevant to the current test case, maybe it'd make more sense to keep it simple and avoid doing extra navigations? Same question for the other test cases where we do it.
| act(() => { | ||
| user.tab(); | ||
| }); |
There was a problem hiding this comment.
Out of curiosity, what's the difference between calling tab() on a user event and firing KEY.TAB?
| <> | ||
| <button>Before Menu</button> | ||
| <ActionMenu {...defaultProps} /> | ||
| <button>After Menu</button> |
There was a problem hiding this comment.
We probably don't need the after menu in this layout, since we never use it?
3 participants
Resolves
Part of https://scratchfoundation.atlassian.net/browse/UEPR-483
Proposed Changes
3 Unit tests to test keyboard navigation.
Reason for Changes
Part of Accessibility initiative for Scratch.