Skip to content

fix(overlay and picker): remove aria-hidden attribute #30553

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

Open
wants to merge 14 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion core/src/components/datetime/datetime.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { Component, Element, Event, Host, Method, Prop, State, Watch, h, writeTa
import { startFocusVisible } from '@utils/focus-visible';
import { getElementRoot, raf, renderHiddenInput } from '@utils/helpers';
import { printIonError, printIonWarning } from '@utils/logging';
import { FOCUS_TRAP_DISABLE_CLASS } from '@utils/overlays';
import { isRTL } from '@utils/rtl';
import { createColorClasses } from '@utils/theme';
import { caretDownSharp, caretUpSharp, chevronBack, chevronDown, chevronForward } from 'ionicons/icons';
Expand Down Expand Up @@ -1606,7 +1607,7 @@ export class Datetime implements ComponentInterface {
forcePresentation === 'time-date'
? [this.renderTimePickerColumns(forcePresentation), this.renderDatePickerColumns(forcePresentation)]
: [this.renderDatePickerColumns(forcePresentation), this.renderTimePickerColumns(forcePresentation)];
return <ion-picker>{renderArray}</ion-picker>;
return <ion-picker class={FOCUS_TRAP_DISABLE_CLASS}>{renderArray}</ion-picker>;
}

private renderDatePickerColumns(forcePresentation: string) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// Picker Column
// --------------------------------------------------

button {
.picker-column-option-button {
@include padding(0);
@include margin(0);

Expand Down Expand Up @@ -40,6 +40,6 @@ button {
opacity: 0.4;
}

:host(.option-disabled) button {
:host(.option-disabled) .picker-column-option-button {
cursor: default;
}
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,9 @@ export class PickerColumnOption implements ComponentInterface {
['option-disabled']: disabled,
})}
>
<button tabindex="-1" aria-label={ariaLabel} disabled={disabled} onClick={() => this.onClick()}>
<div class={'picker-column-option-button'} role="button" aria-label={ariaLabel} onClick={() => this.onClick()}>
<slot></slot>
</button>
</div>
</Host>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { newSpecPage } from '@stencil/core/testing';
import { PickerColumnOption } from '../picker-column-option';

describe('picker column option', () => {
it('button should be enabled by default', async () => {
it('should be enabled by default', async () => {
const page = await newSpecPage({
components: [PickerColumnOption],
html: `
Expand All @@ -12,12 +12,11 @@ describe('picker column option', () => {
});

const option = page.body.querySelector('ion-picker-column-option')!;
const button = option.shadowRoot!.querySelector('button')!;

await expect(button.hasAttribute('disabled')).toEqual(false);
await expect(option.classList.contains('option-disabled')).toEqual(false);
});

it('button should be disabled if specified', async () => {
it('should be disabled if specified', async () => {
const page = await newSpecPage({
components: [PickerColumnOption],
html: `
Expand All @@ -26,8 +25,7 @@ describe('picker column option', () => {
});

const option = page.body.querySelector('ion-picker-column-option')!;
const button = option.shadowRoot!.querySelector('button')!;

await expect(button.hasAttribute('disabled')).toEqual(true);
await expect(option.classList.contains('option-disabled')).toEqual(true);
});
});
63 changes: 9 additions & 54 deletions core/src/components/picker-column/picker-column.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -654,39 +654,6 @@ export class PickerColumn implements ComponentInterface {
return el ? el.getAttribute('aria-label') ?? el.innerText : '';
};

/**
* Render an element that overlays the column. This element is for assistive
* tech to allow users to navigate the column up/down. This element should receive
* focus as it listens for synthesized keyboard events as required by the
* slider role: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/slider_role
*/
private renderAssistiveFocusable = () => {
const { activeItem } = this;
const valueText = this.getOptionValueText(activeItem);

/**
* When using the picker, the valuetext provides important context that valuenow
* does not. Additionally, using non-zero valuemin/valuemax values can cause
* WebKit to incorrectly announce numeric valuetext values (such as a year
* like "2024") as percentages: https://bugs.webkit.org/show_bug.cgi?id=273126
*/
return (
<div
ref={(el) => (this.assistiveFocusable = el)}
class="assistive-focusable"
role="slider"
tabindex={this.disabled ? undefined : 0}
aria-label={this.ariaLabel}
aria-valuemin={0}
aria-valuemax={0}
aria-valuenow={0}
aria-valuetext={valueText}
aria-orientation="vertical"
onKeyDown={(ev) => this.onKeyDown(ev)}
></div>
);
};

render() {
const { color, disabled, isActive, numericInput } = this;
const theme = getIonTheme(this);
Expand All @@ -700,33 +667,21 @@ export class PickerColumn implements ComponentInterface {
['picker-column-disabled']: disabled,
})}
>
{this.renderAssistiveFocusable()}
<slot name="prefix"></slot>
<div
aria-hidden="true"
class="picker-opts"
ref={(el) => {
this.scrollEl = el;
}}
/**
* When an element has an overlay scroll style and
* a fixed height, Firefox will focus the scrollable
* container if the content exceeds the container's
* dimensions.
*
* This causes keyboard navigation to focus to this
* element instead of going to the next element in
* the tab order.
*
* The desired behavior is for the user to be able to
* focus the assistive focusable element and tab to
* the next element in the tab order. Instead of tabbing
* to this element.
*
* To prevent this, we set the tabIndex to -1. This
* will match the behavior of the other browsers.
*/
tabIndex={-1}
role="slider"
tabindex={this.disabled ? undefined : 0}
aria-label={this.ariaLabel}
aria-valuemin={0}
aria-valuemax={0}
aria-valuenow={0}
aria-valuetext={this.getOptionValueText(this.activeItem)}
aria-orientation="vertical"
onKeyDown={(ev) => this.onKeyDown(ev)}
>
<div class="picker-item-empty" aria-hidden="true">
&nbsp;
Expand Down
32 changes: 16 additions & 16 deletions core/src/components/picker-column/test/picker-column.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { h } from '@stencil/core';
import { newSpecPage } from '@stencil/core/testing';

import { PickerColumn } from '../picker-column';
import { PickerColumnOption } from '../../picker-column-option/picker-column-option';
import { PickerColumn } from '../picker-column';

describe('picker-column: assistive element', () => {
describe('picker-column', () => {
beforeEach(() => {
const mockIntersectionObserver = jest.fn();
mockIntersectionObserver.mockReturnValue({
Expand All @@ -22,9 +22,9 @@ describe('picker-column: assistive element', () => {
});

const pickerCol = page.body.querySelector('ion-picker-column')!;
const assistiveFocusable = pickerCol.shadowRoot!.querySelector('.assistive-focusable')!;
const pickerOpts = pickerCol.shadowRoot!.querySelector('.picker-opts')!;

expect(assistiveFocusable.getAttribute('aria-label')).not.toBe(null);
expect(pickerOpts.getAttribute('aria-label')).not.toBe(null);
});

it('should have a custom label', async () => {
Expand All @@ -34,9 +34,9 @@ describe('picker-column: assistive element', () => {
});

const pickerCol = page.body.querySelector('ion-picker-column')!;
const assistiveFocusable = pickerCol.shadowRoot!.querySelector('.assistive-focusable')!;
const pickerOpts = pickerCol.shadowRoot!.querySelector('.picker-opts')!;

expect(assistiveFocusable.getAttribute('aria-label')).toBe('my label');
expect(pickerOpts.getAttribute('aria-label')).toBe('my label');
});

it('should update a custom label', async () => {
Expand All @@ -46,12 +46,12 @@ describe('picker-column: assistive element', () => {
});

const pickerCol = page.body.querySelector('ion-picker-column')!;
const assistiveFocusable = pickerCol.shadowRoot!.querySelector('.assistive-focusable')!;
const pickerOpts = pickerCol.shadowRoot!.querySelector('.picker-opts')!;

pickerCol.setAttribute('aria-label', 'my label');
await page.waitForChanges();

expect(assistiveFocusable.getAttribute('aria-label')).toBe('my label');
expect(pickerOpts.getAttribute('aria-label')).toBe('my label');
});

it('should receive keyboard focus when enabled', async () => {
Expand All @@ -61,9 +61,9 @@ describe('picker-column: assistive element', () => {
});

const pickerCol = page.body.querySelector('ion-picker-column')!;
const assistiveFocusable = pickerCol.shadowRoot!.querySelector<HTMLElement>('.assistive-focusable')!;
const pickerOpts = pickerCol.shadowRoot!.querySelector<HTMLElement>('.picker-opts')!;

expect(assistiveFocusable.tabIndex).toBe(0);
expect(pickerOpts.tabIndex).toBe(0);
});

it('should not receive keyboard focus when disabled', async () => {
Expand All @@ -73,9 +73,9 @@ describe('picker-column: assistive element', () => {
});

const pickerCol = page.body.querySelector('ion-picker-column')!;
const assistiveFocusable = pickerCol.shadowRoot!.querySelector<HTMLElement>('.assistive-focusable')!;
const pickerOpts = pickerCol.shadowRoot!.querySelector<HTMLElement>('.picker-opts')!;

expect(assistiveFocusable.tabIndex).toBe(-1);
expect(pickerOpts.tabIndex).toBe(-1);
});

it('should use option aria-label as assistive element aria-valuetext', async () => {
Expand All @@ -91,9 +91,9 @@ describe('picker-column: assistive element', () => {
});

const pickerCol = page.body.querySelector('ion-picker-column')!;
const assistiveFocusable = pickerCol.shadowRoot!.querySelector('.assistive-focusable')!;
const pickerOpts = pickerCol.shadowRoot!.querySelector('.picker-opts')!;

expect(assistiveFocusable.getAttribute('aria-valuetext')).toBe('My Label');
expect(pickerOpts.getAttribute('aria-valuetext')).toBe('My Label');
});

it('should use option text as assistive element aria-valuetext when no label provided', async () => {
Expand All @@ -107,8 +107,8 @@ describe('picker-column: assistive element', () => {
});

const pickerCol = page.body.querySelector('ion-picker-column')!;
const assistiveFocusable = pickerCol.shadowRoot!.querySelector('.assistive-focusable')!;
const pickerOpts = pickerCol.shadowRoot!.querySelector('.picker-opts')!;

expect(assistiveFocusable.getAttribute('aria-valuetext')).toBe('My Text');
expect(pickerOpts.getAttribute('aria-valuetext')).toBe('My Text');
});
});
58 changes: 56 additions & 2 deletions core/src/components/picker/picker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,63 @@ export class Picker implements ComponentInterface {
* function that has been set in onPointerDown
* so that we enter/exit input mode correctly.
*/
private onClick = () => {
private onClick = (ev: PointerEvent) => {
const { actionOnClick } = this;
if (actionOnClick) {
actionOnClick();
this.actionOnClick = undefined;
}

/**
* In order to avoid a11y issues we must manage focus
* on the picker columns and picker itself.
* This is because once picker is clicked we got an issue/warning because
* picker input is being focused, and once it has tabindex -1 it can't be focused,
* which ends on focusing the picker itself.
* During the process above we fall into issues since there is an element
* with tabindex -1 and aria-hidden='true' that is focused, which is not allowed.
* That said and since onClick is being propagated to the picker itself, we need to
* manage focus on the picker columns and picker itself to avoid the issue.
*/
const clickedTarget = ev.target as HTMLElement;
let elementToFocus: HTMLElement | null = null;

switch (clickedTarget.tagName) {
case 'ION-PICKER':
/**
* If the user clicked the picker itself
* then we should focus the first picker options
* so that users can scroll through them.
*/
const ionPickerColumn = this.el.querySelector('ion-picker-column');
elementToFocus = ionPickerColumn?.shadowRoot?.querySelector('.picker-opts') as HTMLElement | null;
break;

case 'ION-PICKER-COLUMN':
/**
* If the user clicked a picker column
* then we should focus its own picker options
* so that users can scroll through them.
*/
elementToFocus = clickedTarget.shadowRoot?.querySelector('.picker-opts') as HTMLElement | null;
break;

case 'ION-PICKER-COLUMN-OPTION':
/**
* If the user clicked a picker column option
* then we should focus its picker options parent so that
* users can scroll through them.
*/
const ionPickerColumnOption = clickedTarget.closest('ion-picker-column');
if (ionPickerColumnOption) {
elementToFocus = ionPickerColumnOption.shadowRoot?.querySelector('.picker-opts') as HTMLElement | null;
}
break;
}

if (elementToFocus) {
elementToFocus.focus();
}
};

/**
Expand Down Expand Up @@ -537,7 +588,10 @@ export class Picker implements ComponentInterface {

render() {
return (
<Host onPointerDown={(ev: PointerEvent) => this.onPointerDown(ev)} onClick={() => this.onClick()}>
<Host
onPointerDown={(ev: PointerEvent) => this.onPointerDown(ev)}
onClick={(ev: PointerEvent) => this.onClick(ev)}
>
<input
aria-hidden="true"
tabindex={-1}
Expand Down
Loading
Loading