Skip to content

fix(ripple-effect): ripple displays on click or touch #25102

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

Merged
merged 7 commits into from
Apr 12, 2022
Merged
15 changes: 12 additions & 3 deletions core/playwright.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,16 @@ const generateProjects = () => {
...project,
metadata: {
mode,
rtl: false
rtl: false,
_testing: true
}
});
projectsWithMetadata.push({
...project,
metadata: {
mode,
rtl: true
rtl: true,
_testing: true
}
});
});
Expand All @@ -72,7 +74,14 @@ const config: PlaywrightTestConfig = {
* Maximum time expect() should wait for the condition to be met.
* For example in `await expect(locator).toHaveText();`
*/
timeout: 5000
timeout: 5000,
toMatchSnapshot: {
/**
* Increases the maximum allowed pixel difference to account
* for slight browser rendering inconsistencies.
*/
maxDiffPixelRatio: 0.05
}
},
/* Fail the build on CI if you accidentally left test.only in the source code. */
forbidOnly: !!process.env.CI,
Expand Down
10 changes: 5 additions & 5 deletions core/src/components/ripple-effect/test/basic/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,18 @@

<ion-content class="ion-padding" no-bounce>
<p>
<ion-button size="small">Small</ion-button>
<ion-button id="small-btn" size="small">Small</ion-button>
</p>
<p>
<ion-button size="large">Large</ion-button>
<ion-button id="large-btn" size="large">Large</ion-button>
</p>
<p>
<ion-button size="large" fill="outline">Large</ion-button>
<ion-button id="large-btn-outline" size="large" fill="outline">Large</ion-button>
</p>
<p>
<ion-button size="large" fill="clear">Large</ion-button>
<ion-button id="large-btn-clear" size="large" fill="clear">Large</ion-button>
</p>
<div class="my-block ion-activatable">
<div class="my-block ion-activatable" id="ripple-with-button">
<ion-ripple-effect></ion-ripple-effect>
This is just a div + effect behind
<ion-button onclick="buttonClicked()">Nested button</ion-button>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import { expect } from '@playwright/test';
import type { IonicPage } from '@utils/test/playwright';
import { test } from '@utils/test/playwright';

test.describe('ripple-effect: basic', () => {
test('should add .ion-activated when pressed', async ({ page }) => {
await verifyRippleEffect(page, '#small-btn');
await verifyRippleEffect(page, '#large-btn');
await verifyRippleEffect(page, '#large-btn-outline');
await verifyRippleEffect(page, '#large-btn-clear');
await verifyRippleEffect(page, '.block');
});

test.describe('ripple effect with nested ion-button', () => {
test('should add .ion-activated when the block is pressed', async ({ page }) => {
await page.goto(`/src/components/ripple-effect/test/basic?ionic:_testing=false&ionic:mode=md`);

const el = page.locator('#ripple-with-button');

await el.scrollIntoViewIfNeeded();

const boundingBox = await el.boundingBox();

if (boundingBox) {
await page.mouse.move(boundingBox.x + 5, boundingBox.y + 5);
await page.mouse.down();
}

// Waits for the ripple effect to be added
await page.waitForSelector('.ion-activated');

const elHandle = await el.elementHandle();
const classes = await elHandle?.evaluate((el) => el.classList.value);
expect(classes).toMatch('ion-activated');
});

test('should add .ion-activated when the button is pressed', async ({ page }) => {
await verifyRippleEffect(page, '#ripple-with-button ion-button');
});
});
});

const verifyRippleEffect = async (page: IonicPage, selector: string) => {
await page.goto(`/src/components/ripple-effect/test/basic?ionic:_testing=false&ionic:mode=md`);

const el = page.locator(selector);

await el.scrollIntoViewIfNeeded();

const boundingBox = await el.boundingBox();

if (boundingBox) {
await page.mouse.move(boundingBox.x + boundingBox.width / 2, boundingBox.y + boundingBox.height / 2);
await page.mouse.down();
}

// Waits for the ripple effect to be added
await page.waitForSelector(`${selector}.ion-activated`);
Copy link
Contributor

Choose a reason for hiding this comment

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

If I check the button demo on https://ionicframework.com/docs, the button receives ion-activated for a brief moment on click, even with the busted ripple effect. Does the test fail with the broken state? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you possibly being served from a cache? I don't see the class added to the button element when clicking and holding on the button in the docs examples. Chrome also doesn't indicate that the class attribute was modified during mouse down.

Copy link
Contributor

Choose a reason for hiding this comment

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

Happens in incognito, screencast: https://user-images.githubusercontent.com/90629384/163048183-3adafc01-3551-43a5-b6e9-962630f7df91.mp4 The class is on there as long as I have the mouse button held down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strange that doesn't happen on Mac 🤔 https://user-images.githubusercontent.com/13732623/163049314-6e795818-897e-464b-ba0c-ba589f0bd4fe.mp4

Anyways, I confirmed that with the changes on main, the test suite fails. With the changes in this branch, the test suite passes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, that's what's important 👍


const elHandle = await el.elementHandle();
const classes = await elHandle?.evaluate((el) => el.classList.value);
expect(classes).toMatch('ion-activated');
};
5 changes: 3 additions & 2 deletions core/src/utils/tap-click.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ export const startTapClick = (config: Config) => {
const clearDefers = new WeakMap<HTMLElement, any>();

const isScrolling = () => {
return scrollingEl?.parentElement !== null;
// eslint-disable-next-line @typescript-eslint/prefer-optional-chain
return scrollingEl !== undefined && scrollingEl.parentElement !== null;
};

// Touch Events
Expand Down Expand Up @@ -169,7 +170,7 @@ const getActivatableTarget = (ev: any): any => {
const path = ev.composedPath() as HTMLElement[];
for (let i = 0; i < path.length - 2; i++) {
const el = path[i];
if (el?.classList.contains('ion-activatable')) {
if (!(el instanceof ShadowRoot) && el.classList.contains('ion-activatable')) {
return el;
}
}
Expand Down
5 changes: 3 additions & 2 deletions core/src/utils/test/playwright/fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export const test = base.extend<CustomFixtures>({
* to be hydrated before proceeding with the test.
*/
page.goto = async (url: string) => {
const { mode, rtl } = testInfo.project.metadata;
const { mode, rtl, _testing } = testInfo.project.metadata;

const splitUrl = url.split('?');
const paramsString = splitUrl[1];
Expand All @@ -55,8 +55,9 @@ export const test = base.extend<CustomFixtures>({
const urlToParams = new URLSearchParams(paramsString);
const formattedMode = urlToParams.get('ionic:mode') ?? mode;
const formattedRtl = urlToParams.get('rtl') ?? rtl;
const ionicTesting = urlToParams.get('ionic:_testing') ?? _testing;

const formattedUrl = `${splitUrl[0]}?ionic:_testing=true&ionic:mode=${formattedMode}&rtl=${formattedRtl}`;
const formattedUrl = `${splitUrl[0]}?ionic:_testing=${ionicTesting}&ionic:mode=${formattedMode}&rtl=${formattedRtl}`;

const results = await Promise.all([
page.waitForFunction(() => (window as any).testAppLoaded === true),
Expand Down