fix(Search): close expandable search on escape key#14076
Conversation
✅ Deploy Preview for carbon-components-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Tagged visual review to get design's perspective on focusing the button on close via escape? Seems to me like the focus needs to go somewhere after closing via escape but also focusing the magnifier button means the expandable search will reopen on tab out since the input is the next tabbable element in the dom and it expands on focus, so you'd have to tab twice to skip over it. Would also love to hear your thoughts on this @mbgower Expandable: https://deploy-preview-14076--carbon-components-react.netlify.app/?path=/story/components-search--expandable |
|
Yeah, there's something a little odd about this interaction. It might help to put it in a simple page where it might appear, so we can see how the interaction is expected to take place. As it is, having just the one component makes it a little hard to assess in isolation. My take on it, in isolation, is as follows:
|
|
@mbgower in production the search button is not tabbable/focusable at all, the input is focusable and expands on focus. The problem here is with the introduction of that tab stop in the button and the input continuing to be "Expandable on focus" , we get this weird behavior, I'm really not sure what a "fix" here would look like 😅 |
That's a little odd. But setting that aside for a moment, I think if the functionality of the Esc is to clear any search value entered, then after pressing Esc, it would just leave me at the cursor in an empty input (just like would happen in any input). You cannot collapse the search input while it has focus, IMO. That defies the model you have set up. If you look at the behaviour of the search button at the top of the carbondesignsystem.com page, you'll see a search input Esc pattern that works because the search button takes focus and its activation triggers the search input. That is one option. (But I've personally always found that suspect on a web page; it saves little space and makes more time on task.) The other is that the search button in the table never exists, and it's just the input. But if that's so, IMO there is no way to allow the input to 'collapse' except when the user physically leaves it. Esc cannot logically do that in the current configuration. A bigger question for design is 'what do you actually gain by hiding the search input?' I'd really like to understand the thinking. |
|
For some context, this change came about as a suggestion from Ram that he'd expect there to be a way to close the expandableSearch other than tabbing out of it. @carbon-design-system/design from what @mbgower is saying, sounds like the only two options from an a11y standpoint here are to: Any thoughts on either/or? |
|
@francinelucca we can bring this up in a call on Tuesday, June 27th - with Ram and see which is preferred / get some insight. Let me know if that works! |
…rch-keyboard-action-need-to-close-expanded-search
|
@francinelucca @mbgower After speaking with Ram on today's call we have clarity on the desired interaction:
|
|
@andreancardona The above makes sense to me. |
…rch-keyboard-action-need-to-close-expanded-search
This is a grey area. While this might break consumers' tests, I think this is a necessary change to fix a high severity issue for non-sighted users. It might be disruptive, but imo it's worth the cost to fix non-sighted/keyboard nav users' broken experience. |
…rch-keyboard-action-need-to-close-expanded-search
…943-a11y-containedlist-with-expandable-search-keyboard-action-need-to-close-expanded-search
…d-action-need-to-close-expanded-search' of github.com:francinelucca/carbon into 13943-a11y-containedlist-with-expandable-search-keyboard-action-need-to-close-expanded-search
|
I think you have that working pretty well on the main search, right? I think it's good to emulate. Once there is a string entered, Esc will clear. If no string, Esc collapses
|
…943-a11y-containedlist-with-expandable-search-keyboard-action-need-to-close-expanded-search
alisonjoseph
left a comment
There was a problem hiding this comment.
This LGTM and appears to be working as intended after reading through the comments. 😅
…rch-keyboard-action-need-to-close-expanded-search
There was a problem hiding this comment.
Functionality is there, but I had one question for @laurenmrice and the focus state: should the outline be 2px to match the focus outline when the search is expanded? I figured it would match the focus appearance of a ghost button, but right now it's only 1px and seems off
|
@tw15egan Oh yep, good point. The focus should be 2px on the search icon and on the full expanded search for consistency. 👍 |
…rch-keyboard-action-need-to-close-expanded-search
…rch-keyboard-action-need-to-close-expanded-search
|
also closes #13941 |
Closes #13943
Closes #13941
Changelog
New
ExpandableSearchclosses on Escape key keyDown (if the input is clear, otherwise it'll clear the input first)ExpandableSearchclosed with Escape keyaria-controls&aria-expandedproperties forExpandableSearch's activation buttonChanged
onExpand(was not taking event param into account) and addedKeyboardEventas an event param typeExpandableSearchnow opens on button activation (enter/space) instead of focusTesting / Reviewing
Test
ExpandableSearchandContainedList with ExpandableSearchstories and make sure everything behaves as expected but should now:EscapekeyEscapekey (when the input is clear)Searchstory should behave exactly the same as it did before these changes.