fix(picker): migrated picker to FieldLabelMixin and accessible implementation of select-only combobox pattern#6090
Conversation
…web-components into nikkimk/SWC-1449-picker-as-combobox
…web-components into nikkimk/SWC-1449-picker-as-combobox
|
…t and label in combobox button
📚 Branch Preview Links🔍 First Generation Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
nikkimk
changed the title
formFieldMixin and accessible implmentation of select-only combobox pattern
nikkimk
changed the title
formFieldMixin and accessible implmentation of select-only combobox patternFieldLabelMixin and accessible implementation of select-only combobox pattern
There was a problem hiding this comment.
Like the architectural direction here. I have left a few suggestions to strengthen the implementation, and called out some blockers inline that need to be addressed.
There are also a few areas I’d like us to discuss before I can give a final pass on this —happy to sync when you’re ready. Thanks for driving this forward.
Suggestion:
hasAccessibleLabel has 6 @todo statements. Can we add tracking issues and reference them inline. Happy to sync to calibrate which issue is worth investing on priority before we merge this as I feel cross root ARIA issue is important as they could effect the accessibility of the entire label chain?
| aria-haspopup="true" | ||
| aria-labelledby="icon label applied-label pending-label" | ||
| aria-haspopup="listbox" | ||
| aria-labelledby="${ariaLabelledBy} pending-label" |
There was a problem hiding this comment.
blocker: Any consumers relying on JS targeting will break silently. We have removed icon and label it seems. Should we document this in a migration table?
| */ | ||
| @state() | ||
| protected get selectedItemContent(): MenuItemChildren { | ||
| public get selectedItemContent(): MenuItemChildren { |
There was a problem hiding this comment.
This is expanding the API surface. If this mixin needs internal access how about a symbol-based approach? Something like
const SELECTED_ITEM_CONTENT = Symbol('selectedItemContent');
// In the mixin or class:
[SELECTED_ITEM_CONTENT]: MenuItemChildren | undefined;If this is truly needed to be public for a documented consumer use case, Can you please add it to the component's JSDoc @Property documentation and include it in the changeset as a new API addition.
| 'ArrowRight event should not propagate' | ||
| ).to.equal(0); | ||
|
|
||
| //@nikki |
| placeholder="Choose a selection type with a very long label, too long, in fact" | ||
| ${spreadProps(args)} | ||
| > | ||
| <span slot="field-label">Selection type:</span> |
There was a problem hiding this comment.
blocker: This is the new pattern. You have an a11y test for this that references this story which will not validate the deprecation path. Can you update this story to use the deprecated sp-field-label element?
export const DeprecatedSpFieldLabel = (args: StoryArgs): TemplateResult => {
return html`
<sp-field-label for="picker-deprecated">Selection type:</sp-field-label>
<sp-picker
id="picker-deprecated"
@change=${handleChange(args)}
placeholder="Choose a selection type"
${spreadProps(args)}
>
<sp-menu-item value="option-1">Deselect</sp-menu-item>
<!-- ... -->
</sp-picker>
`;
};| // Chrome adds labels and alt text to the icon content, so we need to remove them | ||
| // since we're already capturing them as text for Firefox and WebKit | ||
| // and otherwise our fix for Firefox and Safari would cause the label to be read twice | ||
| const icon = this.selectedItemContent?.icon?.map((node) => { |
There was a problem hiding this comment.
This is cloning icons and stripping attributes on every render which is expensive as it creates new DOM nodes on every render cycle. This can recreate the DOM subtree each time. The best approach here would be to process icons once when the selection changes and not on every render, you can do this in selectedItem setter. A rough idea would be something like this.
@state()
private _processedIcons?: Node[];
public set selectedItem(selectedItem: MenuItem | undefined) {
this.selectedItemContent = selectedItem
? selectedItem.itemChildren
: undefined;
// Process icons once on selection change
this._processedIcons = this.selectedItemContent?.icon?.map((node) => {
const clone = node.cloneNode(true) as HTMLElement;
clone.removeAttribute('label');
clone.removeAttribute('alt');
return clone;
});
.....
}Now In buttonContent getter, use this._processedIcons instead of cloning
| applyFocusElementLabel = (value: string, labelElement: FieldLabel): void => { | ||
| this.appliedLabel = value; | ||
| this.labelAlignment = labelElement.sideAligned ? 'inline' : undefined; | ||
| window.__swc.warn( |
There was a problem hiding this comment.
blocker: This will throw a runtime error. Let's check if (window.__swc?.DEBUG) before we call warn?
| if (!this.hasUpdated && this.querySelector(':scope > sp-menu')) { | ||
| const { localName } = this; | ||
| window.__swc.warn( | ||
| this, | ||
| `You no longer need to provide an <sp-menu> child to ${localName}. Any styling or attributes on the <sp-menu> will be ignored.`, | ||
| 'https://opensource.adobe.com/spectrum-web-components/components/picker/#sizes', | ||
| { level: 'deprecation' } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Can we keep this till the sp-menu support is actually dropped. This seems to be a premature removal, there are many consumers who are yet to fully migrate to our latest.
|
|
||
| ```html demo | ||
| <sp-field-label for="uses-sp-field-label">Selection type:</sp-field-label> | ||
| <sp-field-label for="uses-sp-field-label"></sp-field-label> |
There was a problem hiding this comment.
Let's add a text content here. :D
| <sp-picker | ||
| id="picker-loading" | ||
| label="Loading blending modes..." | ||
| pending |
There was a problem hiding this comment.
Did we add any replacement for this pending-label property example?
| const el = test.querySelector('sp-picker') as Picker; | ||
|
|
||
| await elementUpdated(el); | ||
| const picker = el.querySelector('sp-picker') as Picker; |
There was a problem hiding this comment.
el is already the picker, so el.querySelector('sp-picker') is looking for a nested picker. The if (picker) guard silently no-ops. You can remove this dead code.
2 participants
Description
FieldLabelMixin<sp-field-label>in pickerfield-label<sp-field-label>in pickerplaceholderfield-labelplaceholder<sp-field-label>in pickernameandvaluein a11y tree, including cross browser support for icons-only implementation<sp-field-label>in pickerMotivation and context
Add placeholder functionality to Picker components to provide better user experience and address consumer misuse of label attributes as placeholders. While placeholders are controversial in accessibility circles, there is still interest in implementing them as evidenced by design team discussions. This feature will introduce proper placeholder support while maintaining backward compatibility and accessibility compliance.
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Ignore the following failing tests:
nikkimk/form-field-mixinbranchnikkimk/form-field-mixinbranch to be mergednikkimk/form-field-mixinbranch to be mergedUsing the picker documentation and Storybook examples:
Device review
Accessibility testing checklist
Required: Complete each applicable item and document your testing steps (replace the placeholders with your component-specific instructions).
Keyboard (required — document steps below) — What to test for: Focus order is logical; Tab reaches the component and all interactive descendants; Enter/Space activate where appropriate; arrow keys work for tabs, menus, sliders, etc.; no focus traps; Escape dismisses when applicable; focus indicator is visible.
Screen reader (required — document steps below) — What to test for: Role and name are announced correctly; state changes (e.g. expanded, selected) are announced; labels and relationships are clear; no unnecessary or duplicate announcements.