Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(MenuItem2): improve selected appearance and a11y #5432

Merged

Conversation

bvandercar-vt
Copy link
Contributor

Fixes #0000

Checklist

  • [N/A] Includes tests
  • [N/A] Update documentation

allow liProps to be passed to MenuItem so that user can set aria-selected prop.

@adidahiya
Copy link
Contributor

Why is this better than the previous/existing approach?

@bvandercar-vt
Copy link
Contributor Author

bvandercar-vt commented Jul 12, 2022

Why is this better than the previous/existing approach?

So, I previously thought that the active || selected were true if the item was checked-- I didn't realize that it actually means the item is currently highlighted for selection. So here:

image

currently the item in blue would have aria-selected=true, because apparently that's what the active/selected props are for. But what we really need, is for those checked items to all have aria-selected=true. As far as I can tell, there's no way to determine that an item is checked within MenuItem, so aria-selected must be manually passed to the MenuItem, hence the addition of the liProps allowing you to pass that.

Since aria-selected must be manually passed, I added that to the multiSelectExample -- see that implementation for why we need to do it this way

@adidahiya
Copy link
Contributor

adidahiya commented Jul 19, 2022

As far as I can tell, there's no way to determine that an item is checked within MenuItem

You're right. The semantics around active/selected for MenuItems has been messy for a while (there is some documentation about this in the deprecated active prop).

I think we can be smarter about this going forward, make the design of MenuItems more prescriptive, and avoid adding this plumbing of aria-selected to the API. Here's my proposal:

selected prop

MenuItems ought to use "tick" icons to represent selected status when roleStructure="listoption". This would apply to Select2, Suggest2, and MultiSelect2.

For MultiSelect2, this matches the existing behavior we have implemented in the docs example:

image

It would be great if we could do this out of the box, so users don't have to set icon themselves. We'll have to render a blank icon for non-selected items. Here's how it would look with Select2:

image

Users should still be able to override this icon if they choose with the icon prop.

Once you implement this, setting aria-selected is trivial... just take the value of the selected prop.

active prop

active can be used to indicate keyboard focus, like it currently does for @blueprintjs/select components. We should un-deprecate this prop, at least for roleStructure="listoption", since we are now disambiguating it from selected.

image

MenuItems with roleStructure="menuitem"

In the future, I think it would be worth reconsidering selected prop behavior for regular MenuItems as well (so that selected={true} renders a "tick" icon in all contexts), but for now, I think we should preserve the existing behavior to avoid breaking things too much.

@adidahiya adidahiya changed the title fix MultiSelect2 aria-selected prop [core,select] fix(MenuItem): improve selected appearance and a11y Jul 19, 2022
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

Select2 items aren't getting selected={true} or the tick icon yet:

image

Also, I think Suggest2 & Omnibar menu items in the docs should not get tick icons. They behave differently, it doesn't feel necessary to visually show selection status on the items there. They should probably use the default roleStructure="menuitem".

packages/docs-app/src/common/films.tsx Outdated Show resolved Hide resolved
packages/docs-app/src/common/films.tsx Outdated Show resolved Hide resolved
@bvandercar-vt
Copy link
Contributor Author

bvandercar-vt commented Jul 21, 2022

Also, I think Suggest2 & Omnibar menu items in the docs should not get tick icons. They behave differently, it doesn't feel necessary to visually show selection status on the items there. They should probably use the default roleStructure="menuitem".

they should still have roleSctructure="listoption" because they are children of a Menu ul that has role="listbox"-- that is the key function of roleSctructure="listoption", creating the proper role structure for that. If selected isn't passed, they don't receive the checkmark, so all one has to do is not pass selected for those.

Select2 items aren't getting selected={true} or the tick icon yet:

I was thinking that for a Select2, the checkmark isn't really that useful anyways-- you can only select 1 option, and you already see the selected option in the input field, so seeing a check next to that option doesn't really provide much. If you disagree, and you want it in the example, I could implement a new itemRenderer for the select2 example, sure. The consumer could always pass selected in their own itemRenderer if they want a checkmark next to their selected option in Select2, but I was thinking about keeping that out of the example, for the sake of not having to write a new itemRenderer.

@adidahiya
Copy link
Contributor

adidahiya commented Jul 26, 2022

If it's not going to get the tick icon, then there shouldn't be a space here:

image

but... I think I would prefer to add the icon, even if it's potentially redundant with the Select2 target. Some users may not show the selected item text in the target, and the tick icon can be a helpful indicator in those cases.

There's a similar situation with Omnibar and Suggest2... I don't think there should be a blank space on the left since we're not showing tick icons:

image

... this is why I was suggesting changing the roleStructure, but if that's not appropriate markup, then we must figure out an alternative solution that allows us to omit the blank icons for these components... open to ideas.

@bvandercar-vt
Copy link
Contributor Author

bvandercar-vt commented Jul 26, 2022

If it's not going to get the tick icon, then there shouldn't be a space here:

image

but... I think I would prefer to add the icon, even if it's potentially redundant with the Select2 target. Some users may not show the selected item text in the target, and the tick icon can be a helpful indicator in those cases.

There's a similar situation with Omnibar and Suggest2... I don't think there should be a blank space on the left since we're not showing tick icons:

image

... this is why I was suggesting changing the roleStructure, but if that's not appropriate markup, then we must figure out an alternative solution that allows us to omit the blank icons for these components... open to ideas.

idea 1: we add a new roleStructure called maybe, "listoptionCheckSelected" that enables the checkmark/blank icon -- and passing the normal already-existing "listoption" will have no effect on the icon.

idea 2:
passed selected prop value:

  • undefined: no icon
  • false: blank icon
  • true: check icon
    icon prop passed: overrides

idea 3: new boolean prop called checkSelected that works in conjunction with the selected prop. If checkSelected=false and selected=true, aria-selected would be set to true but there would be no effect on the icon.

Which of these do you like best?

@adidahiya
Copy link
Contributor

@bvandercar-vt I like idea 2!

@adidahiya
Copy link
Contributor

adidahiya commented Jul 28, 2022

I've caused some conflicts here with the latest PR I merged #5471. If you don't mind, I'll push an update to your branch to resolve them @bvandercar-vt

@adidahiya adidahiya changed the title [core,select] fix(MenuItem): improve selected appearance and a11y [popover2,select] fix(MenuItem2): improve selected appearance and a11y Jul 28, 2022
@adidahiya adidahiya changed the title [popover2,select] fix(MenuItem2): improve selected appearance and a11y fix(MenuItem2): improve selected appearance and a11y Jul 28, 2022
@adidahiya
Copy link
Contributor

adidahiya commented Jul 28, 2022

I've updated the PR with my suggestions, and an implementation of your idea (2) from your last comment. I decided to port the selected prop semantic changes to MenuItem2 and leave MenuItem as-is, because in the time while this PR was open, I deprecated MenuItem and froze its code.

I think the docs examples are in a good state now:

Select:

2022-07-27 23 57 56

MultiSelect:

2022-07-27 23 59 12

Suggest:

2022-07-28 00 07 56

Omnibar:

2022-07-28 00 08 19

@adidahiya adidahiya merged commit 8dfcb4a into palantir:develop Jul 28, 2022
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.

2 participants