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.

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

Code review for two-up, pinch-zoom, trackDragging #11

Closed
wants to merge 31 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
c37a4c2
Very basic mouse move
jakearchibald Mar 9, 2018
99cf427
Pinch & pan
jakearchibald Mar 12, 2018
860f793
Supporting pointer events
jakearchibald Mar 12, 2018
dde466b
Avoiding 'as'
jakearchibald Mar 12, 2018
464a414
Adding change event & method to set position. Including side-by-side …
jakearchibald Mar 12, 2018
7c6b668
Updating demo
jakearchibald Mar 14, 2018
d463649
Splitting pointer handling into library
jakearchibald Mar 14, 2018
5600e4c
Fixing Safari & Edge event issues
jakearchibald Mar 14, 2018
05fc027
Switching pointer tracking to a class-based API
jakearchibald Mar 15, 2018
4a324ad
Minor docs improvement
jakearchibald Mar 15, 2018
a6fb4fe
Complying with semistandard
jakearchibald Mar 26, 2018
a0fb0ab
More semi-standard
jakearchibald Mar 27, 2018
abfeca6
Two-up MVP
jakearchibald Mar 28, 2018
7d483c6
Tslint complains about this, but it's required else it's tree-shaken.
jakearchibald Mar 28, 2018
e17c844
Performance tweak
jakearchibald Mar 28, 2018
4702b6c
Restoring event retargeting
jakearchibald Mar 28, 2018
d137136
Stub for multi-panel
jakearchibald Mar 28, 2018
c62d36a
Ooops
jakearchibald Mar 29, 2018
bb17890
Using custom properties
jakearchibald Mar 29, 2018
fe69cc6
Adding wheel support to pinch-zoom
jakearchibald Mar 29, 2018
839d9a2
Pinch-zoom: locking to bounds, the wrong way.
jakearchibald Mar 29, 2018
530a58e
Pinch-zoom: fixing scroll-to-zoom for Firefox 'lines' delta mode.
jakearchibald Mar 29, 2018
a2710e8
Pinch-zoom: Changing bounds so it can't go further outside the bounds
jakearchibald Mar 29, 2018
c1d72ff
Pinch-zoom: Make clamping work if inner element is a different size t…
jakearchibald Mar 29, 2018
9a86f96
Rename pointer tracker file
jakearchibald Mar 29, 2018
5afeb51
Edge fallback for two-up
jakearchibald Mar 29, 2018
644340f
Safari performance bottleneck fix.
jakearchibald Mar 29, 2018
6292fa0
Changes following review
jakearchibald Mar 29, 2018
c4b3d4c
Moving bounds checking into setTransform, and making this the main po…
jakearchibald Apr 3, 2018
4d796bd
Minor changes from feedback
jakearchibald Apr 3, 2018
7ddd714
Using dict for all setTransform params to improve readability
jakearchibald Apr 3, 2018
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
2 changes: 2 additions & 0 deletions components/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
node_modules
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only for the demo stuff. This will be removed in the final merge.

build
12 changes: 12 additions & 0 deletions components/MultiPanel/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import './styles.css';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a stub I created for @kosamari. Feel free to ignore.


export default class MultiPanel extends HTMLElement {
constructor () {
super();
}

connectedCallback () {
}
}

customElements.define('multi-panel', MultiPanel);
3 changes: 3 additions & 0 deletions components/MultiPanel/styles.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
multi-panel {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a stub I created for @kosamari. Feel free to ignore.


}
309 changes: 309 additions & 0 deletions components/PinchZoom/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,309 @@
import './styles.css';
import { PointerTracker, Pointer } from '../../utils/PointerTracker';

interface Point {
clientX: number;
clientY: number;
}

interface ApplyChangeOpts {
panX?: number;
panY?: number;
scaleDiff?: number;
originX?: number;
originY?: number;
}

interface SetTransformOpts {
scale?: number;
x?: number;
y?: number;
/**
* Fire a 'change' event if values are different to current values
*/
allowChangeEvent?: boolean;
}

function getDistance (a: Point, b?: Point): number {
if (!b) return 0;
return Math.sqrt((b.clientX - a.clientX) ** 2 + (b.clientY - a.clientY) ** 2);
}

function getMidpoint (a: Point, b?: Point): Point {
if (!b) return a;

return {
clientX: (a.clientX + b.clientX) / 2,
clientY: (a.clientY + b.clientY) / 2
};
}

// I'd rather use DOMMatrix/DOMPoint here, but the browser support isn't good enough.
// Given that, better to use something everything supports.
let cachedSvg: SVGSVGElement;

