carbon-design-system / ibm-products

A Carbon-powered React component library for IBM Products
https://ibm-products.carbondesignsystem.com
Apache License 2.0
92 stars 136 forks source link

Datagrid design review: Toolbar #3117

Closed elycheea closed 1 year ago

elycheea commented 1 year ago

Design review


Standards

Pattern and behavior

Storybook

elycheea commented 1 year ago

@Ashley-Bock can you take this one?

elycheea commented 1 year ago

@kristastarr Feel free to remove any checks that don’t apply.


Accessibility review

Keyboard

Level 1 - see comments for items needing further attention

Level 2

Dynamic updates

Level 2

Text and non-text

Level 3

Ashley-Bock commented 1 year ago

1. Pop-out styling There are discrepancies between v1 and v2 in terms of the pop-out styling. The v1 better matches the specs in the PAL guidance images, but I'm not sure if the change in v2 was intentional. See below:

v2:

Screenshot 2023-06-14 at 3 17 50 PM

v1:

Screenshot 2023-06-14 at 3 18 26 PM

2. Order of sizes listed In the examples in the PAL guidance, the sizes are listed smallest to largest. In both v1 and v2, they are listed largest to smallest.

v2 example:

Screenshot 2023-06-14 at 3 21 25 PM

PAL guidance image

3. Toolbar height In the guidance, it says that the toolbar height should change as the user adjusts the row height. Neither v1 or v2 demonstrate this in the storybook.

Ashley-Bock commented 1 year ago

Standards

Pattern and behavior

Storybook

elycheea commented 1 year ago
  1. Pop-out styling @matthewgallo I think we changed this to Toggletip to avoid the redundant tooltip. https://github.com/carbon-design-system/ibm-products/pull/2873 Since our product teams provide their own DatagridActions, it might be good to have a good recommendation on Toggletip, OverflowMenu, or other Popover styles? 🤔 @Ashley-Bock I think this one is specific to the row size design review, but for the consistency of the Toolbar, may be a good topic to bring up? v2/Carbon 11 allows use of the newer Toggletip and Popover components, but v1/Carbon 10 only have OverflowMenu.

  2. Order of sizes I think this one is actually configurable by the users of the component so I think this is okay?

  3. Toolbar height @matthewgallo is this something we changed intentionally, something users have control over instead of us, or something that was missed. I could imagine this being a little bit tricky for some of the smaller sizes.

elycheea commented 1 year ago

@Ashley-Bock @matthewgallo So I went looking to figure out whether Toggletip or Popover makes more sense in this context. Then I stumbled upon #2527 which @AlexanderMelox and I happened to find a while back.

I think anything that drops down from the toolbar should be considered a “disclosure^1.

Settings and filters follow the disclosure pattern by clicking an icon button that opens a popover. Interactive elements are often found in settings and filters in order to modify or adjust specific content. The disclosure pattern allows you to nest interactive elements inside of a popover while still being accessible.

So based on this, I think the original design with the Popover is most likely the way to go for now. @Ashley-Bock not sure if we need to add some context/guidance on the disclosure pattern in the PAL guidance, but I don’t think we have a dedicated Toolbar page anyway.

) via Carbon Design System.

kristastarr commented 1 year ago

Some comments about keyboard accessibility - mostly related to Settings. I tried to capture in screen recording.

  1. In most cases, when you tab from Download button to Settings button, you are able to press space/enter to open the Settings popover correctly. However, sometimes when you tab to it, only a faint 1px blue outline appears around the button (which does not match the rest of the focus states), and then nothing happens when you press space, and focus is lost. After pressing tab twice, focus resumes on the settings button. I’ve tried to determine what pattern of actions causes it to work vs not work. I think it might be alternating between correct and incorrect, but I’m not 100% sure.

  2. When you are able to correctly open the dropdown, you must press tab again - and the faint 1px blue focus state appears around settings, - then press tab a second time to enter the list before you are able to press up/down/L/R to navigate the list. Focus should be present in the first list item after pressing space/enter to activate the popover. Also as you press up/down arrows to cycle through the radio button options, the table itself moves up and down behind it.

  3. After using up and down to move focus to your selected radio button, pressing tab moves you to the “Primary button” but wondering if it would be better to press enter to make a selection, which would move focus to the settings button, then use tab to move to the primary button. I’m looking for some examples of best practices to reference.

  4. When popover is open and you tab to settings, it closes. However, if popover is open and you shift+tab to Download before interacting with the popover content, the popover remains open and overlaps with the Download tooltip. I’m thinking it should close so that what happens with shift+tab is consistent with what happens with tab.
    Screenshot 2023-07-03 at 8 16 05 AM

  5. When open, the popover should dismiss when Esc is pressed

  6. When Primary button has focus, and you press tab, focus is unknown, and then if you press tab again it lands on the next interactive element (the “Create new” button), instead of going directly to the Create new button. I think the dropdown for "Primary button" is a tab stop even when it is not displayed.

  7. Visual things: The “Create new” text is not centered vertically in the button The focus state on “View documentation” looks weird since there is lots of white space on the right of the text as the focus indicator takes up the entire width of the “empty-state__content” container Screenshot 2023-07-03 at 8 16 23 AM

