Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(accordion): prevent opening of readonly accordion using keyboard #28865

Merged
merged 4 commits into from Jan 24, 2024
Merged

fix(accordion): prevent opening of readonly accordion using keyboard #28865

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a00ac3
fix(accordion): prevent opening of readonly accordion using keyboard
mapsandapps Jan 22, 2024
555dc90
fixup: remove notes to self
mapsandapps Jan 22, 2024
f24eee5
fixup: remove temporary element
mapsandapps Jan 22, 2024
a504fe7
add a note about why these aren't spec tests
mapsandapps Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion core/src/components/accordion/accordion.tsx
View file
Open in desktop
Expand Up @@ -382,7 +382,10 @@ export class Accordion implements ComponentInterface {
};

private toggleExpanded() {
const { accordionGroupEl, value, state } = this;
const { accordionGroupEl, disabled, readonly, value, state } = this;

if (disabled || readonly) return;

if (accordionGroupEl) {
/**
* Because the accordion group may or may
Expand Down
49 changes: 0 additions & 49 deletions core/src/components/accordion/test/accordion.e2e.ts
View file
Open in desktop

This file was deleted.

136 changes: 136 additions & 0 deletions core/src/components/accordion/test/disabled/accordion.e2e.ts
View file
Open in desktop
@@ -0,0 +1,136 @@
import { expect } from '@playwright/test';
import { configs, test } from '@utils/test/playwright';

configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ config, title }) => {
test.describe(title('accordion: disabled'), () => {
test('should properly set disabled on child accordions', async ({ page }) => {
await page.setContent(
`
<ion-accordion-group animated="false">
<ion-accordion>
<ion-item slot="header">Label</ion-item>
<div slot="content">Content</div>
</ion-accordion>
</ion-accordion-group>
`,
config
);

const accordionGroup = page.locator('ion-accordion-group');
const accordion = page.locator('ion-accordion');

await expect(accordion).toHaveJSProperty('disabled', false);

await accordionGroup.evaluate((el: HTMLIonAccordionGroupElement) => {
el.disabled = true;
});

await page.waitForChanges();

await expect(accordion).toHaveJSProperty('disabled', true);
});

test('should not open accordion on click when group is disabled', async ({ page }) => {
await page.setContent(
`
<ion-accordion-group animated="false" disabled>
<ion-accordion>
<ion-item slot="header">Label</ion-item>
<div slot="content">Content</div>
</ion-accordion>
</ion-accordion-group>
`,
config
);

const accordion = page.locator('ion-accordion');

await expect(accordion).toHaveClass(/accordion-collapsed/);

accordion.click();
await page.waitForChanges();

await expect(accordion).toHaveClass(/accordion-collapsed/);
});

test('should not open accordion on click when accordion is disabled', async ({ page }) => {
await page.setContent(
`
<ion-accordion-group animated="false">
<ion-accordion disabled>
<ion-item slot="header">Label</ion-item>
<div slot="content">Content</div>
</ion-accordion>
</ion-accordion-group>
`,
config
);

const accordion = page.locator('ion-accordion');

await expect(accordion).toHaveClass(/accordion-collapsed/);

accordion.click();
await page.waitForChanges();

await expect(accordion).toHaveClass(/accordion-collapsed/);
});

test('should not open accordion via keyboard navigation when group is disabled', async ({ page, browserName }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe pressing "Enter" on an interactive element will also dispatch a click event, so testing that the accordion does not expand on click should be sufficient here (i.e. we don't need to test click and Enter)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not what I'm seeing. If I revert the bugfix I made in this PR and run the tests, the "should not open accordion via keyboard navigation when accordion is readonly" test fails, and no other tests fail. This is what we expect, since this is the one case this PR addresses. The other tests I added are for behavior that was already correct, but didn't have test coverage.

If the keyboard & mouse tests are redundant with each other, I would expect either both to pass or both to fail in any situation.

Put another way: if I removed the keyboard tests (which I think is what you're suggesting), all the tests would pass without my bugfix being applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes I see what you are saying! In main all we do is set pointer-events: none on the header which prevents click events. But we never disable the underlying element, so you can still tab to an ion-item and then hit "Enter".

Keeping these tests make sense 👍 Thanks for clarifying

await page.setContent(
`
<ion-accordion-group animated="false" disabled>
<ion-accordion>
<ion-item slot="header">Label</ion-item>
<div slot="content">Content</div>
</ion-accordion>
</ion-accordion-group>
`,
config
);

const accordion = page.locator('ion-accordion');
const tabKey = browserName === 'webkit' ? 'Alt+Tab' : 'Tab';

await expect(accordion).toHaveClass(/accordion-collapsed/);

await page.keyboard.press(tabKey);
await page.waitForChanges();

await page.keyboard.press('Enter');
await page.waitForChanges();

await expect(accordion).toHaveClass(/accordion-collapsed/);
});

test('should not open accordion via keyboard navigation when accordion is disabled', async ({
liamdebeasi marked this conversation as resolved.
Show resolved Hide resolved
page,
browserName,
}) => {
await page.setContent(
`
<ion-accordion-group animated="false">
<ion-accordion disabled>
<ion-item slot="header">Label</ion-item>
<div slot="content">Content</div>
</ion-accordion>
</ion-accordion-group>
`,
config
);

const accordion = page.locator('ion-accordion');
const tabKey = browserName === 'webkit' ? 'Alt+Tab' : 'Tab';

await expect(accordion).toHaveClass(/accordion-collapsed/);

await page.keyboard.press(tabKey);
await page.waitForChanges();

await page.keyboard.press('Enter');
await page.waitForChanges();

await expect(accordion).toHaveClass(/accordion-collapsed/);
});
});
});
91 changes: 91 additions & 0 deletions core/src/components/accordion/test/disabled/index.html
View file
Open in desktop
@@ -0,0 +1,91 @@
<!DOCTYPE html>
<html lang="en" dir="ltr">
<head>
<meta charset="UTF-8" />
<title>Accordion - Disabled</title>
<meta name="viewport" content="width=device-width, initial-scale=1.0, minimum-scale=1.0" />
<link href="../../../../../css/ionic.bundle.css" rel="stylesheet" />
<link href="../../../../../scripts/testing/styles.css" rel="stylesheet" />
<script src="../../../../../scripts/testing/scripts.js"></script>
<script type="module" src="../../../../../dist/ionic/ionic.esm.js"></script>
<style>
.grid {
display: grid;
grid-template-columns: repeat(auto-fill, minmax(250px, 1fr));
grid-row-gap: 20px;
grid-column-gap: 20px;
}
</style>
</head>
<body>
<ion-app>
<ion-header translucent="true">
<ion-toolbar>
<ion-title>Accordion - Disabled</ion-title>
</ion-toolbar>
</ion-header>
<ion-content fullscreen="true" color="light">
<ion-header collapse="condense">
<ion-toolbar color="light">
<ion-title size="large">Accordion - Disabled</ion-title>
</ion-toolbar>
</ion-header>

<div class="grid ion-padding">
<div class="grid-item">
<ion-accordion-group>
<ion-accordion value="first">
<ion-item slot="header" color="light">
<ion-label>First Accordion</ion-label>
</ion-item>
<div class="ion-padding" slot="content">First Content</div>
</ion-accordion>
<ion-accordion value="second" disabled="true">
<ion-item slot="header" color="light">
<ion-label>Second Accordion (Disabled)</ion-label>
</ion-item>
<div class="ion-padding" slot="content">Second Content</div>
</ion-accordion>
<ion-accordion value="third">
<ion-item slot="header" color="light">
<ion-label>Third Accordion</ion-label>
</ion-item>
<div class="ion-padding" slot="content">Third Content</div>
</ion-accordion>
</ion-accordion-group>
</div>
<div class="grid-item">
<ion-accordion-group disabled="true">
<ion-accordion value="first">
<ion-item slot="header" color="light">
<ion-label>First Accordion in Disabled Group</ion-label>
</ion-item>
<div class="ion-padding" slot="content">First Content</div>
</ion-accordion>
<ion-accordion value="second">
<ion-item slot="header" color="light">
<ion-label>Second Accordion in Disabled Group</ion-label>
</ion-item>
<div class="ion-padding" slot="content">Second Content</div>
</ion-accordion>
<ion-accordion value="third">
<ion-item slot="header" color="light">
<ion-label>Third Accordion in Disabled Group</ion-label>
</ion-item>
<div class="ion-padding" slot="content">Third Content</div>
</ion-accordion>
</ion-accordion-group>
</div>
<div class="grid-item">
<ion-accordion value="second" disabled="true">
<ion-item slot="header" color="light">
<ion-label>Accordion Without Group (Disabled)</ion-label>
</ion-item>
<div class="ion-padding" slot="content">Second Content</div>
</ion-accordion>
</div>
</div>
</ion-content>
</ion-app>
</body>
</html>