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

Batch render DOM elements to avoid reflow #10530

Merged
merged 3 commits into from
Apr 7, 2021
Merged

Batch render DOM elements to avoid reflow #10530

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
846520d
Batch render DOM elements to avoid reflow
zarov Apr 1, 2021
bbeb2f1
Fixed flow and unit testing for DOM batch rendering
zarov Apr 1, 2021
27ab023
Removed aliases after review
zarov Apr 2, 2021
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
5 changes: 4 additions & 1 deletion src/ui/control/navigation_control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,10 @@ class NavigationControl {
`scale(${1 / Math.pow(Math.cos(this._map.transform.pitch * (Math.PI / 180)), 0.5)}) rotateX(${this._map.transform.pitch}deg) rotateZ(${this._map.transform.angle * (180 / Math.PI)}deg)` :
`rotate(${this._map.transform.angle * (180 / Math.PI)}deg)`;

this._compassIcon.style.transform = rotate;
const icon = this._compassIcon;
this._map._domRenderTaskQueue.add(() => {
icon.style.transform = rotate;
});
}

onAdd(map: Map) {
Expand Down
18 changes: 10 additions & 8 deletions src/ui/control/scale_control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,25 +103,27 @@ function updateScale(map, container, options) {
const maxFeet = 3.2808 * maxMeters;
if (maxFeet > 5280) {
const maxMiles = maxFeet / 5280;
setScale(container, maxWidth, maxMiles, map._getUIString('ScaleControl.Miles'));
setScale(map, container, maxWidth, maxMiles, map._getUIString('ScaleControl.Miles'));
} else {
setScale(container, maxWidth, maxFeet, map._getUIString('ScaleControl.Feet'));
setScale(map, container, maxWidth, maxFeet, map._getUIString('ScaleControl.Feet'));
}
} else if (options && options.unit === 'nautical') {
const maxNauticals = maxMeters / 1852;
setScale(container, maxWidth, maxNauticals, map._getUIString('ScaleControl.NauticalMiles'));
setScale(map, container, maxWidth, maxNauticals, map._getUIString('ScaleControl.NauticalMiles'));
} else if (maxMeters >= 1000) {
setScale(container, maxWidth, maxMeters / 1000, map._getUIString('ScaleControl.Kilometers'));
setScale(map, container, maxWidth, maxMeters / 1000, map._getUIString('ScaleControl.Kilometers'));
} else {
setScale(container, maxWidth, maxMeters, map._getUIString('ScaleControl.Meters'));
setScale(map, container, maxWidth, maxMeters, map._getUIString('ScaleControl.Meters'));
}
}

function setScale(container, maxWidth, maxDistance, unit) {
function setScale(map, container, maxWidth, maxDistance, unit) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: it's better to pass the smallest scope necessary for the function — in this case, queue instead of the whole map object. Makes refactoring easier later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I totally missed this!

const distance = getRoundNum(maxDistance);
const ratio = distance / maxDistance;
container.style.width = `${maxWidth * ratio}px`;
container.innerHTML = `${distance} ${unit}`;
map._domRenderTaskQueue.add(() => {
container.style.width = `${maxWidth * ratio}px`;
container.innerHTML = `${distance} ${unit}`;
});
}