https://github.com/carbon-design-system/ibm-products/assets/21047571/942acf3e-053f-4c2e-85f9-cc465299093a

  1. Primary button dropdown - you can correctly tab to the primary button and press enter/space to activate the dropdown, then first option in the dropdown has focus. However, if you then shift+tab to try to return focus to the primary button, the entire dropdown menu takes focus and no interactions are available, you must press shift+tab again to return focus to the primary button.

  2. When the primary button takes focus, it looks like the blue focus ring is present for a quick millisecond before disappearing. I'm not sure if others are noticing this flicker. The tooltip itself would meet criteria for focus indicator, but I think having the outline in addition would make it even more clear. so we could maybe consider keeping the focus ring as well, especially if others think the flicker of the outline is noticeable, or find a way to remove the outline altogether so there is no flicker. https://github.com/carbon-design-system/ibm-products/assets/21047571/85ae6efd-8b37-43ff-bb6a-6732d3e3a877

kristastarr commented 1 year ago

Tested with Mac Voiceover - everything except Settings works well. For Settings, after you click/activate the button, there is no information about the popover and it's status of open/closed, or it's contents and how to interact with them, so need to add some aria labels here.
See https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-haspopup

elycheea commented 1 year ago
  1. Focus should be present in the first list item after pressing space/enter to activate the popover. Also as you press up/down arrows to cycle through the radio button options, the table itself moves up and down behind it.
  2. When popover is open and you tab to settings, it closes. However, if popover is open and you shift+tab to Download before interacting with the popover content, the popover remains open and overlaps with the Download tooltip. I’m thinking it should close so that what happens with shift+tab is consistent with what happens with tab. Screenshot 2023-07-03 at 8 16 05 AM
  3. When open, the popover should dismiss when Esc is pressed

Documented in the new issue (#3241).

  1. However, sometimes when you tab to it, only a faint 1px blue outline appears around the button (which does not match the rest of the focus states), and then nothing happens when you press space, and focus is lost. After pressing tab twice, focus resumes on the settings button. I’ve tried to determine what pattern of actions causes it to work vs not work. I think it might be alternating between correct and incorrect, but I’m not 100% sure.

I wasn’t able to replicate this. Were you using a specific browser?

  1. After using up and down to move focus to your selected radio button, pressing tab moves you to the “Primary button” but wondering if it would be better to press enter to make a selection, which would move focus to the settings button, then use tab to move to the primary button. I’m looking for some examples of best practices to reference.

No specific tasks for this one at this time but hopefully we’ll have a better idea about the approach as more people use the component.

  1. When Primary button has focus, and you press tab, focus is unknown, and then if you press tab again it lands on the next interactive element (the “Create new” button), instead of going directly to the Create new button. I think the dropdown for "Primary button" is a tab stop even when it is not displayed.

I believe this is actually the whole data table body is its own tab stop. Don’t recall if there was a specific reason for it, but open to more discussion on whether to change that behavior. (Might have implications in other stories though.)

  1. Visual things: The “Create new” text is not centered vertically in the button The focus state on “View documentation” looks weird since there is lots of white space on the right of the text as the focus indicator takes up the entire width of the “empty-state__content” container

Not an issue with the Datagrid in particular — believe this came from an update in the Carbon buttons but should be something we can address separately. I think both of these stem from the EmptyState components though.

  1. Primary button dropdown - you can correctly tab to the primary button and press enter/space to activate the dropdown, then first option in the dropdown has focus. However, if you then shift+tab to try to return focus to the primary button, the entire dropdown menu takes focus and no interactions are available, you must press shift+tab again to return focus to the primary button.
  2. When the primary button takes focus, it looks like the blue focus ring is present for a quick millisecond before disappearing. I'm not sure if others are noticing this flicker. The tooltip itself would meet criteria for focus indicator, but I think having the outline in addition would make it even more clear. so we could maybe consider keeping the focus ring as well, especially if others think the flicker of the outline is noticeable, or find a way to remove the outline altogether so there is no flicker.

Not prioritizing these for now since these will be replaced with the Carbon MenuButton.