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

feat: add context-menu #7350

Merged
merged 60 commits into from
Mar 10, 2021
Merged

Conversation

janhassel
Copy link
Member

@janhassel janhassel commented Nov 30, 2020

Closes #6830

Changelog

New

  • Adds the following react components:
    • ContextMenu
    • ContextMenuOption
    • ContextMenuDivider
    • SelectableContextMenuOption
    • ContextMenuRadioGroup

@netlify
Copy link

netlify bot commented Nov 30, 2020

Deploy preview for carbon-elements ready!

Built with commit fdb5d39

https://deploy-preview-7350--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Nov 30, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit fdb5d39

https://deploy-preview-7350--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Nov 30, 2020

Deploy preview for carbon-elements ready!

Built with commit 4ca8433

https://deploy-preview-7350--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Nov 30, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 8112a4a

https://deploy-preview-7350--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Nov 30, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 4ca8433

https://deploy-preview-7350--carbon-components-react.netlify.app

@janhassel
Copy link
Member Author

janhassel commented Feb 23, 2021

@carmacleod

aria-haspopup on the "Share with" menuitem keeps getting set to aria-haspopup="[object Object]"

✅ Fixed

the "Share with" menuitem li needs to have aria-expanded="true" when the menu is open (and "false" when it is closed).

✅ Done

The submenu ul should have aria-label="Share with"

💬 Let me know if I misunderstand, but the ul has an aria-label on my side. Maybe this is fixed with the new markup for top-level groups (see end of list)
image

Regarding title, can you only add it when there has been truncation?

💬 I would really like to avoid this as it adds a good amount of extra logic to determine in JavaScript whether or not a truncation will happen (which in turn is done by CSS though). I think this can be a point we can bring up at a later stage. The component will still be marked as unstable/experimental for a bit.

multiple groups are allowed, but each one needs a role="group" aria-label="Group name"

✅ Done

if all of the menuitems in a menu belong to the same group...
if, however, there are multiple groups in the same menu...

✅ Done

Do you know if native dropdowns also wrap on windows?

Yep, they sure do.

💬 Good to know! Do Carbon dropdowns wrap on Windows as well? I think we should follow this consistently.

You probably want to support Space as well, which is more familiar to Mac users

✅ Done

Base automatically changed from master to main March 8, 2021 16:35
@carmacleod
Copy link
Contributor

@janhassel Thanks for the really fast fixes! Sorry to take so long to get back to you. This is looking really good now!

I did notice something odd in the example: After the context menu has been opened and closed, the user needs to right-click twice to pop up the menu again. Or Shift+F10 twice. Or ctrl+left click (on Mac) twice.

The submenu ul should have aria-label="Share with"

Let me know if I misunderstand, but the ul has an aria-label on my side. Maybe this is fixed with the new markup for top-level groups

Yes, you fixed it with the new markup for top-level groups. :)

Regarding title, can you only add it when there has been truncation?

💬 I would really like to avoid this as it adds a good amount of extra logic ...

#7768 might already have some of that extra logic?
Let me know if you prefer that I open a new issue to look at this later.

Do you know if native dropdowns also wrap on windows?

Yep, they sure do.

💬 Good to know! Do Carbon dropdowns wrap on Windows as well? I think we should follow this consistently.

Good question. It looks like Combobox does, and Dropdown doesn't. Do people use Dropdown to create menus in Carbon?
I notice that OverflowMenu does wrap. How should we proceed with this? New issue to discuss what to do across components?

💬 I used these roles in the beginning but reverted them back to checkbox and radio as menuitemcheckbox and menuitemradio are not properly communicated by VoiceOver, instead they both get read as "menu item".
I pushed an update, changing them back to to the menuitem* roles. Let me know what you think and if we should go with the spec or with what has better support right now (also let me know how JAWS reads it if you have access to a windows machine).

I think we have to go with the spec, because otherwise we will mess up the Windows screen readers. VO seems to have some strange quirks in this area, like saying "1" when a radio or checkbox menuitem is checked. Those will get fixed in time. The ARIA-AT project is working towards more consistency of AT behavior across platforms.

Just for info, here's what NVDA says now (in Chrome) for the radio and checkbox menuitems. For the radio menu items, it says "radio menu item", and it says both "checked" and "not checked". Note that it doesn't say "checkbox" menuitem, but it does say both checked and not checked.

menu
Share with  subMenu  1 of 9
expanded
None  radio menu item  not checked  1 of 4
Share with  menu
Product team  radio menu item  checked  2 of 4
...
Publish  checked  7 of 9
space
...
Publish  not checked  7 of 9

Here's what JAWS says (in Chrome) for those items (ignore the weird whitespace characters). Note that JAWS doesn't say radio or checkbox. For checkboxes, it says both "checked" and "not checked", but for radios, it only says "checked" and it doesn't say "not checked".

menu� 
To navigate press Up or Down Arrow.
Share with �sub menu� 1 of 9
None 1 of 4
Product team �checked� 2 of 4
...
Publish �not checked� 7 of 9
Leaving menus
...
Publish �checked� 7 of 9

@lorelei-ngooi
Copy link

Is there any ETA on when this would be available?

@janhassel
Copy link
Member Author

janhassel commented Mar 9, 2021

@carmacleod

After the context menu has been opened and closed, the user needs to right-click twice to pop up the menu again

✅ Good catch, should be fixed now

Regarding title, can you only add it when there has been truncation?

Let me know if you prefer that I open a new issue to look at this later.

💬 Yes, please open an issue for that once this PR is merged in

Good question. It looks like Combobox does, and Dropdown doesn't. Do people use Dropdown to create menus in Carbon?
I notice that OverflowMenu does wrap. How should we proceed with this? New issue to discuss what to do across components?

💬 Let's definitely open a serparate issue for this one. Personally, I think these should all behave the same, but a member of the Carbon team needs to make a call and then it should be applied to the individual components.

Just for info, here's what NVDA says now [...]

💬 Thanks, this is really helpful! Let's hope VoiceOver, JAWS and co will adopt these newer roles soon.


@emyarod I believe this means the PR is ready for review again. Let's bring this in as experimental / unstable soon and work on further concerns and ideas in separate, smaller (and more managable) PRs.
Let me know if you have any questions or if you wanna schedule a quick chat!

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

yes I have been following this after I approved the first time, just requested a review from myself while awaiting @carmacleod 's reviews. regarding the menu selection wrap behavior, I think the context menu should also wrap selection but that can be addressed separately

@janhassel
Copy link
Member Author

@emyarod Awesome. Yes, let's handle the wrapping in a separate issue.

Copy link
Contributor

@carmacleod carmacleod left a comment

Choose a reason for hiding this comment

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

+1 - good to go!

@kodiakhq kodiakhq bot merged commit 59bfae0 into carbon-design-system:main Mar 10, 2021
@janhassel janhassel deleted the context-menu branch March 11, 2021 19:54
@carmacleod
Copy link
Contributor

A bit of follow-up housekeeping:

@janhassel
Copy link
Member Author

Thank you, @carmacleod! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component proposal: Context menu
7 participants