From a0ce46193ec9a29ae11118b0116e2b0882eb446e Mon Sep 17 00:00:00 2001 From: ator-dev Date: Wed, 11 Sep 2024 23:50:01 +0100 Subject: [PATCH] Enhance Paint, fire FlowTracker listeners less - Fire spansRemovedListener only if the number of spans removed is actually positive (FlowTracker) - Fire newSpanOwnerListener only if the flow just calculated actually contains spans (and is the first one to do so) (FlowTracker) - Enhance some Paint methods - Fix error spam in Paint's "paint" method - Do not delete from elementDrawContainerMap just before setting in the same element slot, in Paint's "element" method --- src/modules/highlight/engines/paint.mts | 10 ++++- .../engines/paint/methods/element.mts | 5 ++- .../highlight/engines/paint/methods/paint.mts | 12 ++++-- .../models/tree-cache/flow-tracker.mts | 38 +++++++++++++------ src/paint.ts | 33 ++++++++++------ 5 files changed, 67 insertions(+), 31 deletions(-) diff --git a/src/modules/highlight/engines/paint.mts b/src/modules/highlight/engines/paint.mts index b2226a3..9b25f95 100644 --- a/src/modules/highlight/engines/paint.mts +++ b/src/modules/highlight/engines/paint.mts @@ -74,9 +74,12 @@ class PaintEngine implements AbstractTreeCacheEngine, HighlightingStyleObserver this.#termTokens = termTokens; this.#flowTracker = new FlowTracker(this.terms, termPatterns); this.#flowTracker.setNewSpanOwnerListener(flowOwner => { + if (this.#method.highlightables) { + flowOwner = this.#method.highlightables.findHighlightableAncestor(flowOwner); + } this.observeVisibilityChangesFor(flowOwner); if (!this.#elementHighlightingIdMap.has(flowOwner)) { - const id = highlightingId.next().value; + const id = highlightingIds.next().value; this.#elementHighlightingIdMap.set(flowOwner, id); // NOTE: Some webpages may remove unknown attributes. It is possible to check and re-apply it from cache. // TODO make sure there is cleanup once the highlighting ID becomes invalid (e.g. when the cache is removed). @@ -89,6 +92,9 @@ class PaintEngine implements AbstractTreeCacheEngine, HighlightingStyleObserver } }); this.#flowTracker.setNonSpanOwnerListener(flowOwner => { + if (this.#method.highlightables) { + flowOwner = this.#method.highlightables.findHighlightableAncestor(flowOwner); + } // TODO this is done for consistency with the past behaviour; but is it right/necessary? this.#elementHighlightingIdMap.delete(flowOwner); this.#elementStyleRuleMap.delete(flowOwner); @@ -161,7 +167,7 @@ class PaintEngine implements AbstractTreeCacheEngine, HighlightingStyleObserver visibilityObserver.disconnect(); }; } - const highlightingId = (function* () { + const highlightingIds = (function* () { let i = 0; while (true) { yield i++; diff --git a/src/modules/highlight/engines/paint/methods/element.mts b/src/modules/highlight/engines/paint/methods/element.mts index 6f8fc2d..0c46b99 100644 --- a/src/modules/highlight/engines/paint/methods/element.mts +++ b/src/modules/highlight/engines/paint/methods/element.mts @@ -77,12 +77,13 @@ class ElementMethod implements AbstractMethod { this.#elementDrawContainerMap.delete(element); } for (const element of newlyStyledElements) { - this.#elementDrawContainerMap.get(element)?.remove(); - this.#elementDrawContainerMap.delete(element); const container = this.getDrawElementContainer(element); + this.#elementDrawContainerMap.get(element)?.remove(); if (container) { this.#elementDrawContainerMap.set(element, container); this.#drawContainersParent.appendChild(container); + } else { + this.#elementDrawContainerMap.delete(element); } } }); diff --git a/src/modules/highlight/engines/paint/methods/paint.mts b/src/modules/highlight/engines/paint/methods/paint.mts index e5517e3..cb5e5f2 100644 --- a/src/modules/highlight/engines/paint/methods/paint.mts +++ b/src/modules/highlight/engines/paint/methods/paint.mts @@ -12,7 +12,7 @@ import { StyleManager } from "/dist/modules/style-manager.mjs"; import { HTMLStylesheet } from "/dist/modules/stylesheets/html.mjs"; import { EleID, EleClass } from "/dist/modules/common.mjs"; -type TermSelectorStyles = Record @@ -49,7 +49,7 @@ class PaintMethod implements AbstractMethod { endHighlighting () {} getTermsCSS (terms: ReadonlyArray, hues: ReadonlyArray) { - const styles: TermSelectorStyles = {}; + const styles: TermTokenStyles = {}; for (let i = 0; i < terms.length; i++) { styles[this.#termTokens.get(terms[i])] = { hue: hues[i % hues.length], @@ -69,7 +69,8 @@ class PaintMethod implements AbstractMethod { --markmysearch-styles: unset; --markmysearch-boxes: unset; } -}` +} +` ); } @@ -100,4 +101,7 @@ class PaintMethod implements AbstractMethod { } } -export { type TermSelectorStyles, PaintMethod }; +export { + type TermTokenStyles, + PaintMethod, +}; diff --git a/src/modules/highlight/models/tree-cache/flow-tracker.mts b/src/modules/highlight/models/tree-cache/flow-tracker.mts index deb5783..2e02caf 100644 --- a/src/modules/highlight/models/tree-cache/flow-tracker.mts +++ b/src/modules/highlight/models/tree-cache/flow-tracker.mts @@ -167,10 +167,12 @@ class FlowTracker implements AbstractFlowTracker { const flows = this.#elementFlowsMap.get(element); if (flows) { this.#elementFlowsMap.delete(element); - if (this.#spansRemovedListener) + if (this.#spansRemovedListener) { this.#spansRemovedListener(element, flows.flatMap(flow => flow.spans)); - if (this.#nonSpanOwnerListener) + } + if (this.#nonSpanOwnerListener) { this.#nonSpanOwnerListener(element); + } } // eslint-disable-next-line no-cond-assign } while (element = walker.nextNode()); @@ -196,25 +198,28 @@ class FlowTracker implements AbstractFlowTracker { }); if (flowsNew.length > 0) { this.#elementFlowsMap.set(element, flowsNew); - if (spansRemoved.length > 0) { - if (this.#spansRemovedListener) - this.#spansRemovedListener(element, spansRemoved); + if (this.#spansRemovedListener && spansRemoved.length > 0) { + this.#spansRemovedListener(element, spansRemoved); } } else { this.#elementFlowsMap.delete(element); - if (this.#spansRemovedListener) + if (this.#spansRemovedListener) { this.#spansRemovedListener(element, spansRemoved); - if (this.#nonSpanOwnerListener) + } + if (this.#nonSpanOwnerListener) { this.#nonSpanOwnerListener(element); + } } } } else { if (this.#spansRemovedListener || this.#nonSpanOwnerListener) { for (const [ element, flows ] of this.#elementFlowsMap) { - if (this.#spansRemovedListener) + if (this.#spansRemovedListener) { this.#spansRemovedListener(element, flows.flatMap(flow => flow.spans)); - if (this.#nonSpanOwnerListener) + } + if (this.#nonSpanOwnerListener) { this.#nonSpanOwnerListener(element); + } } } this.#elementFlowsMap.clear(); @@ -284,12 +289,21 @@ class FlowTracker implements AbstractFlowTracker { this.#elementFlowsMap.set(ancestor, flows); } flows.push({ text, spans: spansCreated }); - if (flows.length === 1) { - if (this.#newSpanOwnerListener) + if (this.#newSpanOwnerListener) { + if ( + // If: the new flow contains spans + spansCreated.length > 0 + // AND the total number of spans equals the number of spans in the new flow + && (flows.reduce((previous, current) => previous + current.spans.length, 0) + === spansCreated.length) + // So: the element just became a span owner + ) { this.#newSpanOwnerListener(ancestor); + } } - if (this.#spansCreatedListener) + if (this.#spansCreatedListener && spansCreated.length > 0) { this.#spansCreatedListener(ancestor, spansCreated); + } } getAncestorCommon (nodeA_ancestor: HTMLElement, nodeB: Node): HTMLElement | null { diff --git a/src/paint.ts b/src/paint.ts index 38eae31..b667d62 100644 --- a/src/paint.ts +++ b/src/paint.ts @@ -5,7 +5,7 @@ */ import type { Box } from "/dist/modules/highlight/engines/paint.mjs"; -import type { TermSelectorStyles } from "/dist/modules/highlight/engines/paint/methods/paint.mjs"; +import type { TermTokenStyles } from "/dist/modules/highlight/engines/paint/methods/paint.mjs"; registerPaint("markmysearch-highlights", class { static get inputProperties () { @@ -16,27 +16,38 @@ registerPaint("markmysearch-highlights", class { } paint ( - ctx: PaintRenderingContext2D, - geom: PaintSize, + context: PaintRenderingContext2D, + size: PaintSize, properties: StylePropertyMapReadOnly, ) { - const selectorStyles = JSON.parse(properties.get("--markmysearch-styles")?.toString() ?? "{}") as TermSelectorStyles; - const boxes = JSON.parse(properties.get("--markmysearch-boxes")?.toString() ?? "[]") as Array; + // Using ` || ` instead of ` ?? ` + // means that the empty string (a falsy value) will also shortcut to the right-hand-side. + // This is important because properties.get() could return a CSSUnparsedValue which + // evaluates to the empty string, which is not valid JSON. This happens regularly during + // normal operation, and after investigation seems to be a quirk of CSS. + const termTokenStyles = JSON.parse( + // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing + properties.get("--markmysearch-styles")?.toString() || "{}" + ) as TermTokenStyles; + const boxes = JSON.parse( + // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing + properties.get("--markmysearch-boxes")?.toString() || "[]" + ) as Array; for (const box of boxes) { - const style = selectorStyles[box.token]; + const style = termTokenStyles[box.token]; if (!style) { - return; + continue; } - ctx.strokeStyle = `hsl(${style.hue} 100% 10% / 0.4)`; - ctx.strokeRect(box.x, box.y, box.width, box.height); + context.strokeStyle = `hsl(${style.hue} 100% 10% / 0.4)`; + context.strokeRect(box.x, box.y, box.width, box.height); const height = box.height / Math.floor((style.cycle + 3) / 2); if (style.cycle === 0) { // Special case: 1st cycle (only one with a single block of color) must begin with the lightness closer to 50%. style.cycle = -1; } for (let i = 0; i <= (style.cycle + 1) / 2; i++) { - ctx.fillStyle = `hsl(${style.hue} 100% ${(i % 2 == style.cycle % 2) ? 98 : 60}% / 0.4)`; - ctx.fillRect(box.x, box.y + height*i, box.width, height); + context.fillStyle = `hsl(${style.hue} 100% ${(i % 2 == style.cycle % 2) ? 98 : 60}% / 0.4)`; + context.fillRect(box.x, box.y + height*i, box.width, height); } } }