-
Notifications
You must be signed in to change notification settings - Fork 793
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: Updated images in the Dropdown's Style and Accessibility tabs to display the focus state #4342
fix: Updated images in the Dropdown's Style and Accessibility tabs to display the focus state #4342
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style tab
Color section
- Shouldn’t the open states have the focus on the fields?
- Combo box should be two words.
- The caption is wrong here. It should be something like “Color states of dropdown, combo box, and multiselect.”
Interactive states section
Dropdown images
- The default image has the wrong label color for the “Warning” and “Error” states. It should be
$text-secondary
. - For the default and fluid images, shouldn’t the “Hover open” state have the focus state on the field?
Combo box images
- In the default and fluid images for the “Selected” states and the "Read-only" state, the third option should be selected for consistency.
Multiselect images
- The “Disabled” and “Read-only” state is the wrong size. It should be Medium like the others.
- For the default and fluid images, shouldn’t the “Hover open” state have the focus state on the field?
- For the default and fluid images “Hover open” state, the hovered option text should be
$text-primary
. - For the default image, the “Read-only” state should have "3" in the tag for consistency.
- For the fluid image, the “Warning” state message text should be
$text-primary
. - For the default and fluid images in the “Selected open” states, I am pretty sure the word “All” is supposed to be
$text-secondary
even if all options are selected in the menu list. - For the default and fluid images in the "Focus open" state, the dividers should be inset for the options in the menu. It is only the divider for the All option that is flush.
Inline images
- Shouldn’t the “Hover open” state have the focus state on the field?
- This example of inline shows a single select dropdown, so the placeholder text in the field should say "Choose an option".
- The open menus are not all the same width.
- We might need to tweak this image a bit slightly, I made an edit and comment in your Figma file. It is a tricky image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessibility tab
Keyboard interactions section
Section image
- Shouldn’t the focus state be on the option in the menu since it is selected?
Multiselect image
- The chevron needs to be pointing "up" since the menu is open.
src/pages/components/dropdown/images/combobox-style-default-states.png
Outdated
Show resolved
Hide resolved
@laurenmrice this is really a Carbon decision, not an accessibility MUST do. What I believe Kritvi has tried to make consistent is that focus is retained in the parent in two aligned circumstances:
If either a DownArrow is used to open the component or there is an option selected, then focus moves to the option list. I emphasize that this ultimately is a design decision. You could as easily say that focus always moves to an item in the options when the component is opened, regardless of method (which I believe is historically how Carbon did things). That is the pattern the ARIA authoring practices (APG) has for their Listbox pattern. And it is very similar to the Menubutton interaction, which is a similar but different disclosure pattern, with a much more constrained interaction model. However, Kritvi's approach, based on a common implementation in Storybook, is to consistently apply the above numbered behaviour. I believe it is defendable as an approach for any kind of non-menu 'dropdown' disclosure: for a mouse user, the focus remains on the trigger; for a keyboard user, Enter is used as an open/close toggle while DownArrow is used as a open/move-focus-to-child operation. This aligns, to a degree, with the APG Disclosure pattern. Personally, I'd lean more to the Listbox pattern, but if done consistently, the current approach also seems to make sense. |
@mbgower My question was that the image here seems wrong because there is no focus state when the dropdown is open, and there is no selection. My understanding is that there needs to be focus somewhere, whether it be on the field/trigger, or on the first option in the menu. For mouse users, which is what we are showing here, design has decided that if the menu is open and nothing is selected, the focus would appear on the field/trigger. So I just thought this image needed to be updated with that guidance of adding the focus state in. |
@laurenmrice I'll update all images on the Style tab to reflect the latest focus state discussion. |
@laurenmrice I've already made the changes, and they’re reflected in one of the commits, though I'm not sure why it is not showing up. Either way, I’ve addressed all the comments again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kritvi-bhatia17 I think after these changes, it will be good to go!
Color section
- For the default image, the Multiselect closed components is the large size, it needs to be the medium size to match all of the other component sizes in this image for consistency.
Interactive states section
Multiselect
- Something I did not catch before, but the default and fluid images error state error border should be 2px not 1px.
Inline
- Something I did not catch before, but the read-only state input text “Option 3” should be text color
$text-primary
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Hi @mbgower, could you please review this PR? Thanks! |
Rather than look at just the changes in the PR, I looked at the whole a11y tab in the preview. Within that context, I have some concerns not specific to this PR; the keyboard guidance seems unchanged. For example, it still reads:
I believe this isn't precise enough, given the interaction model you have championed, which I describe in a prior comment. To me, either the accessibility tab or the usage tab must must explain the interaction model for focus handling. The above quoted paragraph is one place it could potentially be included, although you then need to address it somehow for all the variatns. Another option is to have a separate subsection called something like 'Focus handling in open dropdowns'. I am generating a PR to address. |
addresses a shortcoming in the current keyboard guidance on the a11y tab as discussed in #4342 (comment)
See #4362 |
* Update accessibility.mdx addresses a shortcoming in the current keyboard guidance on the a11y tab as discussed in #4342 (comment) * chore: format --------- Co-authored-by: Alison Joseph <[email protected]>
Closes #4049 & #3935
This PR reflected the updated images with the focus stroke change and add a new image for Combobox on the Style tab.
Changelog
New
Changed