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

[color-slider][channel-slider][space-picker]: <label for="id"> not attaching to htmlFor element #177

Open
sidewayss opened this issue Dec 2, 2024 · 11 comments
Assignees

Comments

@sidewayss
Copy link

sidewayss commented Dec 2, 2024

https://codepen.io/sidewayss/pen/Wbevapq

Open the console in CodePen and compare to JavaScript pane:

  • Clicking on a label does not set focus to the element, it sets focus to the body.

I assume that the label should set focus to the htmlFor element because I see from these code search results that these elements all have a version of this declared (this one is from <space-picker>):

static formAssociated = {
	like: el => el._el.picker,
	role: "combobox",
	changeEvent: "change",
};

It's harder to decipher in CodePen, but if you open this original version of the pen on my personal site in Chrome, go to Dev Tools, the Issues tab in the console, you'll see "Incorrect use of <label for= etc.". In the pen you'll find 4 such "violating nodes", and the bottom two are the color elements. So Chrome is not recognizing these as labelable elements.

I just tried this in my repo and setting formAssociated = {} instead of = true makes no difference there.
I can't seem to get into NudeElements in the debugger, so I'm not sure how to check if attachInternals() is actually running.

@LeaVerou
Copy link
Member

LeaVerou commented Dec 4, 2024

@DmitrySharabin could you triage?

@DmitrySharabin
Copy link
Member

Here are the results of my investigation and experiments.

Actually, <label> is correctly attached to the corresponding element:
image

But why don't we see a focus ring (or whatever) we defined in the :focus rule?

Default.mov

I suggested that it's because our elements are not focusable by default (they are like <div>s or <span>s, etc.). So, if we make them focusable explicitly (with tabindex), we'll begin seeing our styles.

With.tabindex.mov

However, this is not enough: We expect to be able to select spaces as soon as the picker is focused with the down/up arrow keys. Right? Let's try.

Not.keyboard.accessible.mov

Nope. It doesn't work. What actually needs to be focused when we focus the picker is the underlying <select>. We can handle that by listening to focus events on the element itself.

Handle.focus.event.mov

Yay! But not so fast. If we tab through the page, we'll see that we can focus the space picker, but again, we can't choose spaces with the keyboard. So, those cases should be handled separately.

Issues.mov

@sidewayss
Copy link
Author

I did the same thing, setting tabindex to zero by default. Note that clicking on the label acts differently than clicking on the associated element. I just tested the <space-picker> demo: Clicking on and tabbing to the element focuses the inner <select>. In that case it's because that version of the <space-picker> has tabindex="-1". But even with tabindex="0", clicking on the element would activate the <select> because there's nothing else in the element to click on, not even a border.

However, this is not enough: We expect to be able to select spaces as soon as the picker is focused with the down/up arrow keys. Right?

You might want to consider that a separate issue. IMO it goes beyond focusing the element via a label, which you solved with your tabindex setting. As I alluded to my now closed issue with vague ramblings on the topic, focus in autonomous custom elements can get funky. Setting tabindex="0" opens up a can of worms. Here are some examples:

When you combine focusable and unfocusable elements inside the custom element, complications arise due to combinations of document.activeElement and customElm.shadowRoot.activeElement. This case caused me the most grief:

  • document.activeElement == this && this.shadowRoot.activeElement == null

That's when the custom element is focused, but you have clicked on something unfocusable inside it, or tabbed from an inner element to the outer custom element itself. Yes, the outer custom element is a separate item in the tab order that includes slotted/templated elements, at least when you combine un/focusable inside it. (fyi - there is a Chrome issue that I have yet to submit: in this case customElm.blur() does not fire the blur event. I had to set focus to the interior focusable element and then blur it.)

Once you get into managing focus/blur or focusin/focusout there are various rules that apply. For example, tabbing within the element, including tabbing to/from an interior element to/from the outer element, does not fire a focus or blur event for the outer element - it is the document.activeElement throughout that series of events. In that case I must check event.relatedTarget inside the blur event for the inner element. Obviously it's best to avoid handling these events if you can help it.

To understand all the wrinkles in my setup, I had to test focusing and blurring the inner and outer elements in all the possible ways, via both mouse and keyboard. I don't have any custom elements with just a single focusable element inside, like <space-picker>, so I haven't tested that. It should be simpler than the element in which I had to handle these events, in which there are two unfocusable yet clickable SVG elements alongside an <input>.

@sidewayss
Copy link
Author

sidewayss commented Dec 5, 2024

It sounds like the ideal thing for the <space-picker> would be to attach the label to the inner <select> and leave the picker's tabindex at -1. No extra outer element in the tab order. The picker + label would work the way you want without any intervention. But I don't think it's possible.