function getSVG (): SVGSVGElement {
return cachedSvg || (cachedSvg = document.createElementNS('http://www.w3.org/2000/svg', 'svg'));
}

function createMatrix (): SVGMatrix {
return getSVG().createSVGMatrix();
}

function createPoint (): SVGPoint {
return getSVG().createSVGPoint();
}

export default class PinchZoom extends HTMLElement {
// The element that we'll transform.
// Ideally this would be shadow DOM, but we don't have the browser
// support yet.
private _positioningEl?: Element;
// Current transform.
private _transform: SVGMatrix = createMatrix();

constructor () {
super();

// Watch for children changes.
// Note this won't fire for initial contents,
// so _stageElChange is also called in connectedCallback.
new MutationObserver(() => this._stageElChange())
.observe(this, { childList: true });

// Watch for pointers
const pointerTracker: PointerTracker = new PointerTracker(this, {
start: (pointer, event) => {
// We only want to track 2 pointers at most
if (pointerTracker.currentPointers.length === 2 || !this._positioningEl) return false;
event.preventDefault();

// Record current state
pointerTracker.resetStartPointers();
return true;
},
move: previousPointers => {
this._onPointerMove(previousPointers, pointerTracker.currentPointers);
}
});

this.addEventListener('wheel', event => this._onWheel(event));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Willing to use @bind instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely

}

connectedCallback () {
this._stageElChange();
}

get x () {
return this._transform.e;
}

get y () {
return this._transform.f;
}

get scale () {
return this._transform.a;
}

/**
* Update the stage with a given scale/x/y.
*/
setTransform (opts: SetTransformOpts = {}) {
const {
scale = this.scale,
allowChangeEvent = false
} = opts;

let {
x = this.x,
y = this.y
} = opts;

// If we don't have an element to position, just set the value as given.
// We'll check bounds later.
if (!this._positioningEl) {
this._updateTransform(scale, x, y, allowChangeEvent);
return;
}

// Get current layout
const thisBounds = this.getBoundingClientRect();
const positioningElBounds = this._positioningEl.getBoundingClientRect();

// Not displayed. May be disconnected or display:none.
// Just take the values, and we'll check bounds later.
if (!thisBounds.width || !thisBounds.height) {
this._updateTransform(scale, x, y, allowChangeEvent);
return;
}

// Create points for _positioningEl.
let topLeft = createPoint();
topLeft.x = positioningElBounds.left - thisBounds.left;
topLeft.y = positioningElBounds.top - thisBounds.top;
let bottomRight = createPoint();
bottomRight.x = positioningElBounds.width + topLeft.x;
bottomRight.y = positioningElBounds.height + topLeft.y;

// Calculate the intended position of _positioningEl.
let matrix = createMatrix()
.translate(x, y)
.scale(scale)
// Undo current transform
.multiply(this._transform.inverse());

topLeft = topLeft.matrixTransform(matrix);
bottomRight = bottomRight.matrixTransform(matrix);

// Ensure _positioningEl can't move beyond out-of-bounds.
// Correct for x
if (topLeft.x > thisBounds.width) {
x += thisBounds.width - topLeft.x;
} else if (bottomRight.x < 0) {
x += -bottomRight.x;
}

// Correct for y
if (topLeft.y > thisBounds.height) {
y += thisBounds.height - topLeft.y;
} else if (bottomRight.y < 0) {
y += -bottomRight.y;
}

this._updateTransform(scale, x, y, allowChangeEvent);
}

/**
* Update transform values without checking bounds. This is only called in setTransform.
*/
_updateTransform (scale: number, x: number, y: number, allowChangeEvent: boolean) {
// Return if there's no change
if (
scale === this.scale &&
x === this.x &&
y === this.y
) return;

this._transform.e = x;
this._transform.f = y;
this._transform.d = this._transform.a = scale;

this.style.setProperty('--x', this.x + 'px');
this.style.setProperty('--y', this.y + 'px');
this.style.setProperty('--scale', this.scale + '');

if (allowChangeEvent) {
const event = new Event('change', { bubbles: true });
this.dispatchEvent(event);
}
}

/**
* Called when the direct children of this element change.
* Until we have have shadow dom support across the board, we
* require a single element to be the child of <pinch-zoom>, and
* that's the element we pan/scale.
*/
private _stageElChange () {
this._positioningEl = undefined;

if (this.children.length === 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, crazy idea: this.children.length === 1 && this.children[0] instanceof HTMLElement

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know why _positioningEl needs to be an HTMLElement. I complained at one point, but I've refactored since, so it shouldn't be an issue.

console.warn('There should be at least one child in <pinch-zoom>.');
return;
}

this._positioningEl = this.children[0];

if (this.children.length > 1) {
console.warn('<pinch-zoom> must not have more than one child.');
}

// Do a bounds check
this.setTransform();
}

private _onWheel (event: WheelEvent) {
event.preventDefault();

const thisRect = this.getBoundingClientRect();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reasonable way we can cache this? Listen to document.onresize?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From in-person chat: There's no way to know bounds haven't changed after caching.

This is a common problem though, and we should have a think about what we need on the platform to make this easier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A primitive for that would be amazing...

Copy link
Collaborator

Choose a reason for hiding this comment

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

@todo maybe try ResizeObserver here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Resize observer is useful in cases where we need to immediately react to a change in layout size. Whereas in this case, we only need it on-demand, and we need the position relative to the viewport which resize observer doesn't give us.

Besides, resize observer is Chrome-only, so it doesn't make sense with our target browser support.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Just for completeness sake: If we had broad RO support, you could use ROs to cache the most recent layout values and not have to worry about forcing sync layout.)

let { deltaY } = event;
const { ctrlKey, deltaMode } = event;

if (deltaMode === 1) { // 1 is "lines", 0 is "pixels"
// Firefox uses "lines" for some types of mouse
deltaY *= 15;
}

// ctrlKey is true when pinch-zooming on a trackpad.
const divisor = ctrlKey ? 100 : 300;
const scaleDiff = 1 - deltaY / divisor;

this._applyChange({
scaleDiff,
originX: event.clientX - thisRect.left,
originY: event.clientY - thisRect.top
});
}

private _onPointerMove (previousPointers: Pointer[], currentPointers: Pointer[]) {
// Combine next points with previous points
const thisRect = this.getBoundingClientRect();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t think we can afford to call gBCR on a move event. We really should be looking into caching these.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same caching issue as above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

random thought: What about deferring it until the next render, then reacting only if it changed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A frame delay on something that should stick to the user's finger feels like a lot.

Also, it means Edge/Safari's buggy raf implementation might make it two frames.


// For calculating panning movement
const prevMidpoint = getMidpoint(previousPointers[0], previousPointers[1]);
const newMidpoint = getMidpoint(currentPointers[0], currentPointers[1]);

// Midpoint within the element
const originX = prevMidpoint.clientX - thisRect.left;
const originY = prevMidpoint.clientY - thisRect.top;

// Calculate the desired change in scale
const prevDistance = getDistance(previousPointers[0], previousPointers[1]);
const newDistance = getDistance(currentPointers[0], currentPointers[1]);
const scaleDiff = prevDistance ? newDistance / prevDistance : 1;

this._applyChange({
originX, originY, scaleDiff,
panX: newMidpoint.clientX - prevMidpoint.clientX,
panY: newMidpoint.clientY - prevMidpoint.clientY
});
}

/** Transform the view & fire a change event */
private _applyChange (opts: ApplyChangeOpts = {}) {
const {
panX = 0, panY = 0,
originX = 0, originY = 0,
scaleDiff = 1
} = opts;

const matrix = createMatrix()
// Translate according to panning.
.translate(panX, panY)
// Scale about the origin.
.translate(originX, originY)
.scale(scaleDiff)
.translate(-originX, -originY)
// Apply current transform.
.multiply(this._transform);

// Convert the transform into basic translate & scale.
this.setTransform({
scale: matrix.a,
x: matrix.e,
y: matrix.f,
allowChangeEvent: true
});
}
}

customElements.define('pinch-zoom', PinchZoom);

// TODO:
// scale by method, which takes a scaleDiff and a center
// Go to new scale pos, animate to new scale pos
// On change event
// scale & x & y props
// Exclude selector
// Anchor point for resize
10 changes: 10 additions & 0 deletions components/PinchZoom/missing-types.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
declare interface CSSStyleDeclaration {
willChange: string | null;
}

// TypeScript, you make me sad.
// https://github.com/Microsoft/TypeScript/issues/18756
interface Window {
PointerEvent: typeof PointerEvent;
Touch: typeof Touch;
}
14 changes: 14 additions & 0 deletions components/PinchZoom/styles.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
pinch-zoom {
display: block;
overflow: hidden;
touch-action: none;
--scale: 1;
--x: 0;
--y: 0;
}

pinch-zoom > * {
transform: translate(var(--x), var(--y)) scale(var(--scale));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really like this approach of going via custom props!

transform-origin: 0 0;
will-change: transform;
}
Loading