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
18 changes: 17 additions & 1 deletion core/src/components/modal/gestures/sheet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -452,12 +452,23 @@ export const createSheetGesture = (
const onEnd = (detail: GestureDetail) => {
const snapBreakpoint = calculateSnapBreakpoint(detail.deltaY);

/**
* `moveSheetToBreakpoint` only dismisses when the sheet snaps to
* breakpoint 0. When canDismiss blocks the gesture it keeps the sheet
* from snapping there and dismisses through `handleCanDismiss` instead,
* which only dismisses when canDismiss is a function. A canDismiss
* function can still cancel the dismiss after this event is emitted.
*/
const shouldPreventDismiss = canDismissBlocksGesture && snapBreakpoint === 0;
const isDismissing = shouldPreventDismiss ? typeof baseEl.canDismiss === 'function' : snapBreakpoint === 0;

const eventDetail: ModalDragEventDetail = {
currentY: detail.currentY,
deltaY: detail.deltaY,
velocityY: detail.velocityY,
progress: calculateProgress(detail.currentY),
snapBreakpoint,
isDismissing,
};

/**
Expand All @@ -474,7 +485,12 @@ export const createSheetGesture = (
* swap to moving on drag and if we don't swap back here then the footer will get stuck.
*/
swapFooterPosition('stationary');
onDragEnd(eventDetail);

/**
* The sheet does not move in this case, so the gesture
* never dismisses the modal.
*/
onDragEnd({ ...eventDetail, isDismissing: false });

return;
}
Expand Down
17 changes: 16 additions & 1 deletion core/src/components/modal/gestures/swipe-to-close.ts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Adding this variable changes the ionDragEnd detail length, so the existing assertion in modal-card.e2e.ts will fail. Please update the expected length there.

Original file line number Diff line number Diff line change
Expand Up @@ -304,17 +304,32 @@ export const createSwipeToCloseGesture = (
* check canDismiss. 25% was chosen
* to avoid accidental swipes.
*/
if (isAttemptingDismissWithCanDismiss && clampedStep > maxStep / 4) {
const isAttemptingCanDismiss = isAttemptingDismissWithCanDismiss && clampedStep > maxStep / 4;

if (isAttemptingCanDismiss) {
handleCanDismiss(el, animation);
} else if (shouldComplete) {
onDismiss();
}

/**
* `shouldComplete` is always `false` when canDismiss blocks the
* gesture, since it only describes a dismiss that happens right
* away. In that case the modal dismisses through `handleCanDismiss`
* instead, which only dismisses when canDismiss is a function.
* A canDismiss function can still cancel the dismiss after this
* event is emitted.
*/
const isDismissing = isAttemptingDismissWithCanDismiss
? isAttemptingCanDismiss && typeof el.canDismiss === 'function'
: shouldComplete;

const eventDetail: ModalDragEventDetail = {
currentY: detail.currentY,
deltaY: detail.deltaY,
velocityY: detail.velocityY,
progress: calculateProgress(el, detail.deltaY),
isDismissing,
};

onDragEnd(eventDetail);
Expand Down
9 changes: 9 additions & 0 deletions core/src/components/modal/modal-interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,4 +72,13 @@ export interface ModalDragEventDetail {
* the gesture.
*/
snapBreakpoint?: number;

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 ModalDragEventDetail block in ionic-docs at docs/api/modal.md also needs to be updated, nothing over there picks this up automatically, so as it stands this ships undocumented.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/**
* Whether the modal is attempting to dismiss when the drag gesture
* ends. A `canDismiss` callback can still cancel the dismiss after
* this event is emitted, so use `ionModalDidDismiss` to confirm that
* the modal actually closed.
*
* This is only set on `ionDragEnd`.
*/
isDismissing?: boolean;

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.

Sheet modals never set this, so a consumer writing the obvious if (ev.detail.isDismissing) gets undefined on a sheet that is dismissing. I dragged one down to breakpoint 0 locally and got {"deltaY":180,"velocityY":10.76,"progress":0,"snapBreakpoint":0} with typeof isDismissing === 'undefined'. That's falsy, so the check quietly does nothing.

Both gestures publish through the same ionDragEnd, and sheet.ts already has what it needs in scope where it builds the detail:

isDismissing: snapBreakpoint === 0 && !canDismissBlocksGesture,

Same two values moveSheetToBreakpoint derives dismissal from. The early-return onDragEnd a bit further down would need an explicit false, since it doesn't move the sheet at all.

Card ionDragMove is the other half of this, it gets nothing while sheet emits snapBreakpoint on both move and end, where the move-time meaning is exactly "what happens if you let go now". I think both are worth deciding now rather than after release, because the asymmetry is permanent once it ships. Up to you though, the issue only asked about card modals so I won't block on it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

}
52 changes: 52 additions & 0 deletions core/src/components/modal/test/can-dismiss/modal-card.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,58 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>

await ionModalDidDismiss.next();
});
test('should report isDismissing as true when canDismiss is Promise<true>', async ({ page }) => {
const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent');
const ionModalDidDismiss = await page.spyOnEvent('ionModalDidDismiss');

await page.click('#card-can-dismiss-promise-true');

await ionModalDidPresent.next();

const ionDragEnd = await page.spyOnEvent('ionDragEnd');

const modalHeader = page.locator('#modal-header');
await dragElementBy(modalHeader, page, 0, 500);

const dragEndEvent = await ionDragEnd.next();

/**
* canDismiss forces the swipe animation to stop before it completes,
* so the event has to report the dismiss that canDismiss goes on to
* perform. Waiting for `ionModalDidDismiss` proves the modal really
* did close.
*/
expect(dragEndEvent.detail.isDismissing).toBe(true);

await ionModalDidDismiss.next();
});
test('should report isDismissing as true when canDismiss cancels the dismiss', async ({ page }) => {
const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent');
const ionHandlerDone = await page.spyOnEvent('ionHandlerDone');

await page.click('#card-can-dismiss-promise-false');

await ionModalDidPresent.next();

const ionDragEnd = await page.spyOnEvent('ionDragEnd');

const modalHeader = page.locator('#modal-header');
await dragElementBy(modalHeader, page, 0, 500);

const dragEndEvent = await ionDragEnd.next();

/**
* `ionDragEnd` fires before canDismiss resolves, so it reports the
* dismiss the gesture is attempting rather than the outcome. This is
* the one case where the two differ: canDismiss goes on to cancel the
* dismiss and the modal stays open.
*/
expect(dragEndEvent.detail.isDismissing).toBe(true);

await ionHandlerDone.next();

await expect(page.locator('ion-modal')).toBeVisible();
});
test('should not dismiss on swipe when canDismiss is Promise<false>', async ({ page }) => {
const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent');
const ionHandlerDone = await page.spyOnEvent('ionHandlerDone');
Expand Down
50 changes: 50 additions & 0 deletions core/src/components/modal/test/can-dismiss/modal-sheet.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,56 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>

await ionModalDidDismiss.next();
});
test('should report isDismissing as true when canDismiss is Promise<true>', async ({ page }) => {
const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent');
const ionModalDidDismiss = await page.spyOnEvent('ionModalDidDismiss');

await page.click('#sheet-can-dismiss-promise-true');

await ionModalDidPresent.next();

const ionDragEnd = await page.spyOnEvent('ionDragEnd');

const modalHeader = page.locator('#modal-header');
await dragElementBy(modalHeader, page, 0, 500);

const dragEndEvent = await ionDragEnd.next();

/**
* canDismiss keeps the sheet from snapping to breakpoint 0, so the
* event has to report the dismiss that canDismiss goes on to perform.
*/
expect(dragEndEvent.detail.isDismissing).toBe(true);

await ionModalDidDismiss.next();
});
test('should report isDismissing as true when canDismiss cancels the dismiss', async ({ page }) => {
const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent');
const ionHandlerDone = await page.spyOnEvent('ionHandlerDone');

await page.click('#sheet-can-dismiss-promise-false');

await ionModalDidPresent.next();

const ionDragEnd = await page.spyOnEvent('ionDragEnd');

const modalHeader = page.locator('#modal-header');
await dragElementBy(modalHeader, page, 0, 500);

const dragEndEvent = await ionDragEnd.next();

/**
* `ionDragEnd` fires before canDismiss resolves, so it reports the
* dismiss the gesture is attempting rather than the outcome. This is
* the one case where the two differ: canDismiss goes on to cancel the
* dismiss and the sheet stays open.
*/
expect(dragEndEvent.detail.isDismissing).toBe(true);

await ionHandlerDone.next();

await expect(page.locator('ion-modal')).toBeVisible();
});
test('should not dismiss on swipe when canDismiss is Promise<false>', async ({ page }) => {
const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent');
const ionHandlerDone = await page.spyOnEvent('ionHandlerDone');
Expand Down
53 changes: 46 additions & 7 deletions core/src/components/modal/test/card/modal-card.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,15 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, screenshot, c
});

test.describe(title('card modal: drag events'), () => {
test('should emit ionDragStart, ionDragMove, and ionDragEnd events', async ({ page }) => {
await page.goto('/src/components/modal/test/card', config);
let cardModalPage: CardModalPage;

const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent');
test.beforeEach(async ({ page }) => {
cardModalPage = new CardModalPage(page);
await cardModalPage.navigate('/src/components/modal/test/card', config);
});

await page.click('#drag-events');
await ionModalDidPresent.next();
test('should emit ionDragStart, ionDragMove, and ionDragEnd events', async ({ page }) => {
await cardModalPage.openModalByTrigger('#drag-events');

const ionDragStart = await page.spyOnEvent('ionDragStart');
const ionDragMove = await page.spyOnEvent('ionDragMove');
Expand All @@ -130,7 +132,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, screenshot, c
expect(ionDragStart.length).toBe(1);

expect(ionDragMove.length).toBeGreaterThan(0);
expect(Object.keys(dragMoveEvent.detail).length).toBe(4);
expect(Object.keys(dragMoveEvent.detail).sort()).toEqual(['currentY', 'deltaY', 'progress', 'velocityY']);

expect(ionDragEnd.length).toBe(0);

Expand All @@ -148,7 +150,44 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, screenshot, c
expect(ionDragMove.length).toBeGreaterThan(0);

expect(ionDragEnd.length).toBe(1);
expect(Object.keys(dragEndEvent.detail).length).toBe(4);
expect(Object.keys(dragEndEvent.detail).sort()).toEqual([

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 only checks the key set, so it passes whether the implementation emits true, false, null, or a hardcoded constant. The value is the entire feature, and the canDismiss problem I flagged over in swipe-to-close.ts passes like this.

The fixture already handles both outcomes, and the suite already proves which distances land either side of the threshold: swipeToCloseModal(selector) defaults to 300px and asserts dismissal, and the test further up drags 20px with the modal staying open. So something like:

test('should report isDismissing true when the gesture dismisses the modal', async ({ page }) => {
  await dragElementBy(page.locator('.modal-card ion-header'), page, 0, 300);
  const ev = await ionDragEnd.next();
  expect(ev.detail.isDismissing).toBe(true);
});

test('should report isDismissing false when the modal settles back open', async ({ page }) => {
  await dragElementBy(page.locator('.modal-card ion-header'), page, 0, 20);
  const ev = await ionDragEnd.next();
  expect(ev.detail.isDismissing).toBe(false);
  await expect(page.locator('ion-modal')).toBeVisible();
});

I ran both against a local build with this patch applied and they pass. Worth pairing each with the visibility check so the test proves the field matches reality instead of matching itself. A third one for the canDismiss path would be good too once that's settled, #card-can-dismiss-promise-true in test/can-dismiss/index.html is already there for it.

Swapping .length).toBe(4) for Object.keys(...).sort() is a nice improvement by the way, a count wouldn't have caught a rename.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

'currentY',
'deltaY',
'isDismissing',
'progress',
'velocityY',
]);
});

test('should report isDismissing as true when the gesture dismisses the modal', async ({ page }) => {
await cardModalPage.openModalByTrigger('#card');

const ionDragEnd = await page.spyOnEvent('ionDragEnd');

/**
* `swipeToCloseModal` drags 300px and waits for `ionModalDidDismiss`,
* so the modal is known to have dismissed by the time the event
* detail is checked.
*/
await cardModalPage.swipeToCloseModal('ion-modal ion-header');

const dragEndEvent = await ionDragEnd.next();

expect(dragEndEvent.detail.isDismissing).toBe(true);
});

test('should report isDismissing as false when the modal settles back open', async ({ page }) => {
const modal = await cardModalPage.openModalByTrigger('#card');

const ionDragEnd = await page.spyOnEvent('ionDragEnd');

// 20px is short enough that the gesture never passes the dismiss threshold
await cardModalPage.swipeToCloseModal('ion-modal ion-header', false, 20);

const dragEndEvent = await ionDragEnd.next();

expect(dragEndEvent.detail.isDismissing).toBe(false);
await expect(modal).toBeVisible();
});
});
});
59 changes: 55 additions & 4 deletions core/src/components/modal/test/sheet/modal.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -355,14 +355,15 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
});

