Menu: add additional keydown events and tests

[roomman]

This PR adds some additional <Menu> keydown interactions, along with updated tests. A few notes / questions:

• The majority of changes are to menu-test.js. I went through the headless ui docs and added any missing tests for Space, Escape, End, and Tab. I've also tried to standardise the commentary a bit, so it's easier for others to jump.

• The introduction of Space events created issues with the search feature so I've added a condition to onKeyDown in menu/items, to check if the search task is running or not, and to handle event.key accordingly. This required an additional argument (searchTaskIsRunning) to be yielded down from menu.

• While cross referencing headlessui tests, I noticed that we are missing a default behaviour for this component: on close, focus should return to menu/button. I’ve added something to the close action in menu, to return focus. For now, it includes a condition to check if the button element exists prior to calling focus. This is because there are some tests that either don’t have a button (see it should not focus button when does not exist) or have a button that’s not a menu/button (see controlling open/close programmatically). TBH, I’m a bit unclear why we’d have a menu without rendering menu/button so if people (@far-fetched, @alexlafroscia - ps hope you enjoyed the honeymoon!) can feedback I can refactor as needed?

• I added the data-test-headlessui-menu-button attribute to menu/button to enable the new getMenuButton() to follow the same query selector pattern as headlessui. It did occur to me that we could do the same for menu/items (ie data-test-headlessui-menu-items) rather than hardcoding data-test-menu-button and data-test-menu-items into every test? If you are happy with this suggestion I can add a further commit to this, or follow up with a new PR.

• For consistency with <Dialog> and <ListBox>, should we move assertions into accessibility-assertions? Again, very happy to commit again, or PR if preferred.

Thanks 🙏🏼

[roomman]

@far-fetched the latest commits removed the focus action, and pass setReturnFocus into menu/items, so we can use the focus-trap api as your suggested.

I've not implemented an Escape key handler as it makes sense to delegate this to focus-trap, as it already calls onClose when it deactivates.

In the end, the two failing tests were a simple fix: because we are using keys to close the menu, I needed to place focus on the button so focus-trap knew where to return it.

I've added a test to check setReturnFocus hands focus to an alternate element.

[far-fetched]

Here is existing react docs for menu you can check public API for menu items component. We should be really strict (as long as possible) and keep the same public api. Now we expose something really specific like @setReturnFocus parameter.
I think that proper behavior would be to make this behavior generic and encapsulate it inside menu itself - without any additional configuration (@setReturnFocus) needed.
So basically we want this test green but without providing anything into @setReturnFocus from template. Let's make it generic that if I add another button I will not have to update @setReturnFocus.

[roomman]

Sorry, I think I misunderstood your previous. I agree about sticking the to public API, hence the comment about deviation from the React/Vue implementation.

Now I am clear, I think we can just remove setReturnFocus and let the focus-trap defaults do their thing. Right?

[far-fetched]

Now I am clear, I think we can just remove setReturnFocus and let the focus-trap defaults do their thing. Right?

Probably we need some code for some tests.
I guess that this test can be satisfied with default behavior which focus-trap provides (return focus to trigger element).
Probably this one requires some extra logic - my first idea was leverage setReturnFocus maybe I am wrong - it requires some investigation. I have plan to implement the same behavior for dialog during following weekend I don't have precise solution right now, of course you can start tinkering with menu here ;)

[roomman]

@far-fetched, latest commit removed setReturnFocus and depends on focus-trap default behaviour. Making this draft so that, assuming you're okay with the change, I can squash it before submitting for review. 👍🏻

[alexlafroscia]

Thanks for the PR, and for your patience until I returned!

should we move assertions into accessibility-assertions

Let's leave it alone for right now; I would actually like us to move the test helpers to the addon-test-support directory so that they are grouped together in a location that makes sense for testing and can be consumed by users of this addon in their own tests. That's a refactor that I would love help with, but should be a separate effort. I'll write up a card for it!

