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

AG-13363 Long-Tap iOS Safari #3442

Merged
merged 8 commits into from
Jan 31, 2025
Merged

AG-13363 Long-Tap iOS Safari #3442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7258614
AG-13363 Set -webkit-user-select: none in CSS
olegat Jan 30, 2025
c078ca2
AG-13363 Initial PoC longtap implementation for iOS Safari
olegat Jan 30, 2025
6a52dc2
AG-13363 Set clientXY of synthetic contextmenu event
olegat Jan 30, 2025
a02e98e
AG-13363 Do not interrupt longtaps for touchmoves < 10px
olegat Jan 30, 2025
1522f62
AG-13363 Enable iOS long-taps on legend items
olegat Jan 30, 2025
68ddfcc
AG-13363 Remove console.log calls
olegat Jan 30, 2025
70b4ba6
AG-13363 CR nit: add _MS suffix
olegat Jan 30, 2025
8ffca07
AG-13363 CR nit: add _PXPX suffix
olegat Jan 30, 2025
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
1 change: 1 addition & 0 deletions packages/ag-charts-community/src/chart/interaction/tooltipManager.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
.ag-charts-tooltip--no-interaction {
pointer-events: none;
user-select: none;
-webkit-user-select: none;
}

.ag-charts-tooltip--wrap-always {
Expand Down
8 changes: 8 additions & 0 deletions packages/ag-charts-community/src/chart/legend/legendDOMProxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,14 @@ export class LegendDOMProxy {
button.addListener('contextmenu', (ev) => itemListener.onContextClick(ev.sourceEvent, markerLabel));
button.addListener('blur', () => itemListener.onLeave());
button.addListener('focus', (ev) => itemListener.onHover(ev.sourceEvent, markerLabel));
// Enable touch long-tap context menus:
//
// We don't actually need to listen for drag events. However, on of the quirks of `Widget` is that it only
// adds a 'touchstart' listener if the widget has 'drag-*' listener(s), it's this 'touchstart' listener that
// handles both touch dragging and long-taps. Rather than adding 'touchstart' listeners to all HTMLElement
// (of which most don't even need one), we just add a dummy 'drag-start' to enable long-taps on legend
// buttons.
button.addListener('drag-start', () => {});
});
this.dirty = false;
}
Expand Down
3 changes: 3 additions & 0 deletions packages/ag-charts-community/src/dom/container.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
.ag-charts-wrapper {
position: relative;
user-select: none;
-webkit-user-select: none;
}

.ag-charts-tab-guard {
Expand All @@ -27,6 +28,7 @@
.ag-charts-canvas {
position: relative;
user-select: none;
-webkit-user-select: none;
}

.ag-charts-canvas-container > *,
Expand Down Expand Up @@ -61,6 +63,7 @@
pointer-events: none;
position: absolute;
user-select: none;
-webkit-user-select: none;
}

.ag-charts-canvas-overlay > * {
Expand Down
1 change: 1 addition & 0 deletions packages/ag-charts-community/src/dom/focusStyles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
display: block;
pointer-events: none;
user-select: none;
-webkit-user-select: none;
width: 100%;
height: 100%;
}
Expand Down
76 changes: 65 additions & 11 deletions packages/ag-charts-community/src/widget/widgetListenerInternal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,12 @@ function getTouchOffsets(current: Targetable, { pageX, pageY }: Touch): { offset
return { offsetX: pageX - x, offsetY: pageY - y };
}

function deltaClientSquared(a: Touch, b: Touch): number {
const dx = a.clientX - b.clientX;
const dy = a.clientY - b.clientY;
return dx * dx + dy * dy;
}

function makeTouchDrag<K extends DragEvents>(
current: Targetable,
type: K,
Expand All @@ -126,13 +132,56 @@ function makeTouchDrag<K extends DragEvents>(
};
}