For keyboard navigation, once you set tabindex to zero, you're stuck with the extra, focusable outer element. For mouse/touch, it's not an issue for the default <space-picker>, but if you add a border or padding it becomes one. With tabindex at -1, clicking on the outer border/padding activates document.body - it's effectively disabled. With tabindex at zero, you activate the outer element without focusing the <select>, just like clicking the label without any intervention: shadowRoot.activeElement === null

This is where inheriting from HTMLSelectElement instead of HTMLElement seems appealing. Or to have the label be part of the template, and its innerHTML be a property on the custom element. Not that I'm advocating for either of those here.

@sidewayss
Copy link
Author

https://codepen.io/sidewayss/pen/Wbevapq

I have updated the original codepen to add:

  • a 2nd <space-picker> with tabindex="0", set as an attribute.
  • border/padding to the elements.
  • :hover style to illustrate that tabindex="-1" doesn't set pointer-events: none, it only prevents the element from receiving focus.
  • a click event for the <space-picker> element that logs to the console.

Note the way the two <space-picker> elements respond to different forms of navigation and mouse interaction.

I'm not suggesting any course of action. I'm still figuring out how this works. I set a default tabIndex = 0; in my base class constructor months ago. I wasn't aware of all these details about what tabindex does until now. My only suggestion would be to try avoid handling focus/blur/focusin/focusout events.

@LeaVerou
Copy link
Member

LeaVerou commented Dec 6, 2024

Wait, so even though there are focusable elements within the component, we can’t focus them without setting tabindex on the host?! Yikes. Does ElementInternals really offer nothing for this?

for the other thing, look up delegatesfocus

@sidewayss
Copy link
Author

Wait, so even though there are focusable elements within the component, we can’t focus them without setting tabindex on the host?!

Not when you click on the outer element or its label (or an unfocusable inner element). Keyboard navigation works. Clicking on the inner element works. If there's no padding or border on the outer element then you can't click on it.

Does ElementInternals really offer nothing for this?

I have no idea. You definitely know more about it than I do.

for the other thing, look up delegatesfocus

Nice to know. Would it override this tabindex issue? I'll fiddle with it tomorrow to see exactly what it does. Though for my element, I wouldn't need it. The outer element gets focus instead of the inner unfocusable elements, and that works out fine, in spite of the extra work in terms of handling the events.

@sidewayss
Copy link
Author

sidewayss commented Dec 6, 2024

delegatesFocus docs are here.

The property value is originally set using the delegatesFocus property of the object passed to Element.attachShadow(), or using the shadowrootdelegatesfocus attribute of the <template> element when a shadow root is created declaratively.

This example sets shadowrootdelegatesfocus on the <template>.

So I can't set it in the codepen for demo purposes. Based on the docs and the examples there, the combination of tabindex="0" and delegatesFocus should give you what you want with the labels, and also with padding/border. No need to handle any events.

What's so bad about having to set tabindex to zero if delegatesFocus cleans up the messy parts? Or are there other side-effects besides the complications that arise if you have to add autofocus to the mix (which wouldn't apply for these elements that only have one focusable element inside the template)? I see that to disable the element fully you must set tabindex to -1 for the outer and inner elements, so a small inconvenience there.

I'm also wondering why you wouldn't inherit directly from HTMLSelectElement, or HTMLInputElement for the sliders. Is it because of the polyfill required for Safari? Or are there other reasons? It could avoid all of this and be much simpler - though I've never tried it...

@DmitrySharabin
Copy link
Member

DmitrySharabin commented Dec 9, 2024

OK, I checked the delegatesFocus property, and it looks like it works (without tabindex="0"). If we pass it to attachShadow(), our elements become focusable.

this.attachShadow({mode: "open"});

- this.attachShadow({mode: "open"}); 
+ this.attachShadow({mode: "open", delegatesFocus: true});
delegatesFocus.mov

I'll check how our other elements behave if we enable this feature and send a PR with the fix.

@DmitrySharabin
Copy link
Member

For the following code snippet, I got the results:

<label for=el>Element:</label>
<color-element id=el></color-element>
Element Focused Part
<gamut-badge> No focusable parts
<color-inline> No focusable parts
<space-picker> Underlying <select>
<channel-picker> ???
<color-slider> Underlying <input type=range />
<channel-slider> Spinner with the slider value
<color-swatch> (static) No focusable parts
<color-swatch> (editable) ???
<color-picker> ???
<color-scale> ???
<color-chart> ???

For <channel-picker>, I expect <select> corresponding to the selected channel to be focused by default (for now, it's not, I need to investigate why).

For editable <color-swatch>, it should be the slotted <input> element (for some reason, it's not the case — I need to investigate it, too).

I'm not sure what should be focused inside <color-picker>, <color-scale>, and <color-chart> if they are associated with <label>. 🤷‍♂️

@sidewayss
Copy link
Author

@DmitrySharabin - I think that the answer regarding what gets focus inside <color-scale> is here: #179 (comment)

Or at least that's a good place to start.

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

No branches or pull requests

3 participants