function getDecimalRoundNum(d) {
Expand Down
9 changes: 6 additions & 3 deletions src/ui/handler/box_zoom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,13 @@ class BoxZoomHandler {
minY = Math.min(p0.y, pos.y),
maxY = Math.max(p0.y, pos.y);

DOM.setTransform(this._box, `translate(${minX}px,${minY}px)`);
const box = this._box;
this._map._domRenderTaskQueue.add(() => {
DOM.setTransform(box, `translate(${minX}px,${minY}px)`);

this._box.style.width = `${maxX - minX}px`;
this._box.style.height = `${maxY - minY}px`;
box.style.width = `${maxX - minX}px`;
box.style.height = `${maxY - minY}px`;
});
}

mouseupWindow(e: MouseEvent, point: Point) {
Expand Down
4 changes: 4 additions & 0 deletions src/ui/map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@ class Map extends Camera {
_collectResourceTiming: boolean;
_optimizeForTerrain: boolean;
_renderTaskQueue: TaskQueue;
_domRenderTaskQueue: TaskQueue;
_controls: Array<IControl>;
_logoControl: IControl;
_mapId: number;
Expand Down Expand Up @@ -420,6 +421,7 @@ class Map extends Camera {
this._collectResourceTiming = options.collectResourceTiming;
this._optimizeForTerrain = options.optimizeForTerrain;
this._renderTaskQueue = new TaskQueue();
this._domRenderTaskQueue = new TaskQueue();
this._controls = [];
this._mapId = uniqueId();
this._locale = extend({}, defaultLocale, options.locale);
Expand Down Expand Up @@ -2526,6 +2528,7 @@ class Map extends Camera {
this.painter.setBaseState();

this._renderTaskQueue.run(paintStartTimeStamp);
this._domRenderTaskQueue.run(paintStartTimeStamp);
// A task queue callback may have fired a user event which may have removed the map
if (this._removed) return;

Expand Down Expand Up @@ -2770,6 +2773,7 @@ class Map extends Camera {
this._frame = null;
}
this._renderTaskQueue.clear();
this._domRenderTaskQueue.clear();
this.painter.destroy();
this.handlers.destroy();
delete this.handlers;
Expand Down
7 changes: 6 additions & 1 deletion src/ui/marker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,12 @@ export default class Marker extends Evented {
this._pos = this._pos.round();
}

DOM.setTransform(this._element, `${anchorTranslate[this._anchor]} translate(${this._pos.x}px, ${this._pos.y}px) ${pitch} ${rotation}`);
const el = this._element;
const pos = this._pos;
const anchor = this._anchor;
Copy link
Member

Choose a reason for hiding this comment

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

You can get rid of aliases here too (and check for just this._element).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is embarrassing, I really thought I removed it!

this._map._domRenderTaskQueue.add(() => {
DOM.setTransform(el, `${anchorTranslate[anchor]} translate(${pos.x}px, ${pos.y}px) ${pitch} ${rotation}`);
});
}

/**
Expand Down
8 changes: 6 additions & 2 deletions src/ui/popup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,8 +572,12 @@ export default class Popup extends Evented {
}

const offsetedPos = pos.add(offset[anchor]).round();
DOM.setTransform(this._container, `${anchorTranslate[anchor]} translate(${offsetedPos.x}px,${offsetedPos.y}px)`);
applyAnchorClass(this._container, anchor, 'popup');
const container = this._container;
const _anchor = anchor;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: given that the closure captures the anchor variable and it's not modified later in the method, and that the arrow function retains the context of this, why do we need to define aliases here?

Copy link
Member

Choose a reason for hiding this comment

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

Oops, I edited the comment here but the old version got posted somehow — so, now I see that the DOM elements are aliased so that it doesn't break in case a task is run after the object is removed from the map, but it's not obvious, so might need additional comments to avoid maintainers tripping up on this. Alternatively, we could look into whether this can be refactored to avoid the issue differently — e.g. perhaps marking a task as cancelled when a corresponding object is removed, or maybe adding some guards for the elements inside the closure.

Copy link
Contributor Author

@zarov zarov Apr 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

To be honest, I got an error with flow regarding anchor about it being null in the arrow function and I didn't understand how that can be possible, hence the alias.

It would be better however to see how it can be refactored. What about storing the task ID in the popup, as there should always be only one? Considering the total number of popup, it won't be a burden and then we could cancel the task in remove(). Otherwise another simple solution that seems to work is by simply adding if (this._container && anchor) in the arrow function.

this._map._domRenderTaskQueue.add(() => {
DOM.setTransform(container, `${anchorTranslate[_anchor]} translate(${offsetedPos.x}px,${offsetedPos.y}px)`);
applyAnchorClass(container, _anchor, 'popup');
});
}

_focusFirstElement() {
Expand Down
5 changes: 5 additions & 0 deletions test/unit/ui/control/scale.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import ScaleControl from '../../../../src/ui/control/scale_control.js';
test('ScaleControl appears in bottom-left by default', (t) => {
const map = createMap(t);
map.addControl(new ScaleControl());
map._domRenderTaskQueue.run();

t.equal(map.getContainer().querySelectorAll('.mapboxgl-ctrl-bottom-left .mapboxgl-ctrl-scale').length, 1);
t.end();
Expand All @@ -14,6 +15,7 @@ test('ScaleControl appears in bottom-left by default', (t) => {
test('ScaleControl appears in the position specified by the position option', (t) => {
const map = createMap(t);
map.addControl(new ScaleControl(), 'top-left');
map._domRenderTaskQueue.run();

t.equal(map.getContainer().querySelectorAll('.mapboxgl-ctrl-top-left .mapboxgl-ctrl-scale').length, 1);
t.end();
Expand All @@ -24,11 +26,13 @@ test('ScaleControl should change unit of distance after calling setUnit', (t) =>
const scale = new ScaleControl();
const selector = '.mapboxgl-ctrl-bottom-left .mapboxgl-ctrl-scale';
map.addControl(scale);
map._domRenderTaskQueue.run();

let contents = map.getContainer().querySelector(selector).innerHTML;
t.match(contents, /km/);

scale.setUnit('imperial');
map._domRenderTaskQueue.run();
contents = map.getContainer().querySelector(selector).innerHTML;
t.match(contents, /mi/);
t.end();
Expand All @@ -41,6 +45,7 @@ test('ScaleControl should respect the maxWidth regardless of the unit and actual
const selector = '.mapboxgl-ctrl-bottom-left .mapboxgl-ctrl-scale';
map.addControl(scale);
map.setZoom(12.5);
map._domRenderTaskQueue.run();

const el = map.getContainer().querySelector(selector);
t.ok(parseFloat(el.style.width, 10) <= maxWidth, 'ScaleControl respects maxWidth');
Expand Down
15 changes: 15 additions & 0 deletions test/unit/ui/marker.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ test('Marker anchor defaults to center', (t) => {
const marker = new Marker()
.setLngLat([0, 0])
.addTo(map);
map._domRenderTaskQueue.run();

t.ok(marker.getElement().classList.contains('mapboxgl-marker-anchor-center'));
t.match(marker.getElement().style.transform, /translate\(-50%,-50%\)/);
Expand All @@ -232,6 +233,7 @@ test('Marker anchors as specified by the anchor option', (t) => {
const marker = new Marker({anchor: 'top'})
.setLngLat([0, 0])
.addTo(map);
map._domRenderTaskQueue.run();

t.ok(marker.getElement().classList.contains('mapboxgl-marker-anchor-top'));
t.match(marker.getElement().style.transform, /translate\(-50%,0\)/);
Expand Down Expand Up @@ -259,6 +261,7 @@ test('Popup offsets around default Marker', (t) => {
.setLngLat([0, 0])
.setPopup(new Popup().setText('Test'))
.addTo(map);
map._domRenderTaskQueue.run();

t.ok(marker.getPopup().options.offset.bottom[1] < 0, 'popup is vertically offset somewhere above the tip');
t.ok(marker.getPopup().options.offset.top[1] === 0, 'popup is vertically offset at the tip');
Expand Down Expand Up @@ -296,34 +299,42 @@ test('Popup anchors around default Marker', (t) => {
Object.defineProperty(marker.getPopup()._container, 'offsetHeight', {value: 100});

// marker should default to above since it has enough space
map._domRenderTaskQueue.run();
t.ok(marker.getPopup()._container.classList.contains('mapboxgl-popup-anchor-bottom'), 'popup anchors above marker');

// move marker to the top forcing the popup to below
marker.setLngLat(map.unproject([mapHeight / 2, markerTop]));
map._domRenderTaskQueue.run();
t.ok(marker.getPopup()._container.classList.contains('mapboxgl-popup-anchor-top'), 'popup anchors below marker');

// move marker to the right forcing the popup to the left
marker.setLngLat(map.unproject([mapHeight - markerRight, mapHeight / 2]));
map._domRenderTaskQueue.run();
t.ok(marker.getPopup()._container.classList.contains('mapboxgl-popup-anchor-right'), 'popup anchors left of marker');

// move marker to the left forcing the popup to the right
marker.setLngLat(map.unproject([markerRight, mapHeight / 2]));
map._domRenderTaskQueue.run();
t.ok(marker.getPopup()._container.classList.contains('mapboxgl-popup-anchor-left'), 'popup anchors right of marker');

// move marker to the top left forcing the popup to the bottom right
marker.setLngLat(map.unproject([markerRight, markerTop]));
map._domRenderTaskQueue.run();
t.ok(marker.getPopup()._container.classList.contains('mapboxgl-popup-anchor-top-left'), 'popup anchors bottom right of marker');

// move marker to the top right forcing the popup to the bottom left
marker.setLngLat(map.unproject([mapHeight - markerRight, markerTop]));
map._domRenderTaskQueue.run();
t.ok(marker.getPopup()._container.classList.contains('mapboxgl-popup-anchor-top-right'), 'popup anchors bottom left of marker');

// move marker to the bottom left forcing the popup to the top right
marker.setLngLat(map.unproject([markerRight, mapHeight]));
map._domRenderTaskQueue.run();
t.ok(marker.getPopup()._container.classList.contains('mapboxgl-popup-anchor-bottom-left'), 'popup anchors top right of marker');

// move marker to the bottom right forcing the popup to the top left
marker.setLngLat(map.unproject([mapHeight - markerRight, mapHeight]));
map._domRenderTaskQueue.run();
t.ok(marker.getPopup()._container.classList.contains('mapboxgl-popup-anchor-bottom-right'), 'popup anchors top left of marker');

t.end();
Expand Down Expand Up @@ -725,11 +736,13 @@ test('Marker transforms rotation with the map', (t) => {
const marker = new Marker({rotationAlignment: 'map'})
.setLngLat([0, 0])
.addTo(map);
map._domRenderTaskQueue.run();

const rotationRegex = /rotateZ\(-?([0-9]+)deg\)/;
const initialRotation = marker.getElement().style.transform.match(rotationRegex)[1];

map.setBearing(map.getBearing() + 180);
map._domRenderTaskQueue.run();

const finalRotation = marker.getElement().style.transform.match(rotationRegex)[1];
t.notEqual(initialRotation, finalRotation);
Expand All @@ -745,11 +758,13 @@ test('Marker transforms pitch with the map', (t) => {
.addTo(map);

map.setPitch(0);
map._domRenderTaskQueue.run();

const rotationRegex = /rotateX\(-?([0-9]+)deg\)/;
const initialPitch = marker.getElement().style.transform.match(rotationRegex)[1];

map.setPitch(45);
map._domRenderTaskQueue.run();

const finalPitch = marker.getElement().style.transform.match(rotationRegex)[1];
t.notEqual(initialPitch, finalPitch);
Expand Down
9 changes: 9 additions & 0 deletions test/unit/ui/popup.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,7 @@ test('Popup anchors as specified by the anchor option', (t) => {
.setLngLat([0, 0])
.setText('Test')
.addTo(map);
map._domRenderTaskQueue.run();

t.ok(popup.getElement().classList.contains('mapboxgl-popup-anchor-top-left'));
t.end();
Expand All @@ -419,13 +420,15 @@ test('Popup anchors as specified by the anchor option', (t) => {
.setLngLat([0, 0])
.setText('Test')
.addTo(map);
map._domRenderTaskQueue.run();

Object.defineProperty(popup.getElement(), 'offsetWidth', {value: 100});
Object.defineProperty(popup.getElement(), 'offsetHeight', {value: 100});

t.stub(map, 'project').returns(point);
t.stub(map.transform, 'locationPoint3D').returns(point);
popup.setLngLat([0, 0]);
map._domRenderTaskQueue.run();

t.ok(popup.getElement().classList.contains(`mapboxgl-popup-anchor-${anchor}`));
t.end();
Expand All @@ -440,6 +443,7 @@ test('Popup anchors as specified by the anchor option', (t) => {
.setLngLat([0, 0])
.setText('Test')
.addTo(map);
map._domRenderTaskQueue.run();

t.equal(popup.getElement().style.transform, transform);
t.end();
Expand All @@ -457,12 +461,14 @@ test('Popup automatically anchors to top if its bottom offset would push it off-
.setLngLat([0, 0])
.setText('Test')
.addTo(map);
map._domRenderTaskQueue.run();

Object.defineProperty(popup.getElement(), 'offsetWidth', {value: containerWidth / 2});
Object.defineProperty(popup.getElement(), 'offsetHeight', {value: containerHeight / 2});

t.stub(map, 'project').returns(point);
popup.setLngLat([0, 0]);
map._domRenderTaskQueue.run();

t.ok(popup.getElement().classList.contains('mapboxgl-popup-anchor-top'));
t.end();
Expand All @@ -477,6 +483,7 @@ test('Popup is offset via a PointLike offset option', (t) => {
.setLngLat([0, 0])
.setText('Test')
.addTo(map);
map._domRenderTaskQueue.run();

t.equal(popup.getElement().style.transform, 'translate(0,0) translate(5px,10px)');
t.end();
Expand All @@ -491,6 +498,7 @@ test('Popup is offset via an object offset option', (t) => {
.setLngLat([0, 0])
.setText('Test')
.addTo(map);
map._domRenderTaskQueue.run();

t.equal(popup.getElement().style.transform, 'translate(0,0) translate(5px,10px)');
t.end();
Expand All @@ -505,6 +513,7 @@ test('Popup is offset via an incomplete object offset option', (t) => {
.setLngLat([0, 0])
.setText('Test')
.addTo(map);
map._domRenderTaskQueue.run();

t.equal(popup.getElement().style.transform, 'translate(-100%,0) translate(0px,0px)');
t.end();
Expand Down