Skip to content

Commit

Permalink
fix(select-rich): update WAI ARIA pattern to combobox-select-only
Browse files Browse the repository at this point in the history
  • Loading branch information
gerjanvangeest committed Mar 15, 2023
1 parent adfa29a commit 776686f
Show file tree
Hide file tree
Showing 8 changed files with 27 additions and 10 deletions.
5 changes: 5 additions & 0 deletions .changeset/honest-foxes-pretend.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@lion/ui': patch
---

[select-rich] update design pattern to combobox-select-only
2 changes: 1 addition & 1 deletion docs/components/select-rich/overview.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ It allows providing fully customized options and a fully customized invoker butt
styling,theming or user interaction opportunities.

Its implementation is based on the following Design pattern:
<https://www.w3.org/TR/wai-aria-practices/examples/listbox/listbox-collapsible.html>
<https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/>

```js script
import { LitElement, html } from '@mdjs/mdjs-preview';
Expand Down
9 changes: 3 additions & 6 deletions docs/fundamentals/systems/overlays/rationale.md
Original file line number Diff line number Diff line change
Expand Up @@ -222,11 +222,8 @@ global(modal) dialog.

## Considerations

### Future for mode local (Popper)
We use the native dialog purely because it renders to the "top layer". Not because of its accessible role, which varies per component. In the future we can use the popover attribute(<https://open-ui.org/components/popover.research.explainer/>) for this (which neatly differentiates between presentation and semantics).

- Default overflow and/or max-width behavior when content is too wide or high for the viewport.
So in our use of the dialog we want also a dialog that only does presentation (rendering on top layer in this case), but we want to check the semantics ourselves. Hence: `role="none"`. Even though the outcome is completely accessible, AXE-core uses this rule as a kind of "precautionary measure".

### Aria roles

- No `aria-controls` as support for it is not quite there yet
- No `aria-haspopup`. People knowing the haspop up and hear about it don’t expect a dialog to open (at this moment in time) but expect a sub-menu. Until support for the dialog value has better implementation, it’s probably best to not use aria-haspopup on the element that opens the modal dialog.
After a cost/benefit analysis, we choose to be temporarily incompatible with AXE-core. Therefore, we recommend running tests with `{ ignoredRules: ['aria-allowed-role'] }`. When the popover is stable and used underwater in lion overlays, AXE can again be run without this ignoredRule.
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ export function runListboxMixinSuite(customConfig = {}) {
`);
await el.updateComplete;
await el.updateComplete; // need 2 awaits as overlay.show is an async function

// for more info about why we need the ignoreRules, see: https://lion-web.netlify.app/fundamentals/systems/overlays/rationale/#considerations
await expect(el).to.be.accessible({ ignoredRules: ['aria-allowed-role'] });
});

Expand All @@ -405,6 +405,7 @@ export function runListboxMixinSuite(customConfig = {}) {
<${optionTag} .choiceValue=${20}>Item 2</${optionTag}>
</${tag}>
`);
// for more info about why we need the ignoreRules, see: https://lion-web.netlify.app/fundamentals/systems/overlays/rationale/#considerations
await expect(el).to.be.accessible({ ignoredRules: ['aria-allowed-role'] });
});

Expand Down
4 changes: 4 additions & 0 deletions packages/ui/components/overlays/src/OverlayController.js
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,10 @@ export class OverlayController extends EventTarget {
// We use a dialog for its visual capabilities: it renders to the top layer.
// A11y will depend on the type of overlay and is arranged on contentNode level.
// Also see: https://www.scottohara.me/blog/2019/03/05/open-dialog.html
//
// The role="dialog" is set on the contentNode (or another role), so role="none"
// is valid here, although AXE complains about this setup.
// For now we need to add `ignoredRules: ['aria-allowed-role']` in your AXE tests.
wrappingDialogElement.setAttribute('role', 'none');
wrappingDialogElement.setAttribute('data-overlay-outer-wrapper', '');
// N.B. position: fixed is needed to escape out of 'overflow: hidden'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ export class LionSelectInvoker extends SlotMixin(LionButton) {

connectedCallback() {
super.connectedCallback();
this.setAttribute('role', 'combobox');
this.addEventListener('keydown', this.__handleKeydown);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/ui/components/select-rich/src/LionSelectRich.js
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ export class LionSelectRich extends SlotMixin(ScopedElementsMixin(OverlayMixin(L
__setupInvokerNode() {
this._invokerNode.id = `invoker-${this._inputId}`;
this._invokerNode.setAttribute('aria-haspopup', 'listbox');

this._invokerNode.setAttribute('aria-controls', this._inputId);
this.__setupInvokerNodeEventListener();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { LionSelectInvoker } from '@lion/ui/select-rich.js';
import '@lion/ui/define/lion-select-invoker.js';

/**
* @typedef {import('../../listbox/src/LionOption.js').LionOption} LionOption
* @typedef {import('@lion/ui/listbox.js').LionOption} LionOption
*/

describe('lion-select-invoker', () => {
Expand Down Expand Up @@ -100,4 +100,13 @@ describe('lion-select-invoker', () => {
expect(el._contentWrapperNode).lightDom.to.equal('no valid selection');
});
});

describe('Accessibility', () => {
it('passes axe a11y audit', async () => {
const el = /** @type {LionSelectInvoker} */ (
await fixture(html`<lion-select-invoker></lion-select-invoker>`)
);
await expect(el.role).to.equal('combobox');
});
});
});

0 comments on commit 776686f

Please sign in to comment.