Overall the changes look good, but we should drop the package.json changes and do that separately if they're needed at all.

The data-test- selector bit is a personal preference, but one that I think this addon should establish a firm stance on. IMO addons shouldn't include stuff like that in the code they ship to users; what data-test- attributes the end-user wants is their business, and I don't love the idea of polluting the element with additional stuff like that. As soon as we include those attribute in the addon tree, they are public API and removing them is a breaking change. If we were, for example, to move toward using accessibility roles to query elements for testing purposes, we now need to do a semver major bump because we want to refactor our testing infrastructure, which doesn't feel great. It does mean that we need to add data-test- attributes explicitly in our tests, but lines of code are cheap and it allows our tests to be more explicit, which is a good thing!

[roomman]

Thanks for the PR, and for your patience until I returned!
Not at all @alexlafroscia, I hope you have a fantastic honeymoon. Many congratulations!

That's a refactor that I would love help with, but should be a separate effort. I'll write up a card for it!
Happy to help with refactoring. There are distinct syntactical differences between tests in this add-on. It would be good to agree a 'house style' so there's a bit more consistency. 👍🏻

The data-test- selector bit is a personal preference
Useful feedback on this, thanks. I will refactor the attribute out of the template and add it to the test instead. Update to this PR to follow.

• Refactor data-test attribute into test and resubmit for review

Were these package.json changes introduced by accident?
I merged the master branch back into the PR prior to submitting, to incorporate a bug fix. How would you like me to handle that?

[alexlafroscia]

I hope you have a fantastic honeymoon. Many congratulations!

Thank you! It was just the break I needed to shake off some burn-out and come back refreshed.

Happy to help with refactoring. There are distinct syntactical differences between tests in this add-on. It would be good to agree a 'house style' so there's a bit more consistency. 👍🏻

Yup, this is absolutely something that we need to address; the tests are different depending on who wrote them 😅 I opened #107 to facilitate a discussion about it and we'll see where things land

I merged the master branch back into the PR prior to submitting, to incorporate a bug fix. How would you like me to handle that?

Hmm... that's strange. Browsing by commits on this PR, I just see a single commit with changes to the package.json included alongside everything else.

I would recommend preforming a rebase where you re-create the commit off of the current state of master and drop any changes to the package.json file. If you need a hand with that (since the git-foo can be kind of tricky), I should be able to do that for you and push the commit back to this branch for you to continue iterating on from there!

[roomman]

Browsing by commits on this PR, I just see a single commit with changes to the package.json included alongside everything else.

Yeah, that's because I performed a rebase after merging master into this. I'll have another crack at it and shout if I get stuck. 👍🏻

[alexlafroscia]

To clarify the intended behavior: Space will select an item, unless we're currently searching for an item to activate?

Is that the expected behavior from an accessibility perspective?

[roomman]

Yes, that's the expected behaviour. I'm not 100% certain about accessibility, because I was unable to find examples that included search functionality.

This implementation tries to mirror that in the Vue package of headlessui, for the keydown on a menu-item:

...
// @ts-expect-error Fallthrough is expected here
        case Keys.Space:
          if (api.searchQuery.value !== '') {
            event.preventDefault()
            event.stopPropagation()
            return api.search(event.key)
          }
...

[GavinJoyce]

@alexlafroscia @roomman I've been following this PR from the outside, but I haven't dug into any of the detail.

Are there outstanding items to address before merging this?

[roomman]

@GavinJoyce, scrap my last. I need to do a small refactor to remove a "data-test" attribute from one template and drop it into a test instead. I will make sure that's done early next week. Sorry for my hasty, initial reply.

[GavinJoyce]

No worries, thanks so much for the PR

[GavinJoyce]

FYI, if you rebase the tests should pass

[roomman]

@GavinJoyce sorry this PR has ended up being a bit scrappy for the git log but hopefully it's okay to merge, at last! 🤞🏻

[GavinJoyce]

Thanks @roomman! 🍻