From d65eb3d3f516c6d761b273b93323ddd9dc5150a2 Mon Sep 17 00:00:00 2001 From: Chris Lord Date: Tue, 17 Dec 2024 12:59:25 +0000 Subject: [PATCH 1/2] Synchronise comment blocks with document position Don't postpone and don't animate comment block position changes when scrolling or resizing the document. Signed-off-by: Chris Lord Change-Id: I95c1ae1eb732c6dc682018b85debb6133d85ce8c --- browser/src/canvas/sections/CommentListSection.ts | 13 ++++++++----- browser/src/canvas/sections/CommentSection.ts | 5 +++-- browser/src/dom/PosAnimation.js | 11 +++++++++++ 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/browser/src/canvas/sections/CommentListSection.ts b/browser/src/canvas/sections/CommentListSection.ts index 82b5181c0055..629c133fd08a 100644 --- a/browser/src/canvas/sections/CommentListSection.ts +++ b/browser/src/canvas/sections/CommentListSection.ts @@ -1250,7 +1250,10 @@ export class CommentSection extends app.definitions.canvasSectionObject { this.sectionProperties.selectedComment.hide(); } - this.update(); + var previousAnimationState = this.disableLayoutAnimation; + this.disableLayoutAnimation = true; + this.update(true); + this.disableLayoutAnimation = previousAnimationState; } private showHideComments (): void { @@ -2010,8 +2013,8 @@ export class CommentSection extends app.definitions.canvasSectionObject { this.disableLayoutAnimation = false; } - private layout (zoom: any = null): void { - if (zoom) + private layout (immediate: any = null): void { + if (immediate) this.doLayout(); else if (!this.sectionProperties.layoutTimer) { this.sectionProperties.layoutTimer = setTimeout(function() { @@ -2021,10 +2024,10 @@ export class CommentSection extends app.definitions.canvasSectionObject { } // else - avoid excessive re-layout } - private update (): void { + private update (immediate: boolean = false): void { if (this.sectionProperties.docLayer._docType === 'text') this.updateThreadInfoIndicator(); - this.layout(); + this.layout(immediate); } private updateThreadInfoIndicator(): void { diff --git a/browser/src/canvas/sections/CommentSection.ts b/browser/src/canvas/sections/CommentSection.ts index 46ca59d8e0f5..95fab1cb7dcc 100644 --- a/browser/src/canvas/sections/CommentSection.ts +++ b/browser/src/canvas/sections/CommentSection.ts @@ -337,9 +337,10 @@ export class Comment extends CanvasSectionObject { lastPosY = childPositions[i].posY + 24; } if (i < this.sectionProperties.childLines.length) { - for (let j = i; j < this.sectionProperties.childLines.length; j++) + for (let j = i; j < this.sectionProperties.childLines.length; j++) { this.sectionProperties.childLinesNode.removeChild(this.sectionProperties.childLines[i]); - this.sectionProperties.childLines.splice(i); + this.sectionProperties.childLines.splice(i); + } } } diff --git a/browser/src/dom/PosAnimation.js b/browser/src/dom/PosAnimation.js index cc258c30ae7a..b5ac8d1ec605 100644 --- a/browser/src/dom/PosAnimation.js +++ b/browser/src/dom/PosAnimation.js @@ -16,6 +16,17 @@ L.PosAnimation = L.Class.extend({ this.fire('start'); + // Skip some work if the duration is zero + if (duration <= 0) { + el.style[L.DomUtil.TRANSITION] = ''; + L.DomUtil.setPosition(el, newPos); + this._el._leaflet_pos = newPos; + this._inProgress = false; + this.fire('step').fire('end'); + this._el.dataset.transitioning = false; + return; + } + el.style[L.DomUtil.TRANSITION] = 'all ' + (isNaN(duration) ? 0.25 : 0) + 's cubic-bezier(0,0,' + (easeLinearity || 0.5) + ',1)'; From 57c9cab258aa3c24f369b3d4bef36d14573d65d9 Mon Sep 17 00:00:00 2001 From: Chris Lord Date: Thu, 19 Dec 2024 13:36:44 +0000 Subject: [PATCH 2/2] Optimisation pass on CommentListSection layout CommentListSection layout appears very high in the profile when scrolling. This commit makes various optimisations, mainly around doing less unnecessary work while scrolling and caching regularly accessed values. Comment layout no longer appears anywhere significant in the profile in Writer after this patch. Signed-off-by: Chris Lord Change-Id: I3bc8d040422703375b583fbc8c49bd793547514d --- .../src/canvas/sections/CommentListSection.ts | 73 ++++++++++--------- browser/src/canvas/sections/CommentSection.ts | 31 ++++++-- 2 files changed, 62 insertions(+), 42 deletions(-) diff --git a/browser/src/canvas/sections/CommentListSection.ts b/browser/src/canvas/sections/CommentListSection.ts index 629c133fd08a..60b7d1f5391f 100644 --- a/browser/src/canvas/sections/CommentListSection.ts +++ b/browser/src/canvas/sections/CommentListSection.ts @@ -94,6 +94,9 @@ export class CommentSection extends app.definitions.canvasSectionObject { // To associate comment id with its index in commentList array. private idIndexMap: Map; + private annotationMinSize: number; + private annotationMaxSize: number; + constructor () { super(); @@ -119,6 +122,8 @@ export class CommentSection extends app.definitions.canvasSectionObject { this.sectionProperties.commentsAreListed = (this.sectionProperties.docLayer._docType === 'text' || this.sectionProperties.docLayer._docType === 'presentation' || this.sectionProperties.docLayer._docType === 'drawing') && !(window).mode.isMobile(); this.idIndexMap = new Map(); this.mobileCommentModalId = this.map.uiManager.generateModalId(this.mobileCommentId); + this.annotationMinSize = Number(getComputedStyle(document.documentElement).getPropertyValue('--annotation-min-size')); + this.annotationMaxSize = Number(getComputedStyle(document.documentElement).getPropertyValue('--annotation-max-size')); } public onInitialize (): void { @@ -1252,7 +1257,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { var previousAnimationState = this.disableLayoutAnimation; this.disableLayoutAnimation = true; - this.update(true); + this.update(true, false); this.disableLayoutAnimation = previousAnimationState; } @@ -1801,10 +1806,10 @@ export class CommentSection extends app.definitions.canvasSectionObject { return this.disableLayoutAnimation ? 0 : undefined; // undefined means it will use default value } - private layoutUp (subList: any, actualPosition: Array, lastY: number): number { + private layoutUp (subList: any, actualPosition: Array, lastY: number, relayout: boolean = true): number { var height: number; for (var i = 0; i < subList.length; i++) { - height = subList[i].getCommentHeight(); + height = subList[i].getCommentHeight(relayout); lastY = subList[i].sectionProperties.data.anchorPix[1] + height < lastY ? subList[i].sectionProperties.data.anchorPix[1]: lastY - (height * app.dpiScale); (new L.PosAnimation()).run(subList[i].sectionProperties.container, {x: Math.round(actualPosition[0] / app.dpiScale), y: Math.round(lastY / app.dpiScale)}, this.getAnimationDuration()); if (!subList[i].isEdit()) @@ -1813,7 +1818,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { return lastY; } - private loopUp (startIndex: number, x: number, startY: number): number { + private loopUp (startIndex: number, x: number, startY: number, relayout: boolean = true): number { var tmpIdx = 0; var checkSelectedPart: boolean = this.mustCheckSelectedPart(); startY -= this.sectionProperties.marginY; @@ -1834,7 +1839,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { } while (tmpIdx > -1 && this.sectionProperties.commentList[tmpIdx].sectionProperties.data.parent === this.sectionProperties.commentList[tmpIdx + 1].sectionProperties.data.id); if (subList.length > 0) { - startY = this.layoutUp(subList, [x, subList[0].sectionProperties.data.anchorPix[1]], startY); + startY = this.layoutUp(subList, [x, subList[0].sectionProperties.data.anchorPix[1]], startY, relayout); i = i - subList.length; } else { i = tmpIdx; @@ -1844,7 +1849,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { return startY; } - private layoutDown (subList: any, actualPosition: Array, lastY: number): number { + private layoutDown (subList: any, actualPosition: Array, lastY: number, relayout: boolean = true): number { var selectedComment = subList[0] === this.sectionProperties.selectedComment; for (var i = 0; i < subList.length; i++) { lastY = subList[i].sectionProperties.data.anchorPix[1] > lastY ? subList[i].sectionProperties.data.anchorPix[1]: lastY; @@ -1852,6 +1857,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { var isRTL = document.documentElement.dir === 'rtl'; if (selectedComment) { + // FIXME: getBoundingClientRect is expensive and this is a hot path (called continuously during animations and scrolling) const posX = (this.sectionProperties.showSelectedBigger ? Math.round((document.getElementById('document-container').getBoundingClientRect().width - subList[i].sectionProperties.container.getBoundingClientRect().width)/2) : Math.round(actualPosition[0] / app.dpiScale) - this.sectionProperties.deflectionOfSelectedComment * (isRTL ? -1 : 1)); @@ -1860,14 +1866,14 @@ export class CommentSection extends app.definitions.canvasSectionObject { else (new L.PosAnimation()).run(subList[i].sectionProperties.container, {x: Math.round(actualPosition[0] / app.dpiScale), y: Math.round(lastY / app.dpiScale)}, this.getAnimationDuration()); - lastY += (subList[i].getCommentHeight() * app.dpiScale); + lastY += (subList[i].getCommentHeight(relayout) * app.dpiScale); if (!subList[i].isEdit()) subList[i].show(); } return lastY; } - private loopDown (startIndex: number, x: number, startY: number): number { + private loopDown (startIndex: number, x: number, startY: number, relayout: boolean = true): number { var tmpIdx = 0; var checkSelectedPart: boolean = this.mustCheckSelectedPart(); // Pass over all comments present @@ -1887,7 +1893,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { } while (tmpIdx < this.sectionProperties.commentList.length && this.sectionProperties.commentList[tmpIdx].sectionProperties.data.parent !== '0'); if (subList.length > 0) { - startY = this.layoutDown(subList, [x, subList[0].sectionProperties.data.anchorPix[1]], startY); + startY = this.layoutDown(subList, [x, subList[0].sectionProperties.data.anchorPix[1]], startY, relayout); i = i + subList.length; } else { i = tmpIdx; @@ -1901,6 +1907,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { if (this.sectionProperties.arrow) { document.getElementById('document-container').removeChild(this.sectionProperties.arrow); this.sectionProperties.arrow = null; + app.sectionContainer.requestReDraw(); } } @@ -1916,7 +1923,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { endPoint[1] = Math.floor(endPoint[1] / app.dpiScale); if (this.sectionProperties.arrow !== null) { - var line: SVGLineElement = (document.getElementById('comment-arrow-line')); + var line: SVGLineElement = (this.sectionProperties.arrow.firstElementChild); line.setAttribute('x1', String(startPoint[0])); line.setAttribute('y1', String(startPoint[1])); line.setAttribute('x2', String(endPoint[0])); @@ -1945,7 +1952,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { } } - private doLayout (): void { + private doLayout (relayout: boolean = true): void { if ((window).mode.isMobile() || this.sectionProperties.docLayer._docType === 'spreadsheet') { if (this.sectionProperties.commentList.length > 0) this.orderCommentList(); @@ -1954,7 +1961,8 @@ export class CommentSection extends app.definitions.canvasSectionObject { if (this.sectionProperties.commentList.length > 0) { this.orderCommentList(); - this.resetCommentsSize(); + if (relayout) + this.resetCommentsSize(); var isRTL = document.documentElement.dir === 'rtl'; @@ -1986,21 +1994,20 @@ export class CommentSection extends app.definitions.canvasSectionObject { this.showArrow([tempCrd[0], tempCrd[1]], [posX, tempCrd[1]]); } } - else { + else this.hideArrow(); - app.sectionContainer.requestReDraw(); - } var lastY = 0; if (selectedIndex) { - this.loopUp(selectedIndex - 1, x, yOrigin); - lastY = this.loopDown(selectedIndex, x, yOrigin); + this.loopUp(selectedIndex - 1, x, yOrigin, relayout); + lastY = this.loopDown(selectedIndex, x, yOrigin, relayout); } else { - lastY = this.loopDown(0, x, topRight[1]); + lastY = this.loopDown(0, x, topRight[1], relayout); } } - this.resizeComments(); + if (relayout) + this.resizeComments(); lastY += this.containerObject.getDocumentTopLeft()[1]; if (lastY > app.file.size.pixels[1]) { @@ -2013,21 +2020,21 @@ export class CommentSection extends app.definitions.canvasSectionObject { this.disableLayoutAnimation = false; } - private layout (immediate: any = null): void { + private layout (immediate: any = null, relayout: boolean = true): void { if (immediate) - this.doLayout(); + this.doLayout(relayout); else if (!this.sectionProperties.layoutTimer) { - this.sectionProperties.layoutTimer = setTimeout(function() { + this.sectionProperties.layoutTimer = setTimeout(() => { delete this.sectionProperties.layoutTimer; - this.doLayout(); - }.bind(this), 10 /* ms */); + this.doLayout(relayout); + }, 10 /* ms */); } // else - avoid excessive re-layout } - private update (immediate: boolean = false): void { - if (this.sectionProperties.docLayer._docType === 'text') + private update (immediate: boolean = false, relayout: boolean = true): void { + if (relayout && this.sectionProperties.docLayer._docType === 'text') this.updateThreadInfoIndicator(); - this.layout(immediate); + this.layout(immediate, relayout); } private updateThreadInfoIndicator(): void { @@ -2143,15 +2150,11 @@ export class CommentSection extends app.definitions.canvasSectionObject { // reset theis size to default (100px text) private resetCommentsSize (): void { if (this.sectionProperties.docLayer._docType === 'text') { - const minMaxHeight = Number(getComputedStyle(document.documentElement).getPropertyValue('--annotation-min-size')); for (var i = 0; i < this.sectionProperties.commentList.length;i++) { - if (this.sectionProperties.commentList[i].sectionProperties.contentNode.style.display !== 'none') - this.sectionProperties.commentList[i].sectionProperties.contentNode.style.maxHeight = minMaxHeight + 'px'; - } - if (this.sectionProperties.selectedComment) { - if (this.sectionProperties.selectedComment.sectionProperties.contentNode.style.display !== 'none') { - const maxMaxHeight = Number(getComputedStyle(document.documentElement).getPropertyValue('--annotation-max-size')); - this.sectionProperties.selectedComment.sectionProperties.contentNode.style.maxHeight = maxMaxHeight + 'px'; + if (this.sectionProperties.commentList[i].sectionProperties.contentNode.style.display !== 'none') { + const maxHeight = (this.sectionProperties.commentList[i] === this.sectionProperties.selectedComment) ? + this.annotationMaxSize : this.annotationMinSize; + this.sectionProperties.commentList[i].sectionProperties.contentNode.style.maxHeight = maxHeight + 'px'; } } } diff --git a/browser/src/canvas/sections/CommentSection.ts b/browser/src/canvas/sections/CommentSection.ts index 95fab1cb7dcc..dd55f27dc2ae 100644 --- a/browser/src/canvas/sections/CommentSection.ts +++ b/browser/src/canvas/sections/CommentSection.ts @@ -44,6 +44,9 @@ export class Comment extends CanvasSectionObject { map: any; pendingInit: boolean = true; + cachedCommentHeight: number | null = null; + hidden: boolean | null = null; + // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types constructor (data: any, options: any, commentListSectionPointer: cool.CommentSection) { super(); @@ -321,7 +324,7 @@ export class Comment extends CanvasSectionObject { posY: this.sectionProperties.children[i].sectionProperties.container._leaflet_pos.y}); } childPositions.sort((a, b) => { return a.posY - b.posY; }); - let lastPosY = this.sectionProperties.container._leaflet_pos.y + this.getCommentHeight(); + let lastPosY = this.sectionProperties.container._leaflet_pos.y + this.getCommentHeight(false); let i = 0; for (; i < childPositions.length; i++) { if (this.sectionProperties.childLines[i] === undefined) { @@ -844,6 +847,8 @@ export class Comment extends CanvasSectionObject { } public show(): void { + if (this.hidden === false) return; + this.doPendingInitializationInView(true /* force */); this.showMarker(); @@ -853,11 +858,16 @@ export class Comment extends CanvasSectionObject { if (cool.CommentSection.commentWasAutoAdded) return; - if (this.sectionProperties.docLayer._docType === 'text') + + // We don't cache the hidden state for spreadsheets. Only one comment can be + // visible and they're hidden when scrolling, so it's easier this way. + if (this.sectionProperties.docLayer._docType === 'text') { this.showWriter(); - else if (this.sectionProperties.docLayer._docType === 'presentation' || this.sectionProperties.docLayer._docType === 'drawing') + this.hidden = false; + } else if (this.sectionProperties.docLayer._docType === 'presentation' || this.sectionProperties.docLayer._docType === 'drawing') { this.showImpressDraw(); - else if (this.sectionProperties.docLayer._docType === 'spreadsheet') + this.hidden = false; + } else if (this.sectionProperties.docLayer._docType === 'spreadsheet') this.showCalc(); this.setLayoutClass(); @@ -869,6 +879,7 @@ export class Comment extends CanvasSectionObject { this.sectionProperties.nodeReply.style.display = 'none'; this.sectionProperties.showSelectedCoordinate = false; L.DomUtil.removeClass(this.sectionProperties.container, 'cool-annotation-collapsed-show'); + this.hidden = true; } private hideCalc() { @@ -894,6 +905,7 @@ export class Comment extends CanvasSectionObject { this.sectionProperties.nodeReply.style.display = 'none'; } L.DomUtil.removeClass(this.sectionProperties.container, 'cool-annotation-collapsed-show'); + this.hidden = true; } // check if this is "our" autosaved comment @@ -908,7 +920,7 @@ export class Comment extends CanvasSectionObject { } public hide (): void { - if (this.isEdit()) { + if (this.hidden === true || this.isEdit()) { return; } @@ -1511,8 +1523,13 @@ export class Comment extends CanvasSectionObject { return 1; // Comment list not fully initialized but we know we are not root } - public getCommentHeight(): number { - return this.sectionProperties.container.getBoundingClientRect().height - this.sectionProperties.childLinesNode.getBoundingClientRect().height; + public getCommentHeight(invalidateCache: boolean = true): number { + if (invalidateCache) + this.cachedCommentHeight = null; + if (this.cachedCommentHeight === null) + this.cachedCommentHeight = this.sectionProperties.container.getBoundingClientRect().height + - this.sectionProperties.childLinesNode.getBoundingClientRect().height; + return this.cachedCommentHeight; } public setCollapsed(): void {