test.describe(title('sheet modal: drag events'), () => {
test('should emit ionDragStart, ionDragMove, and ionDragEnd events', async ({ page }) => {
test.beforeEach(async ({ page }) => {
await page.goto('/src/components/modal/test/sheet', config);

const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent');

await page.click('#drag-events');
await ionModalDidPresent.next();

});
test('should emit ionDragStart, ionDragMove, and ionDragEnd events', async ({ page }) => {
const ionDragStart = await page.spyOnEvent('ionDragStart');
const ionDragMove = await page.spyOnEvent('ionDragMove');
const ionDragEnd = await page.spyOnEvent('ionDragEnd');
Expand All @@ -378,7 +379,13 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
expect(ionDragStart.length).toBe(1);

expect(ionDragMove.length).toBeGreaterThan(0);
expect(Object.keys(dragMoveEvent.detail).length).toBe(5);
expect(Object.keys(dragMoveEvent.detail).sort()).toEqual([
'currentY',
'deltaY',
'progress',
'snapBreakpoint',
'velocityY',
]);

expect(ionDragEnd.length).toBe(0);

Expand All @@ -396,7 +403,51 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
expect(ionDragMove.length).toBeGreaterThan(0);

expect(ionDragEnd.length).toBe(1);
expect(Object.keys(dragEndEvent.detail).length).toBe(5);
expect(Object.keys(dragEndEvent.detail).sort()).toEqual([
'currentY',
'deltaY',
'isDismissing',
'progress',
'snapBreakpoint',
'velocityY',
]);
});
test('should report isDismissing as true when the gesture dismisses the modal', async ({ page }) => {
const ionDragEnd = await page.spyOnEvent('ionDragEnd');
const ionModalDidDismiss = await page.spyOnEvent('ionModalDidDismiss');

const header = page.locator('.modal-sheet ion-header');
const headerBox = (await header.boundingBox())!;
const viewport = page.viewportSize()!;

/**
* The sheet starts at breakpoint 0.5, so its header sits near the middle
* of the viewport and the drag has to stay inside the bottom half.
* Dragging as far as the viewport allows moves the sheet well past the
* midpoint between breakpoints 0 and 0.25, so it snaps to 0 and dismisses.
*/
const dragByY = viewport.height - (headerBox.y + headerBox.height / 2) - 10;

await dragElementBy(header, page, 0, dragByY);

const dragEndEvent = await ionDragEnd.next();

expect(dragEndEvent.detail.isDismissing).toBe(true);

await ionModalDidDismiss.next();
});
test('should report isDismissing as false when the sheet snaps to another breakpoint', async ({ page }) => {
const ionDragEnd = await page.spyOnEvent('ionDragEnd');

const header = page.locator('.modal-sheet ion-header');

// Dragging this little keeps the sheet above breakpoint 0
await dragElementBy(header, page, 0, 50);

const dragEndEvent = await ionDragEnd.next();

expect(dragEndEvent.detail.isDismissing).toBe(false);
await expect(page.locator('ion-modal')).toBeVisible();
});
});

Expand Down