Skip to content

fix: only add aria-activedescendant when menu is open#926

Closed
stefanprobst wants to merge 4 commits intodownshift-js:masterfrom
stefanprobst:fix/aria-activedescendant
Closed

fix: only add aria-activedescendant when menu is open#926
stefanprobst wants to merge 4 commits intodownshift-js:masterfrom
stefanprobst:fix/aria-activedescendant

Conversation

@stefanprobst
Copy link
Copy Markdown

What: in useSelect/useCombobox only add aria-activedescendant attribute when menu is open to avoid pointing to non-existing id.

Why: Fixes #921

How: check if isOpen is true before adding aria-activedescendant.

Checklist:

  • Documentation
  • Tests
  • TypeScript Types
  • Flow Types
  • Ready to be merged

Copy link
Copy Markdown
Collaborator

@silviuaavram silviuaavram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also run npm run validate and commit the .size-snapshot please. Thank you!

)

fireEvent.click(toggleButton)
// fireEvent.blur(input)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure -- comment was just for symmetry with the comments above

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean also the line below, I don't see why is it needed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the input.blur() is needed actually (or toggleButton.focus() alternatively). i had assumed it was just an oversight this test did not have that line while e.g. the one above did.
without manually blurring/focusing the actions fire in wrong order:

without manual blur:

  • __togglebutton_click__
  • __togglebutton_click__
  • __togglebutton_click__
  • __input_blur__

with:

  • __togglebutton_click__
  • __togglebutton_click__
  • __input_blur__
  • __togglebutton_click__

@silviuaavram
Copy link
Copy Markdown
Collaborator

As I merged a big change as part of v5 I re-created this PR. Ran the tests without the .blur in the discussion and they were passing. Please take a look at #945

@stefanprobst stefanprobst deleted the fix/aria-activedescendant branch February 17, 2020 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[useSelect] aria-activedescendant does not have valid value with controlled selectedItem

2 participants