Skip to content
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
63 changes: 40 additions & 23 deletions core/src/components/button/button.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import type { ComponentInterface, EventEmitter } from '@stencil/core';
import { Component, Element, Event, Host, Prop, Watch, State, forceUpdate, h } from '@stencil/core';
import type { AnchorInterface, ButtonInterface } from '@utils/element-interface';
import type { Attributes } from '@utils/helpers';
import { inheritAriaAttributes, hasShadowDom } from '@utils/helpers';
import {
inheritAriaAttributes,
hasShadowDom,
watchForAriaAttributeChanges,
type AttributeWatcher,
type Attributes,
} from '@utils/helpers';
import { printIonWarning } from '@utils/logging';
import { createColorClasses, hostContext, openURL } from '@utils/theme';

Expand Down Expand Up @@ -35,6 +40,8 @@ export class Button implements ComponentInterface, AnchorInterface, ButtonInterf
private formButtonEl: HTMLButtonElement | null = null;
private formEl: HTMLFormElement | null = null;
private inheritedAttributes: Attributes = {};
private didLoad = false;
private ariaWatcher?: AttributeWatcher;

@Element() el!: HTMLElement;

Expand Down Expand Up @@ -158,27 +165,6 @@ export class Button implements ComponentInterface, AnchorInterface, ButtonInterf
*/
@Event() ionBlur!: EventEmitter<void>;

/**
* This component is used within the `ion-input-password-toggle` component
* to toggle the visibility of the password input.
* These attributes need to update based on the state of the password input.
* Otherwise, the values will be stale.
*
* @param newValue
* @param _oldValue
* @param propName
*/
@Watch('aria-checked')
@Watch('aria-label')
@Watch('aria-pressed')
onAriaChanged(newValue: string, _oldValue: string, propName: string) {
this.inheritedAttributes = {
...this.inheritedAttributes,
[propName]: newValue,
};
forceUpdate(this);
}