function startTouchDrag(
const LONG_TAP_DURATION_MS = 500; /* milliseconds */
const LONG_TAP_INTERRUPT_MIN_TOUCHMOVE_PXPX = 100; /* px²*/

let gIsInLongTap = false;
function startOneFingerTouch(
dragTouchEnabled: boolean,
that: { globalTouchDragCallbacks?: TouchDragCallbacks },
myCallbacks: TouchDragCallbacks,
initialTouch: Touch,
target: HTMLElement
) {
if (that.globalTouchDragCallbacks != null) return;
if (that.globalTouchDragCallbacks != null || gIsInLongTap) return;

let longTapInterrupted = false;
setTimeout(() => {
if (!longTapInterrupted) {
// Cancel current 'drag-start':
target.dispatchEvent(new TouchEvent('touchcancel', { touches: [initialTouch], bubbles: true }));

// Block new 'drag-start' events:
gIsInLongTap = true;

// Block 'touchmove' page-scroll until the user lifts the finger:
const longTapMove = (e: Event) => {
e.preventDefault();
};
// Unblock new 'drag-start' events:
const longTapEnd = (e: Event) => {
gIsInLongTap = false;
e.preventDefault();
target.removeEventListener('touchmove', longTapMove);
target.removeEventListener('touchend', longTapEnd);
target.removeEventListener('touchcancel', longTapEnd);
};
target.addEventListener('touchmove', longTapMove, { passive: false });
target.addEventListener('touchend', longTapEnd, { passive: false });
target.addEventListener('touchcancel', longTapEnd, { passive: false });

// Fire context menu
const { clientX, clientY } = initialTouch;
const contextMenuEvent = new MouseEvent('contextmenu', {
bubbles: true,
cancelable: true,
view: window,
clientX,
clientY,
});
target.dispatchEvent(contextMenuEvent);
}
}, LONG_TAP_DURATION_MS);

const findInitialFinger = (...touchLists: TouchList[]): Touch | undefined => {
const touches: Touch[] = touchLists.map((touchList) => Array.from(touchList)).flat();
Expand All @@ -142,16 +191,21 @@ function startTouchDrag(
const touchmove = (moveEvent: TouchEvent) => {
const touch = findInitialFinger(moveEvent.targetTouches);
if (touch != null) {
that.globalTouchDragCallbacks?.touchmove(moveEvent, touch);
longTapInterrupted =
longTapInterrupted || deltaClientSquared(initialTouch, touch) < LONG_TAP_INTERRUPT_MIN_TOUCHMOVE_PXPX;
if (dragTouchEnabled && touch != null) {
that.globalTouchDragCallbacks?.touchmove(moveEvent, touch);
}
}
};
const touchend = (endEvent: TouchEvent) => {
longTapInterrupted = true;
Copy link
Member

@alantreadway alantreadway Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In the cases where longTapInterrupted is set to true, could you cancel the setTimeout() via clearTimeout() so that we don't have dangling JS variable contexts hanging around?

It would also be good if we could clear these up on chart.destroy() too, otherwise the callback could happen after chart destruction, with inevitable errors - also references in the callback context can prevent garbage collection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the cases where longTapInterrupted is set to true, could you cancel the setTimeout() via clearTimeout() so that we don't have dangling JS variable contexts hanging around?

Can you clarify how we would end up with a dangling reference? I don't mind using clearTimeout(), but I cannot see how that improves memory safety. Without a clearTimeout(), the setTimeout() will eventually call then remove the function, which would in turn make its variable context eligible for garbage collection.

It would also be good if we could clear these up on chart.destroy() too, otherwise the callback could happen after chart destruction, with inevitable errors

The callbacks will not cause errors because we always remove the listeners on chart.destroy() (see WidgetListenerInternal.destroy()). So these callbacks will, at worst, loop through an undefined or empty array of listeners.

Calling chart.destroy() whilst a finger is being dragged on that chart can indeed delay garbage collection until after the finger is lifted. Removing the listeners on chart.destroy() isn't necessary, but it would stop this delay. However, the challenge is that these are global callbacks shared amongst all chart instances, we must be cautious to ensure that destroying one chart doesn't interrupt another chart that's currently being dragged.

We could perhaps refactor the startMouseDrag and startOneFingerTouch to be temporary member objects of WidgetListenerInternal to manage the memory better. But the way I see it there's very little gain in doing that: there's only one mouse & touch device in the window and therefore there's only ever one drag motion in progress. So we can safely manage the memory regardless of whether or not the chart gets destroyed.

also references in the callback context can prevent garbage collection?

Are you referring to that.globalTouchDragCallbacks? If yes, that variable is used correctly AFAIK. It sets to undefined either in the touchend handler (called on 'touchstart', 'touchend' or 'touchcancel') or in the WidgetListenerInternal.destroy() method (called by chart.destroy()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed on slack: I've tested the spark-line use case and didn't notice anything alarming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See follow-up #3446 (currently untested)

target.removeEventListener('touchstart', touchend);
target.removeEventListener('touchmove', touchmove);
target.removeEventListener('touchend', touchend);
target.removeEventListener('touchcancel', touchend);
const touch = findInitialFinger(endEvent.changedTouches, endEvent.touches);
if (touch != null) {
target.removeEventListener('touchstart', touchend);
target.removeEventListener('touchmove', touchmove);
target.removeEventListener('touchend', touchend);
target.removeEventListener('touchcancel', touchend);
that.globalTouchDragCallbacks?.touchend(endEvent, touch);
that.globalTouchDragCallbacks = undefined;
}
Expand Down Expand Up @@ -297,12 +351,12 @@ export class WidgetListenerInternal {

private triggerTouchDrag<T extends Targetable>(current: T, startEvent: TouchEvent) {
const touch = startEvent.targetTouches.item(0);
if (this.dragTouchEnabled && startEvent.targetTouches.length === 1 && touch != null) {
this.startTouchDrag(current, startEvent, touch);
if (startEvent.targetTouches.length === 1 && touch != null) {
this.startOneFingerTouch(current, startEvent, touch);
}
}

private startTouchDrag<T extends Targetable>(current: T, initialEvent: TouchEvent, initialTouch: Touch) {
private startOneFingerTouch<T extends Targetable>(current: T, initialEvent: TouchEvent, initialTouch: Touch) {
const origin: DragOrigin = { pageX: NaN, pageY: NaN, ...getTouchOffsets(current, initialTouch) };
partialAssign(['pageX', 'pageY'], origin, initialTouch);

Expand All @@ -318,7 +372,7 @@ export class WidgetListenerInternal {
};
this.localTouchDragCallbacks = dragCallbacks;

startTouchDrag(GlobalCallbacks, dragCallbacks, initialTouch, current.getElement());
startOneFingerTouch(this.dragTouchEnabled, GlobalCallbacks, dragCallbacks, initialTouch, current.getElement());

const dragStartEvent = makeTouchDrag(current, 'drag-start', origin, initialEvent, initialTouch);
this.dispatch('drag-start', current, dragStartEvent);
Expand Down