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: make remove button in sl-tag focusable #1741

Draft
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

mariohamann
Copy link
Contributor

@mariohamann mariohamann commented Nov 23, 2023

Current behaviour

Remove buttons in tags are currently not reachable via keyboard, see https://shoelace.style/components/tag#removable

CleanShot.2023-11-23.at.17.40.39-converted.mp4

New behaviour

Remove buttons are reachable via keyboard, see https://shoelace-git-fork-mariohamann-next-font-awesome.vercel.app/components/tag#removable

CleanShot.2023-11-23.at.17.45.30.mp4

As test is provided which fails before the change. I wanted to utilize window.activeElement but I believe it doesn't play well with ShadowDOM, so I went with the emit solution.

Copy link

vercel bot commented Nov 23, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
shoelace ✅ Ready (Inspect) Visit Preview Nov 23, 2023 4:44pm

@claviska
Copy link
Member

I'm not sure what the behavior here should be, but I can tell you why it is how it is today and we can discuss it.

In the past, we've seen requests for things like the clear icon in <sl-input> and <sl-select> be tabbable to make it keyboard accessible. However, this actually isn't desirable. In fact, putting the clear button in focus can be a frustrating experience, as the control now has a varying number of tabs to get through before moving onto the next one. For example:

If the input is empty, no clear button is shown so one tab will exit the field. But if the input is not empty, a clear button is shown and one tab will not exit, but focus the cursor on a destructive action instead.

Native [search] inputs work the same way. Tabbing doesn't move to the clear button. The focus behavior is always consistent. The reason for this is that keyboard users already have a number of ways to clear an input, some of which are arguably faster or just as easy as tabbing to a clear button:

  • CMD + A, Backspace/delete
  • CMD + Backspace/delete
  • Select with mouse + Backspace/delete

Note that, despite being out of the tab order, clear buttons are still discoverable and accessible to screen reader users through the virtual cursor.

The thought behind this decision comes from Scott O'Hara's explainer:

https://scottaohara.github.io/clear-text-field-button/

The reason I bring all this up is because the tag's remove button is a lot like a clear button, and accepting this PR would cause a variable number of tabs when used with <sl-select multiple>. That is, one tab per item selected.

While you could take the time to tab through each option to remove it, it's probably easier to access it through the menu where activating any selected item will toggle the selection and remove the tag. For this use case, we probably don't want to focus on the remove button. However, there may be other use cases where this makes sense.

I guess the question is, how are you using tags so they need this focus? And if we're going to focus the remove button, shouldn't be also make the tag itself focusable?

That might even be a different component altogether. Maybe we shouldn't use tags in <sl-select>. 🤔

@mariohamann
Copy link
Contributor Author

mariohamann commented Nov 27, 2023

Getting the problem. The tag is an interesting element – it visually fits most to the concept of Chips in Material Design. (https://m3.material.io/components/chips/overview)

But sl-tag doesn't provide any interactivity – besides the remove button, which at least would be fine for some use cases like "activated filters" in shops or similar, IF the remove button would be focusable. 😀

What do you think about extending the tag that it can (!) be a button or link, receiving interactive styles etc.? Would you be open for a (new, so we can close this one) PR like that?

@claviska
Copy link
Member

I'd like to get @lindsaym-fa's opinion on this to see if we want to keep using tags in <sl-select multiple> or not. From there, we can better decide how to proceed.

@lindsaym-fa
Copy link
Collaborator

And if we're going to focus the remove button, shouldn't be also make the tag itself focusable?
That might even be a different component altogether. Maybe we shouldn't use tags in <sl-select>. 🤔

Definitely a lot of ways we can tackle this, but I'm of the mind that tags are well-suited for sl-select. sl-tag should inherently be able to support keyboard operation, though that might (and probably should) look different in the context of sl-select.

It makes sense to me for sl-tag itself to be focusable, rather than simply the 'remove' button. (Perhaps that means we offer another attribute for a developer to indicate whether the tag is interactive and provide additional hooks for implementing behaviors.) Either way, I agree that <sl-tag removable> should be in some way focusable with Tab and removable with Enter or Space. If the entire tag is focusable, we could also consider removing with Delete or Backspace.

Within a contained input context, however, arrow keys are a more familiar way to control which element has "focus" while maintaining true focus on the input. That's the pattern I'd expect to see in <sl-select multiple> — Tab moves focus to and from sl-select, and each tag within can receive "focus" with the left or right arrow keys. Pressing Space, Delete, or Backspace removes the "focused" tag.

@mariohamann
Copy link
Contributor Author

That makes sense.

So could the following be an approach:

  1. sl-tag gets type: 'submit' | 'button' and href: string properties without default values, which would enable opt-in for interactive styles (hover etc.) + would make either a semantically button or link? I would change the sl-icon-button inside the sl-tag to become an sl-icon with the same event handlers, so we shouldn't have any regression on the current behaviour and (as it wasn't tabbable before) a11y.
  2. Next iteration could then be to improve sl-select multiple to make those elements focusable via keyboard as you outlined with arrows etc.

I could at take over the first part inside this PR and could create an issue then for the second part?

@a-hockergard
Copy link

A very interesting discussion.

  1. I see a great value in having both static (span) and interactive (link/button) tags. As @lindsaym-fa mention focusing the sl-tag itself makes sens and removing with Enter, Space, Delete or Backspace. Especially if used in sl-select multiple.

  2. Looking forward to see progress on this issue as well.

@claviska claviska marked this pull request as draft January 23, 2024 15:46
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.

None yet

4 participants