/**
* This is responsible for rendering a hidden native
* button element inside the associated form. This allows
Expand Down Expand Up @@ -223,6 +209,37 @@ export class Button implements ComponentInterface, AnchorInterface, ButtonInterf
this.inheritedAttributes = inheritAriaAttributes(this.el);
}

connectedCallback() {
Comment thread
Zac-Smucker-Bryan marked this conversation as resolved.
// Only run the initial snapshot once. On subsequent reconnects the
// host has already been stripped, so inheritAriaAttributes would
// return {} and overwrite previously captured values.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment matches the code now. There's no snapshot in connectedCallback and no call to inheritAriaAttributes, so it reads like a leftover from the revision that did move the inherit here. The reason for the gate is worth writing down in its place: inheritAttributes calls removeAttribute, so arming the observer any earlier would have it see the strip as a removal.

Stray blank line between the comment and the if as well, and the same comment is in card.tsx, which doesn't even import inheritAriaAttributes.


if (this.didLoad) {
this.startAriaWatcher();
}
}

componentDidLoad() {
this.didLoad = true;
this.startAriaWatcher();
}

disconnectedCallback() {
this.ariaWatcher?.destroy();
this.ariaWatcher = undefined;
}

private startAriaWatcher() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The didLoad and ariaWatcher fields plus this method and the three lifecycle hooks are near-identical across all three components, about thirty lines each. Each one re-deriving the arming rule for itself is, I think, how the card and item watch sets ended up wrong. I'd like this folded into a controller in the style of createSlotMutationController, but that's a bigger change than the rest of this and I'm happy to take it on rather than push it back to you.

One thing worth keeping though: the JSDoc on the @Watch block this replaces was the only record we had that ion-button needs live ARIA because ion-input-password-toggle re-renders aria-label and aria-pressed on it. A line of that here would stop a future refactor quietly dropping the watcher. The toggle still works, for what it's worth, all six of its a11y tests pass on this branch.

this.ariaWatcher = watchForAriaAttributeChanges(
Comment thread
Zac-Smucker-Bryan marked this conversation as resolved.
this.el,
(changed) => {
this.inheritedAttributes = { ...this.inheritedAttributes, ...changed };
forceUpdate(this);
},
['aria-disabled']
);
}

private get hasIconOnly() {
return !!this.el.querySelector('[slot="icon-only"]');
}
Expand Down
111 changes: 111 additions & 0 deletions core/src/components/button/test/a11y/button.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,3 +148,114 @@ configs({ directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
});
});
});

configs({ directions: ['ltr'] }).forEach(({ title, config }) => {
test.describe(title('button: aria attribute sync'), () => {
const watchedAriaAttributes = ['aria-checked', 'aria-label', 'aria-pressed', 'aria-description'];

for (const attr of watchedAriaAttributes) {
test(`native button updates ${attr} when host attribute changes`, async ({ page }) => {
Comment thread
Zac-Smucker-Bryan marked this conversation as resolved.
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/30626',
});

await page.setContent(`<ion-button ${attr}="initial">Button</ion-button>`, config);

const host = page.locator('ion-button');
const nativeButton = host.locator('button');

await expect(nativeButton).toHaveAttribute(attr, 'initial');

await host.evaluate((el, attr) => el.setAttribute(attr, 'updated'), attr);

await expect(nativeButton).toHaveAttribute(attr, 'updated');
});
}

test('should not sync aria-disabled from the host', async ({ page }) => {
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/30626',
});

await page.setContent(`<ion-button aria-disabled="true">Button</ion-button>`, config);

const host = page.locator('ion-button');
const nativeButton = host.locator('button');

// Initial inheritance moves the developer-provided value to native.
// The host's aria-disabled is subsequently owned by the disabled prop.
await expect(host).not.toHaveAttribute('aria-disabled');
await expect(nativeButton).toHaveAttribute('aria-disabled', 'true');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one passes with the source reverted to main too. It only checks the initial inheritance, which main already did, and the aria-disabled ignore list only affects post-load mutations, so you could delete the ignore list entirely and nothing here would notice. Setting aria-disabled on the host after load and asserting the native value doesn't change would cover it.

The name reads a bit odd too, since the assertion is that the value does get inherited at load.

});

test('preserves inherited aria-label after detach and reattach', async ({ page }) => {
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/30626',
});

await page.setContent(
`
<div id="container">
<ion-button aria-label="label">Button</ion-button>
</div>
`,
config
);

const host = page.locator('ion-button');
const nativeButton = host.locator('button');

await expect(nativeButton).toHaveAttribute('aria-label', 'label');

// Detach, reattach, and force a render via a prop change.
await host.evaluate((el) => {
const parent = el.parentElement!;
parent.removeChild(el);
parent.appendChild(el);
(el as HTMLIonButtonElement).color = 'primary';
});

// Assert the original value survived
await expect(nativeButton).toHaveAttribute('aria-label', 'label');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this can fail. Reverting the four source files to main and keeping the specs, this one still passes, along with the card and item copies. The value comes from inheritedAttributes, which is instance state, and componentWillLoad doesn't run again on a move, so it survives whether or not there's a watcher.

That means the connectedCallback re-arm has no coverage: deleting startAriaWatcher() from connectedCallback in all three components fails nothing. Setting aria-label to a new value after the reattach and asserting the native element picks it up would cover what I was after in the earlier thread.

});

test('syncs aria-label updates and removal after initial inheritance', async ({ page }) => {
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/30626',
});

await page.setContent(
`
<ion-button aria-label="initial">Button</ion-button>
`,
config
);

const host = page.locator('ion-button');
const nativeButton = host.locator('button');

// Initial inheritance moves the value from the host to the native button.
await expect(host).not.toHaveAttribute('aria-label');
await expect(nativeButton).toHaveAttribute('aria-label', 'initial');

// Post-load writes remain on the host and are synchronized to native
await host.evaluate((el) => el.setAttribute('aria-label', 'second'));
await expect(host).toHaveAttribute('aria-label');
await expect(nativeButton).toHaveAttribute('aria-label', 'second');

// An empty string is a valid ARIA attribute value and remains synchronized.
await host.evaluate((el) => el.setAttribute('aria-label', ''));
await expect(host).toHaveAttribute('aria-label');
await expect(nativeButton).toHaveAttribute('aria-label', '');

// Native MutationObserver behavior sees a real removal after a post-load write.
await host.evaluate((el) => el.removeAttribute('aria-label'));
await expect(host).not.toHaveAttribute('aria-label');
await expect(nativeButton).not.toHaveAttribute('aria-label');
});
});
});
43 changes: 40 additions & 3 deletions core/src/components/card/card.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
import type { ComponentInterface } from '@stencil/core';
import { Element, Component, Host, Prop, h } from '@stencil/core';
import { Element, Component, Host, Prop, h, forceUpdate } from '@stencil/core';
import type { AnchorInterface, ButtonInterface } from '@utils/element-interface';
import type { Attributes } from '@utils/helpers';
import { inheritAttributes } from '@utils/helpers';
import {
inheritAttributes,
watchForAriaAttributeChanges,
type AttributeWatcher,
type Attributes,
} from '@utils/helpers';
import { createColorClasses, openURL } from '@utils/theme';

import { getIonMode } from '../../global/ionic-global';
Expand All @@ -24,6 +28,8 @@ import type { RouterDirection } from '../router/utils/interface';
})
export class Card implements ComponentInterface, AnchorInterface, ButtonInterface {
private inheritedAriaAttributes: Attributes = {};
private didLoad = false;
private ariaWatcher?: AttributeWatcher;

@Element() el!: HTMLElement;
/**
Expand Down Expand Up @@ -91,6 +97,37 @@ export class Card implements ComponentInterface, AnchorInterface, ButtonInterfac
this.inheritedAriaAttributes = inheritAttributes(this.el, ['aria-label']);
}

connectedCallback() {
Comment thread
Zac-Smucker-Bryan marked this conversation as resolved.
// Only run the initial snapshot once. On subsequent reconnects the
// host has already been stripped, so inheritAriaAttributes would
// return {} and overwrite previously captured values.

if (this.didLoad) {
this.startAriaWatcher();
}
}

componentDidLoad() {
this.didLoad = true;
this.startAriaWatcher();
}

disconnectedCallback() {
this.ariaWatcher?.destroy();
this.ariaWatcher = undefined;
}

private startAriaWatcher() {
this.ariaWatcher = watchForAriaAttributeChanges(
this.el,
(changed) => {
this.inheritedAriaAttributes = { ...this.inheritedAriaAttributes, ...changed };
forceUpdate(this);
},
['aria-disabled']

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same ignore-list issue as the item watcher, and the same fix. Card has no role expression on its Host so that one doesn't apply here, but aria-describedby and aria-hidden still reach the native element after load when neither does at load.

Two small things while you're here. Card never renders aria-disabled on its Host and never inherits it, so that ignore entry isn't guarding anything. And a card with no button and no href renders no native element at all, so on a plain ion-card the watcher runs with nowhere to put its output. Worth gating on isClickable().

);
}

private isClickable(): boolean {
return this.href !== undefined || this.button;
}
Expand Down
94 changes: 94 additions & 0 deletions core/src/components/card/test/a11y/card.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,97 @@ configs({ directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
});
});
});

configs({ directions: ['ltr'] }).forEach(({ title, config }) => {
test.describe(title('item: aria attribute sync'), () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
test.describe(title('item: aria attribute sync'), () => {
test.describe(title('card: aria attribute sync'), () => {

Looks like this one survived the copy/paste cleanup from the earlier pass. It collides exactly with the item spec's describe block, so both come through as item: in the report.

Same file, the detach/reattach test still names its evaluate arg itemEl and casts an ion-card to HTMLIonButtonElement, which only compiles because all three elements happen to have color. Item spec has the same cast.

test('native element updates aria-label when host attribute changes', async ({ page }) => {
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/30626',
});

await page.setContent(
`
<ion-card button="true" aria-label="label">Card</ion-card>
`,
config
);

const host = page.locator('ion-card');
const nativeCard = host.locator('[part="native"]');

await expect(nativeCard).toHaveAttribute('aria-label', 'label');

await host.evaluate((el) => el.setAttribute('aria-label', 'updated'));

await expect(nativeCard).toHaveAttribute('aria-label', 'updated');
});

test('preserves inherited aria-label after detach and reattach', async ({ page }) => {
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/30626',
});

await page.setContent(
`
<div id="container">
<ion-card button="true" aria-label="label">Card</ion-card>
</div>
`,
config
);

const host = page.locator('ion-card');
const nativeCard = host.locator('[part="native"]');

await expect(nativeCard).toHaveAttribute('aria-label', 'label');

// Detach, reattach, and force a render via a prop change.
await host.evaluate((itemEl) => {
const parent = itemEl.parentElement!;
parent.removeChild(itemEl);
parent.appendChild(itemEl);
(itemEl as HTMLIonButtonElement).color = 'primary';
});

// Assert the original value survived
await expect(nativeCard).toHaveAttribute('aria-label', 'label');
});

test('syncs aria-label updates and removal after initial inheritance', async ({ page }) => {
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/30626',
});
await page.setContent(
`
<ion-card button="true" aria-label="initial">Card</ion-card>
`,
config
);

const host = page.locator('ion-card');
const nativeCard = host.locator('[part="native"]');

// Initial inheritance moves the value from the host to the native button.
await expect(host).not.toHaveAttribute('aria-label');
await expect(nativeCard).toHaveAttribute('aria-label', 'initial');

// Post-load writes remain on the host and are synchronized to native
await host.evaluate((el) => el.setAttribute('aria-label', 'second'));
await expect(host).toHaveAttribute('aria-label');
await expect(nativeCard).toHaveAttribute('aria-label', 'second');

// An empty string is a valid ARIA attribute value and remains synchronized.
await host.evaluate((el) => el.setAttribute('aria-label', ''));
await expect(host).toHaveAttribute('aria-label');
await expect(nativeCard).toHaveAttribute('aria-label', '');

// Native MutationObserver behavior sees a real removal after a post-load write.
await host.evaluate((el) => el.removeAttribute('aria-label'));
await expect(host).not.toHaveAttribute('aria-label');
await expect(nativeCard).not.toHaveAttribute('aria-label');
});
});
});
Loading
Loading