Refactor select #1794
Refactor select #1794
Conversation
🦋 Changeset detectedLatest commit: 20e547a The changes in this PR will be included in the next version bump. This PR includes changesets to release 33 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🕸 Website previewYou can view a preview here (commit |
🕸 Storybook previewYou can view a preview here (commit |
| this.tabIndex = this.disabled ? -1 : 0; | ||
| if (changes.has('placeholder') || changes.has('selected')) { | ||
| if (this.placeholder && !this.selected) { | ||
| this.setAttribute('aria-placeholder', this.placeholder); |
There was a problem hiding this comment.
The placeholder is not being read by NVDA.
| @@ -24,20 +33,29 @@ export class SelectButton extends ScopedElementsMixin(LitElement) { | |||
| /** @internal */ | |||
| static override styles: CSSResultGroup = styles; | |||
|
|
|||
| // eslint-disable-next-line no-unused-private-class-members | |||
| #events = new EventsController(this, { keydown: this.#onKeydown }); | |||
There was a problem hiding this comment.
Right now I cannot open the select with space key, but opening select with space is possible in the native HTML select element, so I guess it should be possible here as well?
There was a problem hiding this comment.
Or maybe it should be also possible to open it with enter?
https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/
| @@ -249,9 +253,9 @@ export class Select<T = unknown> extends ObserveAttributesMixin(FormControlMixin | |||
| super.firstUpdated(changes); | |||
|
|
|||
| requestAnimationFrame(() => { | |||
| this.button.setAttribute('aria-controls', this.listbox.id); | |||
There was a problem hiding this comment.
Why there is no aria-controls?
There was a problem hiding this comment.
If we really want to remove aria-controls, please remove it as well from the documentation, from the accessibility WAI ARIA description. :)
| Various improvements: | ||
| - Add `clearable` property for clearing the selection | ||
| - Add `aria-hidden="true"` to the placeholder content | ||
| - Add `aria-placeholder` to the `<sl-select-button>` when the placeholder is shown |
There was a problem hiding this comment.
Please describe it in the documentation, in the accessibility tab (WAI-ARIA part) as well.
| @@ -13,6 +19,9 @@ declare global { | |||
| } | |||
| } | |||
|
|
|||
| Icon.register(faCircleXmark, fasCircleXmark); | |||
There was a problem hiding this comment.
Maybe those icons need to be added to the core.json file instead of registering them here?
| @@ -49,39 +51,40 @@ export default { | |||
| ${slot?.() ?? | |||
| html` | |||
| <sl-select | |||
| ?clearable=${clearable} | |||
There was a problem hiding this comment.
Clicking on the label in the form-field should focus the select component (select-button), right?
Just like it works with other form fields.
Right now it's not working.
| ${anchor({ | ||
| element: this.button, | ||
| offset: Select.offset, | ||
| position: 'bottom-start', | ||
| viewportMargin: Select.viewportMargin, | ||
| rootMarginTop: this.rootMarginTop | ||
| rootMarginTop: this.rootMarginTop, |
There was a problem hiding this comment.
In the current version of the select, when we scroll down and the select is opened, when it's not visible, the dropdown is closing and when we scroll up and the select is visible again, the dropdown is visible again as well (when the select was not closed explicitly by the user):
https://storybook.sanomalearning.design/?path=/story/form-select--hide-when-out-of-view
In the version from this PR it's not working the same, the listbox is not being shown again, when I scroll up:
<sl-listbox>,<sl-option>and<sl-option-group>components instead of custom onesclearableproperty for clearing the selectionaria-hidden="true"to the placeholder contentaria-placeholderto the<sl-select-button>when the placeholder is shown<sl-select>componentFixes #1783
Fixes #1761