[UEPR-483] Action Menu Tests#452
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?
There was a problem hiding this comment.
fireEvent.keyDown practically seemed to apply some .preventDefault() behavior.
| <> | ||
| <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?
…y' into task/uepr-483-action-menu-tests
KManolov3
left a comment
KManolov3
left a comment
There was a problem hiding this comment.
Minor comments, but overall looks good 🙌
| }); | ||
|
|
||
| test('escape closes menu and returns focus to main button', () => { | ||
| test('arrow_up from first item cycles to last, arrow_down from last cycles to first', async () => { |
There was a problem hiding this comment.
Nitpick: naming - ActionMenu keyboard navigation + arrow_up from first item cycle to last, ... can become ActionMenu keyboard navigation + cycles to last when arrow_up pressed from first item, ...
There was a problem hiding this comment.
Nitpick: also ideally those would be separate testcases as they don't require that much setup/teardown to justify them being combined, but don't mind it staying that way
kbangelov
merged commit e54d895
into
scratchfoundation:release/UEPR-297-accessibility-improvements
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.