From 53ee084307f060f8b1aac10cfd152a521aa6a129 Mon Sep 17 00:00:00 2001 From: Ben Kennedy Date: Thu, 5 Nov 2026 12:44:35 +0000 Subject: [PATCH 1/6] Cut more page shapes with one callback The wrapper now owns the shapes it used to hand back to the element path. What other components insert inside the cut is removed with it, whatever their order, and a marker another component removes still marks the cut, so the inserting-component watchers and the removed-marker checks are gone. A container holding raw text is left alone. Nested containers are tracked by depth and only the outermost is cut, so a marker that is a direct child of an inner one is still a nested marker. Container and marker selector lists are split per part before the `>` and descendant selectors are built, and the element path now builds its descendant selectors the same way. Each range component gets its own wrapper tag, so several ranges and other content on the range component take the fast path. An element that is the container of two ranges, or the container of one and a marker of another, sends the page back to the element path. The fuzz compares the wrapper with each range applied element by element on its own ahead of the other components, and covers selector lists, several ranges and content on the range component. --- .changeset/range-wrapper-more-shapes.md | 5 + src/stages/buildReplacementHandlers.ts | 3 +- src/stages/findMarkerPositions.ts | 7 +- src/stages/surfaceComponents.ts | 11 +- src/stages/wrappedRange.ts | 364 ++++++++++++++---------- test/fakes/EndTagSpyRewriter.ts | 4 +- test/stages/wrappedRange.spec.ts | 340 +++++++++++++++++----- 7 files changed, 493 insertions(+), 241 deletions(-) create mode 100644 .changeset/range-wrapper-more-shapes.md diff --git a/.changeset/range-wrapper-more-shapes.md b/.changeset/range-wrapper-more-shapes.md new file mode 100644 index 00000000..7a8a0658 --- /dev/null +++ b/.changeset/range-wrapper-more-shapes.md @@ -0,0 +1,5 @@ +--- +"@monetizationos/proxy": minor +--- + +More pages use the one-callback range cut. Content other components insert inside the range is removed with it, whatever the component order, and a marker removed by another component still marks where the range starts or ends. A container holding raw text, such as `textarea` or `script`, is left alone, and only the outermost of nested containers is cut. Selector lists, several range components in one decision, and other content on the range component all use the one-callback path. Selector lists also apply per part when cutting element by element. diff --git a/src/stages/buildReplacementHandlers.ts b/src/stages/buildReplacementHandlers.ts index 9983fcb7..2f783ac6 100644 --- a/src/stages/buildReplacementHandlers.ts +++ b/src/stages/buildReplacementHandlers.ts @@ -4,6 +4,7 @@ import type { SubSurfaceBehaviorApi } from '../types' import { renderElement } from './elements' import type { ComponentRange } from './findMarkerPositions' import { isVoidElement } from './voidElements' +import { combineSelectors } from './wrappedRange' type ReplacementState = { // `IN_START`: a start marker nested in the first one doesn't start the range again @@ -160,6 +161,6 @@ export const buildReplacementHandlers = ( } return [ { selector, handler: parentHandler }, - { selector: `${selector} *`, handler: { element: onChildElement } }, + { selector: combineSelectors(selector, ' ', '*'), handler: { element: onChildElement } }, ] } diff --git a/src/stages/findMarkerPositions.ts b/src/stages/findMarkerPositions.ts index a06347fb..eed61b41 100644 --- a/src/stages/findMarkerPositions.ts +++ b/src/stages/findMarkerPositions.ts @@ -2,6 +2,7 @@ import type { ElementHandlers, HtmlRewriterAdapter, RewriterElement } from '../a import type { HtmlStageContext } from '../context' import type { SurfaceDecisionResponse } from '../types' import { isVoidElement } from './voidElements' +import { combineSelectors } from './wrappedRange' export type SurfaceMarkers = { componentsWithInvalidSelectors: string[] @@ -81,7 +82,7 @@ export const findMarkerPositions = async ( }, }, { - selector: `${componentBehavior.metadata.cssSelector} *`, + selector: combineSelectors(componentBehavior.metadata.cssSelector, ' ', '*'), componentKey, handler: { element(element: RewriterElement) { @@ -113,7 +114,7 @@ export const findMarkerPositions = async ( }, toMarker ? { - selector: `${componentBehavior.metadata.cssSelector} ${toMarker}`, + selector: combineSelectors(componentBehavior.metadata.cssSelector, ' ', toMarker), componentKey, handler: { element() { @@ -139,7 +140,7 @@ export const findMarkerPositions = async ( : null, fromMarker ? { - selector: `${componentBehavior.metadata.cssSelector} ${fromMarker}`, + selector: combineSelectors(componentBehavior.metadata.cssSelector, ' ', fromMarker), componentKey, handler: { element() { diff --git a/src/stages/surfaceComponents.ts b/src/stages/surfaceComponents.ts index ccb31366..7848d7e7 100644 --- a/src/stages/surfaceComponents.ts +++ b/src/stages/surfaceComponents.ts @@ -34,7 +34,11 @@ export default async function handleSurfaceComponents( if (wrapper.wrapped() === 0) { return new Response(wrappedPage, init) } - return rewriter.create().on(wrapper.unwrap.selector, wrapper.unwrap.handler).transform(new Response(wrappedPage, init)) + let unwrap = rewriter.create() + for (const { selector, handler } of wrapper.unwrap) { + unwrap = unwrap.on(selector, handler) + } + return unwrap.transform(new Response(wrappedPage, init)) } /** With no `wrapper`, ranges are cut element by element. */ @@ -89,9 +93,8 @@ export async function applyComponents( }, ] - if (wrapper && componentKey === wrapper.componentKey) { - registerWrapper(wrapper.checks) - } else if (supportsRangeReplacement && !componentsWithInvalidSelectors.includes(componentKey)) { + const cutByWrapper = wrapper?.componentKeys.includes(componentKey) + if (supportsRangeReplacement && !cutByWrapper && !componentsWithInvalidSelectors.includes(componentKey)) { handlers.push(...buildReplacementHandlers(ctx, componentBehavior, markers[componentKey])) } else if (!supportsRangeReplacement && componentBehavior.content.replaceRange) { logger.log({ diff --git a/src/stages/wrappedRange.ts b/src/stages/wrappedRange.ts index afc59398..688d92f0 100644 --- a/src/stages/wrappedRange.ts +++ b/src/stages/wrappedRange.ts @@ -1,6 +1,6 @@ import type { ElementHandlers, RewriterElement } from '../adapters/HtmlRewriterAdapter' import type { HtmlStageContext } from '../context' -import type { SubSurfaceBehaviorApi, SurfaceDecisionResponse } from '../types' +import type { SurfaceDecisionResponse } from '../types' import { renderElement } from './elements' import { isVoidElement } from './voidElements' @@ -9,78 +9,116 @@ type Handler = { selector: string; handler: ElementHandlers } // Their content is text, so a wrapper tag inserted into it is never parsed as a tag const RAW_TEXT_ELEMENTS = new Set(['iframe', 'noembed', 'noframes', 'noscript', 'plaintext', 'script', 'style', 'textarea', 'title', 'xmp']) -const INSERTIONS = ['prepend', 'append', 'before', 'after'] as const - const withoutNested = (selector: string) => selector.replace(/"[^"]*"|'[^']*'|\[[^\]]*\]|\([^)]*\)/g, '') // One compound selector, so `${container} > ${marker}` only matches a direct child -const isCompound = (selector: string) => !/[\s>+~,]/.test(withoutNested(selector.trim())) +const isCompound = (selector: string) => !/[\s>+~]/.test(withoutNested(selector.trim())) + +/** The top-level parts of a selector list. */ +export const splitSelectorList = (selector: string): string[] => { + const parts: string[] = [] + let nesting = 0 + let quote = '' + let start = 0 + for (let i = 0; i < selector.length; i++) { + const char = selector[i] + if (char === '\\') { + i++ + } else if (quote) { + quote = char === quote ? '' : quote + } else if (char === '"' || char === "'") { + quote = char + } else if (char === '(' || char === '[') { + nesting++ + } else if (char === ')' || char === ']') { + nesting-- + } else if (char === ',' && nesting === 0) { + parts.push(selector.slice(start, i).trim()) + start = i + 1 + } + } + parts.push(selector.slice(start).trim()) + return parts +} + +/** `${container}${combinator}${inner}` for every pair of parts, so a list on either side keeps the combinator. */ +export const combineSelectors = (container: string, combinator: ' ' | ' > ', inner: string) => + splitSelectorList(container) + .flatMap((outer) => splitSelectorList(inner).map((part) => `${outer}${combinator}${part}`)) + .join(', ') export type RangeWrapper = { - componentKey: string + componentKeys: string[] /** Pass 1, ahead of every other component: split each container into `` segments. */ handlers: Handler[] - /** Pass 1, in the range component's own place, so they see markers removed by the components before it. */ - checks: Handler[] - /** Pass 2: drop the segment holding the range and put the replacement there; unwrap the rest. */ - unwrap: Handler + /** Pass 2: drop the segment holding each range and put its replacement there; unwrap the rest. */ + unwrap: Handler[] wrapped: () => number - /** Pass 1 met a page the wrapper can't cut the way the element path would, so the caller must redo it that way. */ + /** Pass 1 met a page the wrapper can't cut, so the caller must redo it element by element. */ unsupported: () => boolean fail: () => void } +type RangeState = { + depth: number + // `inStart`: a start marker nested in the first one doesn't start the range again + phase: 'scanning' | 'inStart' | 'open' | 'closed' + // Set by a `>` handler for the check on the same element, which runs after it + directStart: boolean + directEnd: boolean +} + /** - * Cuts a range by splitting its container into wrapper elements in one pass and removing the one that holds the range - * in the next, so the cut costs one callback instead of one per element. The split only balances when both markers - * are direct children of the container. Only a decision with exactly one range component, and no other content on - * that component, qualifies. + * Cuts each range by splitting its container into wrapper elements in one pass and removing the one that holds the + * range in the next, so the cut costs one callback instead of one per element. The split only balances when both + * markers are direct children of the outermost container; a nested marker makes the page unsupported. What other + * components insert inside the cut is removed with it, and a marker removed by another component still marks the cut. + * A container holding raw text, or inside `math`, is left alone. */ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisionResponse): RangeWrapper | null => { - const ranges = Object.entries(decisions.componentBehaviors).filter(([, b]) => b.content?.replaceRange && b.metadata.cssSelector) - if (ranges.length !== 1) { + const ranges = Object.entries(decisions.componentBehaviors).flatMap(([componentKey, behavior]) => { + const replaceRange = behavior.content?.replaceRange + const container = behavior.metadata.cssSelector + // The component path skips `:last-child` containers + return replaceRange && container && !container.includes(':last-child') ? [{ componentKey, replaceRange, container }] : [] + }) + if (!ranges.length) { return null } - const [componentKey, behavior] = ranges[0] as [string, SubSurfaceBehaviorApi] - const { replaceRange, ...others } = behavior.content ?? {} - if (!replaceRange || Object.values(others).some((value) => (Array.isArray(value) ? value.length : value))) { - return null - } - const { fromMarker, toMarker } = replaceRange - if (fromMarker && toMarker && fromMarker === toMarker) { - return null + for (const { replaceRange } of ranges) { + const { fromMarker, toMarker } = replaceRange + if (fromMarker && toMarker && fromMarker === toMarker) { + return null + } + for (const marker of [fromMarker, toMarker]) { + if (marker && !splitSelectorList(marker).every(isCompound)) { + return null + } + } } - const container = behavior.metadata.cssSelector as string - if ((fromMarker && !isCompound(fromMarker)) || (toMarker && !isCompound(toMarker)) || withoutNested(container).includes(',')) { - return null - } - // Components ahead of the range whose inserts the element path places differently, or keeps where the wrapper cuts - const keys = Object.keys(decisions.componentBehaviors) - const inserting = keys - .slice(0, keys.indexOf(componentKey)) - .map((key) => decisions.componentBehaviors[key]) - .filter((b) => b?.metadata.cssSelector && INSERTIONS.some((key) => b.content?.[key]?.length)) - .map((b) => b?.metadata.cssSelector as string) - - const tag = `mos-range-${Math.random().toString(36).slice(2, 10)}` - const cut = `<${tag}>` - const keep = `<${tag} data-keep>` - const close = `` + + const nonce = Math.random().toString(36).slice(2, 10) const html = { html: true } - const replacement = (replaceRange.replaceWith ?? []).map((item) => renderElement(ctx, item)) let unsupported = false let wrapped = 0 - let depth = 0 - // `inStart`: a start marker nested in the first one doesn't start the range again - let phase: 'scanning' | 'inStart' | 'open' | 'closed' = 'closed' - // Set by a `>` handler for the check on the same element, which runs after it - let directStart = false - let directEnd = false - // Cleared by the range's own check on the container, after the components ahead of it - let atContainer = false + // Ranges whose container handler has run for the current element, cleared once its marker handlers have run + let atContainer = 0 // The rewriter mismatches the wrapper's end tags inside MathML let mathDepth = 0 + // Only the last end tag callback registered on an element runs, so one registered on `math` keeps its count + const onEnd = (element: RewriterElement, callback: () => void) => { + const math = element.tagName.toLowerCase() === 'math' + element.onEndTag?.( + math + ? () => { + mathDepth-- + callback() + } + : callback, + ) + } + const handlers: Handler[] = [ { selector: 'math', @@ -92,141 +130,154 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi mathDepth-- }) } catch { - // Self-closing ``, so nothing is inside it mathDepth-- } }, }, }, - { + ] + const markers: Handler[] = [] + const cleanups: Handler[] = [] + const unwrap: Handler[] = [] + + ranges.forEach(({ replaceRange, container }, index) => { + const { fromMarker, toMarker } = replaceRange + const tag = `mos-range-${nonce}-${index}` + const cut = `<${tag}>` + const keep = `<${tag} data-keep>` + const close = `` + const state: RangeState = { depth: 0, phase: 'closed', directStart: false, directEnd: false } + + handlers.push({ selector: container, handler: { element(element: RewriterElement) { + // The same element holding two ranges would need two end tag callbacks + unsupported ||= atContainer++ > 0 if (isVoidElement(element)) { return } - if (!element.onEndTag || RAW_TEXT_ELEMENTS.has(element.tagName.toLowerCase()) || mathDepth > 0 || depth > 0) { + if (!element.onEndTag) { unsupported = true return } - atContainer = true - depth++ - phase = fromMarker ? 'scanning' : 'open' - // Closed by the container's own end, so what other components append lands outside it - element.prepend(fromMarker ? keep : cut, html) - element.append(close, html) - wrapped++ - try { - element.onEndTag(() => { - phase = 'closed' - depth-- - }) - } catch { - // Self-closing foreign element (``), so there is no end tag to close the cut at - unsupported = true - } - }, - }, - }, - ] - const checks: Handler[] = [ - { - selector: container, - handler: { - element() { - atContainer = false - }, - }, - }, - ] - - if (toMarker) { - handlers.push({ - selector: `${container} > ${toMarker}`, - handler: { - element(element: RewriterElement) { - if (phase === 'open') { - element.before(close + keep, html) - phase = 'closed' - directEnd = true - } - }, - }, - }) - checks.push({ - selector: `${container} ${toMarker}`, - handler: { - element(element: RewriterElement) { - if (directEnd) { - directEnd = false - unsupported ||= element.removed - } else if (phase === 'open') { - unsupported = true - } - }, - }, - }) - } - if (fromMarker) { - handlers.push({ - selector: `${container} > ${fromMarker}`, - handler: { - element(element: RewriterElement) { - if (phase !== 'scanning') { + if (state.depth > 0) { + // Only the outermost container is cut; counting this one marks the markers in it as nested + try { + onEnd(element, () => { + state.depth-- + }) + state.depth++ + } catch { + // No end tag, so nothing is nested in it + } return } - element.after(close + cut, html) - directStart = true - phase = 'inStart' - if (!element.onEndTag || isVoidElement(element)) { - phase = 'open' + if (RAW_TEXT_ELEMENTS.has(element.tagName.toLowerCase()) || mathDepth > 0) { return } try { - element.onEndTag(() => { - phase = 'open' + onEnd(element, () => { + state.phase = 'closed' + state.depth-- }) } catch { - // Self-closing foreign element, so nothing is inside it - phase = 'open' - } - }, - }, - }) - checks.push({ - selector: `${container} ${fromMarker}`, - handler: { - element(element: RewriterElement) { - if (directStart) { - directStart = false - unsupported ||= element.removed - } else if (phase === 'scanning') { unsupported = true + return } + state.depth++ + state.phase = fromMarker ? 'scanning' : 'open' + // Closed by the container's own end, so what other components append lands outside it + element.prepend(fromMarker ? keep : cut, html) + element.append(close, html) + wrapped++ }, }, }) - } - - // After the wrapper's own handlers, so they see its state for the same element - for (const selector of inserting) { - handlers.push({ - selector, + cleanups.push({ + selector: container, handler: { element() { - if (atContainer || phase === 'open' || (phase === 'inStart' && directStart)) { - unsupported = true - } + atContainer = 0 }, }, }) - } - return { - componentKey, - handlers, - checks, - unwrap: { + if (toMarker) { + markers.push( + { + selector: combineSelectors(container, ' > ', toMarker), + handler: { + element() { + state.directEnd = state.depth === 1 + }, + }, + }, + { + selector: combineSelectors(container, ' ', toMarker), + handler: { + element(element: RewriterElement) { + const direct = state.directEnd + state.directEnd = false + // Before the start marker, or inside it + if (state.phase !== 'open') { + return + } + if (!direct || atContainer) { + unsupported = true + return + } + element.before(close + keep, html) + state.phase = 'closed' + }, + }, + }, + ) + } + if (fromMarker) { + markers.push( + { + selector: combineSelectors(container, ' > ', fromMarker), + handler: { + element() { + state.directStart = state.depth === 1 + }, + }, + }, + { + selector: combineSelectors(container, ' ', fromMarker), + handler: { + element(element: RewriterElement) { + const direct = state.directStart + state.directStart = false + if (state.phase !== 'scanning') { + return + } + if (!direct || atContainer) { + unsupported = true + return + } + element.after(close + cut, html) + state.phase = 'inStart' + try { + if (!element.onEndTag || isVoidElement(element)) { + state.phase = 'open' + } else { + onEnd(element, () => { + state.phase = 'open' + }) + } + } catch { + state.phase = 'open' + } + }, + }, + }, + ) + } + + const replacement = (replaceRange.replaceWith ?? []).map((item) => renderElement(ctx, item)) + unwrap.push({ selector: tag, handler: { element(element: RewriterElement) { @@ -240,7 +291,14 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi element.remove() }, }, - }, + }) + }) + + return { + componentKeys: ranges.map(({ componentKey }) => componentKey), + // Every container handler, then every marker handler, so a marker sees whether it is also a container + handlers: [...handlers, ...markers, ...cleanups], + unwrap, wrapped: () => wrapped, unsupported: () => unsupported, fail: () => { diff --git a/test/fakes/EndTagSpyRewriter.ts b/test/fakes/EndTagSpyRewriter.ts index 3831f229..629f4c3a 100644 --- a/test/fakes/EndTagSpyRewriter.ts +++ b/test/fakes/EndTagSpyRewriter.ts @@ -7,11 +7,12 @@ import type { } from '../../src/adapters/HtmlRewriterAdapter' import { LolHtmlRewriter } from './LolHtmlRewriter' -/** Wraps the real rewriter and records the tag of every element `onEndTag` is called on. */ +/** Wraps the real rewriter and records the tag of every element `onEndTag` is called on, and how many passes ran. */ export class EndTagSpyRewriter implements HtmlRewriterAdapter { readonly inner = new LolHtmlRewriter() readonly capabilities: HtmlRewriterCapabilities = this.inner.capabilities readonly endTagCalls: string[] = [] + sessions = 0 // Methods are bound to the element, which a real rewriter element needs private spyOn(element: RewriterElement): RewriterElement { @@ -31,6 +32,7 @@ export class EndTagSpyRewriter implements HtmlRewriterAdapter { } create(): HtmlRewriterSession { + this.sessions++ const session = this.inner.create() const spy: HtmlRewriterSession = { on: (selector: string, handlers: ElementHandlers) => { diff --git a/test/stages/wrappedRange.spec.ts b/test/stages/wrappedRange.spec.ts index c9e2e88b..06b1e84f 100644 --- a/test/stages/wrappedRange.spec.ts +++ b/test/stages/wrappedRange.spec.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest' import { normalizeMOSConfig } from '../../src/config' import type { PipelineContext } from '../../src/context' import handleSurfaceComponents, { applyComponents } from '../../src/stages/surfaceComponents' +import { combineSelectors, planRangeWrapper, splitSelectorList } from '../../src/stages/wrappedRange' import type { SurfaceDecisionResponse, WebElement } from '../../src/types' import { EndTagSpyRewriter } from '../fakes/EndTagSpyRewriter' @@ -19,12 +20,19 @@ const ctx: PipelineContext = { type Range = { fromMarker?: string; toMarker?: string; replaceWith?: WebElement[] } type Others = Record }> -type Options = { others?: Others; othersFirst?: boolean } +type Options = { others?: Others; othersFirst?: boolean; own?: Record } const html = (content: string) => ({ type: 'html' as const, content }) -const decision = (selector: string, range: Range, { others = {}, othersFirst = false }: Options = {}): SurfaceDecisionResponse => { +const decision = ( + selector: string, + range: Range, + { others = {}, othersFirst = false, own = {} }: Options = {}, +): SurfaceDecisionResponse => { const rangeBehavior = { - range: { metadata: { cssSelector: selector }, content: { replaceRange: { replaceWith: [html('NEW')], ...range } } }, + range: { + metadata: { cssSelector: selector }, + content: { ...own, replaceRange: { replaceWith: [html('NEW')], ...range } }, + }, } const otherBehaviors = Object.fromEntries( Object.entries(others).map(([key, { selector: cssSelector, content }]) => [key, { metadata: { cssSelector }, content }]), @@ -40,16 +48,45 @@ const decision = (selector: string, range: Range, { others = {}, othersFirst = f } } -const run = async (body: string, selector: string, range: Range, options?: Options, elementByElement = false) => { +const transform = async (body: string, decisions: SurfaceDecisionResponse, elementByElement = false) => { const rewriter = new EndTagSpyRewriter() const response = new Response(body, { headers: { 'Content-Type': 'text/html; charset=utf-8' } }) - const decisions = decision(selector, range, options) const result = elementByElement ? await applyComponents(ctx, response, decisions, rewriter, null) : await handleSurfaceComponents(ctx, response, decisions, rewriter) - return { body: await result.text(), endTagCalls: rewriter.endTagCalls } + const path = elementByElement || !planRangeWrapper(ctx, decisions) ? 'elements' : rewriter.sessions > 2 ? 'fallback' : 'wrapper' + return { body: await result.text(), endTagCalls: rewriter.endTagCalls, path } } +/** + * What the wrapper is expected to give, cut element by element: each range applied on its own ahead of the other + * components, so what they insert inside a cut goes with it and a marker they remove still marks the cut. `excluded` + * are containers the wrapper leaves alone. + */ +const expectedByElements = async (body: string, decisions: SurfaceDecisionResponse, excluded = '') => { + const rest: SurfaceDecisionResponse['componentBehaviors'] = {} + let current = body + for (const [key, behavior] of Object.entries(decisions.componentBehaviors)) { + const { replaceRange, ...content } = behavior.content ?? {} + if (!replaceRange) { + rest[key] = behavior + continue + } + const cssSelector = splitSelectorList(behavior.metadata.cssSelector as string) + .map((part) => part + excluded) + .join(', ') + const range = { ...decisions, componentBehaviors: { [key]: { metadata: { cssSelector }, content: { replaceRange } } } } + current = (await transform(current, range, true)).body + if (Object.keys(content).length) { + rest[key] = { ...behavior, content } + } + } + return (await transform(current, { ...decisions, componentBehaviors: rest }, true)).body +} + +const run = (body: string, selector: string, range: Range, options?: Options, elementByElement = false) => + transform(body, decision(selector, range, options), elementByElement) + /** Runs the page through the wrapper and element by element, and expects the same output from both. */ const bothWays = async (body: string, selector: string, range: Range, options?: Options) => { const wrapped = await run(body, selector, range, options) @@ -58,12 +95,31 @@ const bothWays = async (body: string, selector: string, range: Range, options?: return { ...wrapped, elementByElement } } +/** Runs the page through the wrapper alone, and expects it not to fall back. */ +const wrapped = async (body: string, selector: string, range: Range, options?: Options) => { + const result = await run(body, selector, range, options) + expect(result.path).toBe('wrapper') + return result.body +} + const page = '

t

a

b

c

d

f
' +describe('selector lists', () => { + it('splits on top-level commas only', () => { + expect(splitSelectorList('.a, div[title="x,y"] , :not(.b, .c)')).toEqual(['.a', 'div[title="x,y"]', ':not(.b, .c)']) + expect(splitSelectorList("[data-x='a\\'s,'], .b")).toEqual(["[data-x='a\\'s,']", '.b']) + }) + + it('combines every part of both sides', () => { + expect(combineSelectors('.a, .b', ' > ', 'p, hr')).toBe('.a > p, .a > hr, .b > p, .b > hr') + }) +}) + describe('range replacement by wrapper', () => { it('cuts from the start marker to the end of the container with one callback', async () => { - const { body, endTagCalls, elementByElement } = await bothWays(page, '.c', { fromMarker: 'p.s' }) + const { body, endTagCalls, elementByElement, path } = await bothWays(page, '.c', { fromMarker: 'p.s' }) expect(body).toBe('

t

a

NEW
f
') + expect(path).toBe('wrapper') expect(endTagCalls.sort()).toEqual(['div', 'p']) expect(elementByElement.endTagCalls.length).toBeGreaterThan(endTagCalls.length) }) @@ -138,6 +194,15 @@ describe('range replacement by wrapper', () => { expect(endTagCalls.sort()).toEqual(['div', 'div']) }) + it('ignores an end marker nested in the start marker', async () => { + const { body, path } = await bothWays('

b

', '.c', { + fromMarker: '.s', + toMarker: '.e', + }) + expect(body).toBe('
NEW
') + expect(path).toBe('wrapper') + }) + it('leaves a container with no end tag alone', async () => { const { body, endTagCalls } = await bothWays('

a

b

', 'img.c', {}) expect(body).toBe('

a

b

') @@ -159,10 +224,12 @@ describe('range replacement by wrapper', () => { } it('keep what they insert around the container and the markers', async () => { - const { body } = await bothWays(page, '.c', { fromMarker: 'p.s', toMarker: 'hr.e' }, { others: around }) - expect(body).toBe( - '
P

t

B

a

FNEWB
F

d

A
f
', - ) + for (const othersFirst of [false, true]) { + const body = await wrapped(page, '.c', { fromMarker: 'p.s', toMarker: 'hr.e' }, { others: around, othersFirst }) + expect(body).toBe( + '
P

t

B

a

FNEWB
F

d

A
f
', + ) + } }) it('keep what they append to a container cut to its end', async () => { @@ -170,94 +237,190 @@ describe('range replacement by wrapper', () => { expect(body).toBe('
P

t

a

NEWA
f
') }) + it('put what they prepend to a container cut from its top before the replacement', async () => { + for (const othersFirst of [false, true]) { + const others = { container: around.container as never } + const body = await wrapped(page, '.c', { toMarker: 'hr.e' }, { others, othersFirst }) + expect(body).toBe('
PNEW

d

A
f
') + } + }) + it('still apply to the content that stays', async () => { const others = { heading: { selector: 'h1', content: { append: [html('x')] } } } const { body } = await bothWays(page, '.c', { fromMarker: 'p.s' }, { others }) expect(body).toBe('

tx

a

NEW
f
') }) - }) - - describe('falls back to cutting element by element', () => { - const expectsFallback = async (body: string, selector: string, range: Range, options?: Options) => { - const { endTagCalls, elementByElement } = await bothWays(body, selector, range, options) - // The abandoned wrapper pass may have registered callbacks before the element path starts over - expect(endTagCalls.slice(-elementByElement.endTagCalls.length)).toEqual(elementByElement.endTagCalls) - } - it('when the start marker is not a direct child of the container', async () => { - await expectsFallback('

a

b

c

', '.c', { fromMarker: 'p.s' }) + it('lose what they insert inside the cut, whatever their order', async () => { + const others: Others = { + inside: { selector: 'section', content: { before: [html('O')], prepend: [html('O')] } }, + nested: { selector: 'section p', content: { after: [html('O')] } }, + } + for (const othersFirst of [false, true]) { + const body = await wrapped(page, '.c', { fromMarker: 'p.s', toMarker: 'hr.e' }, { others, othersFirst }) + expect(body).toBe('

t

a

NEW

d

f
') + } }) - it('when the end marker is not a direct child of the container', async () => { - await expectsFallback('

a

b

c

', '.c', { - fromMarker: 'p.s', - toMarker: 'hr.e', - }) + it('do not move the cut when they remove a marker', async () => { + for (const othersFirst of [false, true]) { + const others = { + start: { selector: 'p.s', content: { remove: true } }, + end: { selector: 'hr.e', content: { remove: true } }, + } + const body = await wrapped(page, '.c', { fromMarker: 'p.s', toMarker: 'hr.e' }, { others, othersFirst }) + expect(body).toBe('

t

NEW

d

f
') + } }) + }) - it('when containers are nested', async () => { - await expectsFallback('

a

b

c

', '.c', { - fromMarker: 'p.s', - }) + describe('content on the range component', () => { + it('lands around the cut', async () => { + const own = { prepend: [html('P')], append: [html('A')], before: [html('B')], after: [html('F')] } + expect(await wrapped(page, '.c', { toMarker: 'hr.e' }, { own })).toBe( + 'B
PNEW

d

A
F
f
', + ) }) - it('when a component before the range removes a marker', async () => { - for (const marker of ['p.s', 'hr.e']) { - const options = { others: { gone: { selector: marker, content: { remove: true } } }, othersFirst: true } - await expectsFallback(page, '.c', { fromMarker: 'p.s', toMarker: 'hr.e' }, options) - } + it('removes the container with the range', async () => { + const own = { remove: true, after: [html('F')] } + expect(await wrapped(page, '.c', { fromMarker: 'p.s' }, { own })).toBe('F
f
') }) + }) - it('when the container holds raw text', async () => { + describe('leaves a container alone', () => { + it('when it holds raw text', async () => { for (const tag of ['textarea', 'title', 'script', 'style', 'noscript']) { - await expectsFallback(`<${tag} class="c">

a

b

z

`, '.c', {}) + const body = `<${tag} class="c">

a

b

z

` + expect(await wrapped(body, '.c', {})).toBe(body) } }) - it('when the container is in MathML', async () => { + it('when it is in MathML', async () => { for (const body of [ '

a

x', '

a

x
', + 'ax', ]) { - await expectsFallback(body, '.c', { fromMarker: '.s' }) + expect(await wrapped(body, '.c', { fromMarker: '.s' })).toBe(body) } + expect(await wrapped('x

z

', 'math > mi.c', {})).toBe( + 'x

z

', + ) }) - it('when a marker selector has a combinator', async () => { - await expectsFallback('

t

a

b

c

', '.c', { - fromMarker: 'section .s', - }) + it('but still cuts the containers after it', async () => { + const body = '

a

x

a

b
' + expect(await wrapped(body, '.c', { fromMarker: '.s' })).toBe( + '

a

x

a

NEW
', + ) + }) + }) + + describe('nested containers', () => { + it('cut only the outermost', async () => { + const body = '

a

b

c

d

' + const { body: out, path } = await bothWays(body, '.c', { fromMarker: 'p.s' }) + expect(out).toBe('

a

b

c

NEW
') + expect(path).toBe('wrapper') + }) + + it('cut only the outermost, holding a start marker in the math', async () => { + const body = '
x

b

' + const { body: out, path } = await bothWays(body, '.c', { fromMarker: '.s' }) + expect(out).toBe('
xNEW
') + expect(path).toBe('wrapper') + }) + }) + + it('cuts each container of a selector list', async () => { + const body = `

a

b

${page}` + const { body: out, path } = await bothWays(body, '.x, .c', { fromMarker: 'p.s', toMarker: 'hr.e' }) + expect(out).toBe( + '

a

NEW

t

a

NEW

d

f
', + ) + expect(path).toBe('wrapper') + }) + + it('starts the range at the first marker of a list', async () => { + const { body, path } = await bothWays(page, '.c', { fromMarker: 'section, p.s', toMarker: 'footer, hr' }) + expect(body).toBe('

t

a

NEW

d

f
') + expect(path).toBe('wrapper') + }) + + describe('several range components', () => { + const second = (selector: string, range: Range = {}): Others => ({ + second: { selector, content: { replaceRange: { replaceWith: [html('2')], ...range } } }, }) - it('when the container selector names the MathML element', async () => { - await expectsFallback('

a

x

z

', 'math > mi.c', { fromMarker: '.s' }) + it('cut each range', async () => { + const { body, path } = await bothWays(page, '.c', { fromMarker: 'p.s' }, { others: second('footer') }) + expect(body).toBe('

t

a

NEW
2
') + expect(path).toBe('wrapper') }) - it('when a component before the range inserts content', async () => { - for (const [selector, content] of [ - ['p.s', { after: [html('O')] }], - ['section', { before: [html('O')] }], + it('cut a range nested in another', async () => { + for (const [range, expected] of [ + [{ toMarker: 'hr.e' }, '
NEW

d

'], + [{ fromMarker: 'p.s' }, '

t

a

NEW
'], + [{ fromMarker: 'h1' }, '

t

NEW
'], ] as const) { - const options = { others: { other: { selector, content } }, othersFirst: true } - await expectsFallback(page, '.c', { fromMarker: 'p.s', toMarker: 'hr.e' }, options) + const { body, path } = await bothWays(page, '.c', range, { others: second('section') }) + expect(body).toBe(`${expected}
f
`) + expect(path).toBe('wrapper') } + const kept = await bothWays(page, '.c', { toMarker: 'section' }, { others: second('section', { fromMarker: 'p' }) }) + expect(kept.body).toBe( + '
NEW

c

2

d

f
', + ) + }) + }) + + describe('falls back to cutting element by element', () => { + const expectsFallback = async (body: string, selector: string, range: Range, options?: Options) => { + const { endTagCalls, elementByElement, path } = await bothWays(body, selector, range, options) + expect(path).toBe('fallback') + // The abandoned wrapper pass may have registered callbacks before the element path starts over + expect(endTagCalls.slice(-elementByElement.endTagCalls.length)).toEqual(elementByElement.endTagCalls) + } + + it('when the start marker is not a direct child of the container', async () => { + await expectsFallback('

a

b

c

', '.c', { fromMarker: 'p.s' }) + }) + + it('when the end marker is not a direct child of the container', async () => { + await expectsFallback('

a

b

c

', '.c', { + fromMarker: 'p.s', + toMarker: 'hr.e', + }) + }) + + it('when a marker is a direct child of a nested container', async () => { + await expectsFallback('

a

b

c

', '.c', { + fromMarker: 'p.s', + }) + await expectsFallback('

a

c

', '.c', { toMarker: 'hr.e' }) + }) + + it('when a nested container is a marker', async () => { + await expectsFallback('

a

b

c

', '.c', { fromMarker: '.s' }) + }) + + it('when an element is the container of two ranges', async () => { + const others = { second: { selector: 'div', content: { replaceRange: { fromMarker: 'h1' } } } } + await expectsFallback(page, '.c', { fromMarker: 'p.s' }, { others }) }) - it('when the container selector is a list', async () => { - await expectsFallback(`

x

${page}`, '.x, .c', { fromMarker: 'p.s', toMarker: 'hr.e' }) + it('when the container of one range is a marker of another', async () => { + const others = { second: { selector: 'body', content: { replaceRange: { fromMarker: '.c' } } } } + await expectsFallback(`${page}`, '.c', { toMarker: 'hr.e' }, { others }) }) }) - it('uses the element path when the decision has several range components', async () => { - const two = decision('.c', { fromMarker: 'p.s' }) - two.componentBehaviors.second = { - metadata: { cssSelector: 'footer' }, - content: { replaceRange: { replaceWith: [html('2')] } }, - } - const rewriter = new EndTagSpyRewriter() - const result = await handleSurfaceComponents(ctx, new Response(page, { headers: { 'Content-Type': 'text/html' } }), two, rewriter) - expect(await result.text()).toBe('

t

a

NEW
2
') - expect(rewriter.endTagCalls.length).toBeGreaterThan(2) + it('cuts element by element when a marker selector has a combinator', async () => { + const body = '

t

a

b

c

' + const { path } = await bothWays(body, '.c', { fromMarker: 'section .s' }) + expect(path).toBe('elements') }) it('gives the same page as cutting element by element on random pages', async () => { @@ -279,32 +442,51 @@ describe('range replacement by wrapper', () => { } return out } + // No element any range selector matches, so the only container in MathML is the `mi` + const mathTree = () => `x1` + const insert = () => ({ [pick(['append', 'prepend', 'before', 'after'])]: [html('O')] }) + const counts = { wrapper: 0, fallback: 0, elements: 0 } for (let i = 0; i < 300; i++) { const container = pick(['div', 'div', 'div', 'textarea', 'mi']) - const containerHtml = `<${container} class="c">${tree(3)}` + const containerHtml = `<${container} class="c">${container === 'mi' ? mathTree() : tree(3)}` const body = `${tree(2)}${container === 'mi' ? `${containerHtml}` : containerHtml}${rnd() < 0.3 ? `
${tree(2)}
` : ''}${tree(1)}` const range = { - fromMarker: pick([undefined, '.s', 'p', 'h2', 'img']), + fromMarker: pick([undefined, '.s', 'p', 'h2', 'img', 'h2, .s']), toMarker: pick([undefined, undefined, '.e', 'hr', 'li']), } const otherSelector = pick(['.s', '.e', '.c', 'p', 'li']) - const options = pick([ + const others: Others = pick([ {}, - { others: { other: { selector: otherSelector, content: { remove: true } } }, othersFirst: rnd() < 0.5 }, - { - others: { - other: { - selector: otherSelector, - content: { [pick(['append', 'prepend', 'before', 'after'])]: [html('O')] }, + { other: { selector: otherSelector, content: { remove: true } } }, + { other: { selector: otherSelector, content: insert() } }, + ]) + if (rnd() < 0.3) { + others.second = { + selector: pick(['section', 'ul', 'li', 'span.x', 'h2']), + content: { + replaceRange: { + fromMarker: pick([undefined, '.s', 'p']), + toMarker: pick([undefined, '.e']), + replaceWith: [html('2')], }, }, - othersFirst: rnd() < 0.5, - }, - ]) - const selector = pick(['.c', 'div', 'article', 'body']) - const wrapped = await run(body, selector, range, options) - const elementByElement = await run(body, selector, range, options, true) - expect(wrapped.body, JSON.stringify({ body, selector, range, options })).toBe(elementByElement.body) + } + } + const options: Options = { + others, + othersFirst: rnd() < 0.5, + own: rnd() < 0.2 ? (rnd() < 0.2 ? { remove: true } : insert()) : {}, + } + const selector = pick(['.c', 'div', 'article', 'body', '.c, article', 'article, div', 'section, .c']) + const decisions = decision(selector, range, options) + const result = await transform(body, decisions) + counts[result.path as keyof typeof counts]++ + const expected = + result.path === 'fallback' + ? (await transform(body, decisions, true)).body + : await expectedByElements(body, decisions, ':not(textarea):not(mi)') + expect(result.body, JSON.stringify({ body, selector, range, options, path: result.path })).toBe(expected) } + expect(counts.wrapper).toBeGreaterThan(counts.fallback) }) }) From 3574c3b2bb784988e49c4cf97f2320b976148b91 Mon Sep 17 00:00:00 2001 From: Ben Kennedy Date: Thu, 5 Nov 2026 13:22:11 +0000 Subject: [PATCH 2/6] Cut a range with one callback when its start marker is nested --- .changeset/range-wrapper-more-shapes.md | 2 +- src/stages/surfaceComponents.ts | 7 +- src/stages/wrappedRange.ts | 218 ++++++++++++++++++++---- test/fakes/EndTagSpyRewriter.ts | 4 +- test/stages/wrappedRange.spec.ts | 190 ++++++++++++++++++--- 5 files changed, 363 insertions(+), 58 deletions(-) diff --git a/.changeset/range-wrapper-more-shapes.md b/.changeset/range-wrapper-more-shapes.md index 7a8a0658..b54aa969 100644 --- a/.changeset/range-wrapper-more-shapes.md +++ b/.changeset/range-wrapper-more-shapes.md @@ -2,4 +2,4 @@ "@monetizationos/proxy": minor --- -More pages use the one-callback range cut. Content other components insert inside the range is removed with it, whatever the component order, and a marker removed by another component still marks where the range starts or ends. A container holding raw text, such as `textarea` or `script`, is left alone, and only the outermost of nested containers is cut. Selector lists, several range components in one decision, and other content on the range component all use the one-callback path. Selector lists also apply per part when cutting element by element. +More pages use the one-callback range cut. Content other components insert inside the range is removed with it, whatever the component order, and a marker removed by another component still marks where the range starts or ends. A container holding raw text, such as `textarea` or `script`, is left alone, and only the outermost of nested containers is cut. Selector lists, several range components in one decision, and other content on the range component all use the one-callback path. Selector lists also apply per part when cutting element by element. A start marker nested inside its container now uses the one-callback path too, when its range is the only one in the decision. diff --git a/src/stages/surfaceComponents.ts b/src/stages/surfaceComponents.ts index 7848d7e7..cfdad5e9 100644 --- a/src/stages/surfaceComponents.ts +++ b/src/stages/surfaceComponents.ts @@ -27,8 +27,11 @@ export default async function handleSurfaceComponents( headers.delete('content-length') const init = { status: response.status, statusText: response.statusText, headers } const wrapped = await applyComponents(ctx, new Response(page, init), surfaceDecisions, rewriter, wrapper) - const wrappedPage = await wrapped.arrayBuffer() - if (wrapper.unsupported()) { + let wrappedPage: Uint8Array | null = new Uint8Array(await wrapped.arrayBuffer()) + if (!wrapper.unsupported() && wrapper.nested() > 0) { + wrappedPage = await wrapper.splice(wrappedPage, rewriter, headers) + } + if (wrapper.unsupported() || !wrappedPage) { return applyComponents(ctx, new Response(page, init), surfaceDecisions, rewriter, null) } if (wrapper.wrapped() === 0) { diff --git a/src/stages/wrappedRange.ts b/src/stages/wrappedRange.ts index 688d92f0..8451aaad 100644 --- a/src/stages/wrappedRange.ts +++ b/src/stages/wrappedRange.ts @@ -1,4 +1,4 @@ -import type { ElementHandlers, RewriterElement } from '../adapters/HtmlRewriterAdapter' +import type { ElementHandlers, HtmlRewriterAdapter, RewriterElement } from '../adapters/HtmlRewriterAdapter' import type { HtmlStageContext } from '../context' import type { SurfaceDecisionResponse } from '../types' import { renderElement } from './elements' @@ -56,6 +56,10 @@ export type RangeWrapper = { /** Pass 1 met a page the wrapper can't cut, so the caller must redo it element by element. */ unsupported: () => boolean fail: () => void + /** Start markers pass 1 found nested in their container, each cut by `splice` before pass 2. */ + nested: () => number + /** Cuts the ranges of nested start markers out of pass 1's output, or null when the page must be redone element by element. */ + splice: (page: Uint8Array, rewriter: HtmlRewriterAdapter, headers: Headers) => Promise | null> } type RangeState = { @@ -65,14 +69,25 @@ type RangeState = { // Set by a `>` handler for the check on the same element, which runs after it directStart: boolean directEnd: boolean + // The start marker was nested, so the range is cut by `splice` + nested: boolean + // The container isn't HTML, so a fragment of it can't be parsed on its own + foreign: boolean } +// Starts every sentinel; pages hardly ever hold it +const MARK = 1 +const COMMENT_OPEN = 4 +const COMMENT_CLOSE = 3 + /** * Cuts each range by splitting its container into wrapper elements in one pass and removing the one that holds the * range in the next, so the cut costs one callback instead of one per element. The split only balances when both - * markers are direct children of the outermost container; a nested marker makes the page unsupported. What other - * components insert inside the cut is removed with it, and a marker removed by another component still marks the cut. - * A container holding raw text, or inside `math`, is left alone. + * markers are direct children of the outermost container. A nested start marker of the only range instead gets + * sentinels around it and its replacement, and `splice` cuts the bytes from there to the end marker or the container's + * end, putting back the end tags of the elements the start marker sits in. Any other nested marker makes the page + * unsupported. What other components insert inside the cut is removed with it, and a marker removed by another + * component still marks the cut. A container holding raw text, or inside `math`, is left alone. */ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisionResponse): RangeWrapper | null => { const ranges = Object.entries(decisions.componentBehaviors).flatMap(([componentKey, behavior]) => { @@ -101,41 +116,47 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi let unsupported = false let wrapped = 0 + let nested = 0 + const sentinel = (index: number, kind: 'o' | 'c' | 's' | 'r' | 'e') => `${String.fromCharCode(MARK)}${nonce}-${index}-${kind}` + const comment = (index: number, kind: 's' | 'r' | 'e') => `` // Ranges whose container handler has run for the current element, cleared once its marker handlers have run let atContainer = 0 - // The rewriter mismatches the wrapper's end tags inside MathML - let mathDepth = 0 + // Open `math` and `svg` elements. The rewriter mismatches the wrapper's end tags inside MathML, and `splice` reads + // the kept prefix as HTML, which misreads SVG's self-closing tags. + const foreign = { math: 0, svg: 0 } + const foreignTag = (element: RewriterElement) => { + const tag = element.tagName.toLowerCase() + return tag === 'math' || tag === 'svg' ? tag : null + } - // Only the last end tag callback registered on an element runs, so one registered on `math` keeps its count + // Only the last end tag callback registered on an element runs, so one registered on `math` or `svg` keeps its count const onEnd = (element: RewriterElement, callback: () => void) => { - const math = element.tagName.toLowerCase() === 'math' + const tag = foreignTag(element) element.onEndTag?.( - math + tag ? () => { - mathDepth-- + foreign[tag]-- callback() } : callback, ) } - const handlers: Handler[] = [ - { - selector: 'math', - handler: { - element(element: RewriterElement) { - mathDepth++ - try { - element.onEndTag?.(() => { - mathDepth-- - }) - } catch { - mathDepth-- - } - }, + const handlers: Handler[] = (['math', 'svg'] as const).map((tag) => ({ + selector: tag, + handler: { + element(element: RewriterElement) { + foreign[tag]++ + try { + element.onEndTag?.(() => { + foreign[tag]-- + }) + } catch { + foreign[tag]-- + } }, }, - ] + })) const markers: Handler[] = [] const cleanups: Handler[] = [] const unwrap: Handler[] = [] @@ -146,7 +167,8 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi const cut = `<${tag}>` const keep = `<${tag} data-keep>` const close = `` - const state: RangeState = { depth: 0, phase: 'closed', directStart: false, directEnd: false } + const replacement = (replaceRange.replaceWith ?? []).map((item) => renderElement(ctx, item)) + const state: RangeState = { depth: 0, phase: 'closed', directStart: false, directEnd: false, nested: false, foreign: false } handlers.push({ selector: container, @@ -173,7 +195,7 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi } return } - if (RAW_TEXT_ELEMENTS.has(element.tagName.toLowerCase()) || mathDepth > 0) { + if (RAW_TEXT_ELEMENTS.has(element.tagName.toLowerCase()) || foreign.math > 0) { return } try { @@ -187,9 +209,12 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi } state.depth++ state.phase = fromMarker ? 'scanning' : 'open' - // Closed by the container's own end, so what other components append lands outside it - element.prepend(fromMarker ? keep : cut, html) - element.append(close, html) + state.nested = false + state.foreign = foreign.svg > 0 + // Closed by the container's own end, so what other components append lands outside it. Sentinels + // in the tags let `splice` find the container; pass 2 removes the tags with them. + element.prepend(fromMarker ? `<${tag} data-keep ${sentinel(index, 'o')}>` : cut, html) + element.append(fromMarker ? `` : close, html) wrapped++ }, }, @@ -227,7 +252,7 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi unsupported = true return } - element.before(close + keep, html) + element.before(state.nested ? comment(index, 'e') : close + keep, html) state.phase = 'closed' }, }, @@ -253,11 +278,23 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi if (state.phase !== 'scanning') { return } - if (!direct || atContainer) { + // Another range's wrapper tags would come apart in the splice + if (atContainer || (!direct && (ranges.length > 1 || state.foreign || foreign.math > 0))) { unsupported = true return } - element.after(close + cut, html) + if (direct) { + element.after(close + cut, html) + } else { + // Later `after` content lands closer to the element + element.before(comment(index, 's'), html) + element.after(comment(index, 'r'), html) + for (const [content, options] of replacement.slice().reverse()) { + element.after(content, options) + } + state.nested = true + nested++ + } state.phase = 'inStart' try { if (!element.onEndTag || isVoidElement(element)) { @@ -276,7 +313,6 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi ) } - const replacement = (replaceRange.replaceWith ?? []).map((item) => renderElement(ctx, item)) unwrap.push({ selector: tag, handler: { @@ -304,5 +340,119 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi fail: () => { unsupported = true }, + nested: () => nested, + splice: async (page, rewriter, headers) => { + // Only taken with one range, so every sentinel is that range's + const needle = new TextEncoder().encode(sentinel(0, 'o').slice(0, -1)) + const find = searcher(needle) + const out: Uint8Array[] = [] + let copyFrom = 0 + let contentStart = -1 + let closers: Uint8Array | null = null + let cutting = false + let cuts = 0 + for (let at = find(page, 0); at !== -1; at = find(page, at + 1)) { + const kind = String.fromCharCode(page[at + needle.length] ?? 0) + const end = at + needle.length + 1 + if (cutting) { + if (kind === 'e') { + copyFrom = end + COMMENT_CLOSE + } else if (kind === 'c') { + copyFrom = at - ` length + chunk.length, 0)) + let offset = 0 + for (const chunk of out) { + spliced.set(chunk, offset) + offset += chunk.length + } + return spliced + }, + } +} + +/** Finds `needle` in a page, skipping ahead by up to its length on a mismatch rather than reading every byte. */ +const searcher = (needle: Uint8Array) => { + const last = needle.length - 1 + const skip = new Uint8Array(256).fill(needle.length) + for (let i = 0; i < last; i++) { + skip[needle[i] ?? 0] = last - i + } + return (page: Uint8Array, from: number) => { + for (let at = from; at + last < page.length; at += skip[page[at + last] ?? 0] ?? 1) { + let i = last + while (i >= 0 && page[at + i] === needle[i]) { + i-- + } + if (i < 0) { + return at + } + } + return -1 + } +} + +/** End tags for the elements still open at the end of `prefix`, innermost first; null if they can't be told. */ +const closingTags = async (rewriter: HtmlRewriterAdapter, prefix: Uint8Array, headers: Headers) => { + const open: { name: string }[] = [] + let failed = false + const session = rewriter.create().on('*', { + element(element: RewriterElement) { + if (isVoidElement(element)) { + return + } + if (!element.onEndTag) { + failed = true + return + } + const entry = { name: element.tagName } + try { + // An end tag also closes the elements left open inside it + element.onEndTag(() => { + const index = open.lastIndexOf(entry) + if (index !== -1) { + open.length = index + } + }) + open.push(entry) + } catch { + // Self-closing, so never open + } + }, + }) + await session.transform(new Response(prefix, { headers })).arrayBuffer() + const names = open.map(({ name }) => name).reverse() + // Written as ASCII, whatever the page's encoding + if (failed || !names.every((name) => /^[\x21-\x7e]+$/.test(name))) { + return null } + return new TextEncoder().encode(names.map((name) => ``).join('')) } diff --git a/test/fakes/EndTagSpyRewriter.ts b/test/fakes/EndTagSpyRewriter.ts index 629f4c3a..f3df697f 100644 --- a/test/fakes/EndTagSpyRewriter.ts +++ b/test/fakes/EndTagSpyRewriter.ts @@ -7,12 +7,13 @@ import type { } from '../../src/adapters/HtmlRewriterAdapter' import { LolHtmlRewriter } from './LolHtmlRewriter' -/** Wraps the real rewriter and records the tag of every element `onEndTag` is called on, and how many passes ran. */ +/** Wraps the real rewriter and records the tag of every element `onEndTag` is called on, how many passes ran, and their selectors. */ export class EndTagSpyRewriter implements HtmlRewriterAdapter { readonly inner = new LolHtmlRewriter() readonly capabilities: HtmlRewriterCapabilities = this.inner.capabilities readonly endTagCalls: string[] = [] sessions = 0 + readonly selectors: string[] = [] // Methods are bound to the element, which a real rewriter element needs private spyOn(element: RewriterElement): RewriterElement { @@ -36,6 +37,7 @@ export class EndTagSpyRewriter implements HtmlRewriterAdapter { const session = this.inner.create() const spy: HtmlRewriterSession = { on: (selector: string, handlers: ElementHandlers) => { + this.selectors.push(selector) session.on(selector, { ...handlers, element: handlers.element ? (element) => handlers.element?.(this.spyOn(element)) : undefined, diff --git a/test/stages/wrappedRange.spec.ts b/test/stages/wrappedRange.spec.ts index 06b1e84f..fd15e8fe 100644 --- a/test/stages/wrappedRange.spec.ts +++ b/test/stages/wrappedRange.spec.ts @@ -54,8 +54,12 @@ const transform = async (body: string, decisions: SurfaceDecisionResponse, eleme const result = elementByElement ? await applyComponents(ctx, response, decisions, rewriter, null) : await handleSurfaceComponents(ctx, response, decisions, rewriter) - const path = elementByElement || !planRangeWrapper(ctx, decisions) ? 'elements' : rewriter.sessions > 2 ? 'fallback' : 'wrapper' - return { body: await result.text(), endTagCalls: rewriter.endTagCalls, path } + // Only the element path selects every element in a container + const fellBack = rewriter.selectors.some((selector) => selector.endsWith(' *')) + const path = elementByElement || !planRangeWrapper(ctx, decisions) ? 'elements' : fellBack ? 'fallback' : 'wrapper' + // A pass that selects every element finds the open elements around a nested start marker + const spliced = path === 'wrapper' && rewriter.selectors.includes('*') + return { body: await result.text(), endTagCalls: rewriter.endTagCalls, path, spliced } } /** @@ -103,6 +107,7 @@ const wrapped = async (body: string, selector: string, range: Range, options?: O } const page = '

t

a

b

c

d

f
' +const nested = '

t

a

b

c

d

f
' describe('selector lists', () => { it('splits on top-level commas only', () => { @@ -384,10 +389,6 @@ describe('range replacement by wrapper', () => { expect(endTagCalls.slice(-elementByElement.endTagCalls.length)).toEqual(elementByElement.endTagCalls) } - it('when the start marker is not a direct child of the container', async () => { - await expectsFallback('

a

b

c

', '.c', { fromMarker: 'p.s' }) - }) - it('when the end marker is not a direct child of the container', async () => { await expectsFallback('

a

b

c

', '.c', { fromMarker: 'p.s', @@ -395,13 +396,28 @@ describe('range replacement by wrapper', () => { }) }) - it('when a marker is a direct child of a nested container', async () => { - await expectsFallback('

a

b

c

', '.c', { - fromMarker: 'p.s', - }) + it('when the end marker is a direct child of a nested container', async () => { await expectsFallback('

a

c

', '.c', { toMarker: 'hr.e' }) }) + it('when the start marker is nested and the decision has another range', async () => { + const others = { second: { selector: 'footer', content: { replaceRange: {} } } } + await expectsFallback(nested, '.c', { fromMarker: 'p.s' }, { others }) + }) + + it('when the start marker is nested in MathML', async () => { + await expectsFallback('
x1

b

', '.c', { fromMarker: '.s' }) + }) + + it('when the start marker is nested in an SVG container', async () => { + await expectsFallback('', '.c', { fromMarker: '.s' }) + }) + + it('when another component removes what holds a nested start marker', async () => { + const others = { section: { selector: 'section', content: { remove: true } } } + await expectsFallback(nested, '.c', { fromMarker: 'p.s' }, { others }) + }) + it('when a nested container is a marker', async () => { await expectsFallback('

a

b

c

', '.c', { fromMarker: '.s' }) }) @@ -417,6 +433,106 @@ describe('range replacement by wrapper', () => { }) }) + describe('nested start marker', () => { + const spliced = async (body: string, selector: string, range: Range, options?: Options) => { + const result = await bothWays(body, selector, range, options) + expect(result.path).toBe('wrapper') + expect(result.spliced).toBe(true) + return result + } + + it('cuts to the end of the container, keeping the end tags of what holds the marker', async () => { + const { body, endTagCalls, elementByElement } = await spliced(nested, '.c', { fromMarker: 'p.s' }) + expect(body).toBe('

t

a

NEW
f
') + expect(elementByElement.endTagCalls.length).toBeGreaterThan(endTagCalls.length) + }) + + it('cuts up to an end marker that is a direct child', async () => { + const body = nested.replace('

d

', '

x

d

') + const result = await spliced(body, '.c', { fromMarker: 'p.s', toMarker: 'hr.e' }) + expect(result.body).toBe( + '

t

a

NEW

d

f
', + ) + }) + + it('cuts after a void or self-closing start marker', async () => { + const { body } = await spliced('

b

c

', '.c', { fromMarker: '.s' }) + expect(body).toBe('
NEW
') + const svg = await spliced('

b

', '.c', { + fromMarker: '.s', + }) + expect(svg.body).toBe('
NEW
') + }) + + it('cuts each container', async () => { + const { body } = await spliced(`${nested}${nested}`, '.c', { fromMarker: 'p.s' }) + const once = '

t

a

NEW
f
' + expect(body).toBe(once + once) + }) + + it('cuts a page mixing nested and direct start markers', async () => { + const { body } = await spliced(`${page}${nested}`, '.c', { fromMarker: 'p.s' }) + expect(body).toBe( + '

t

a

NEW
f

t

a

NEW
f
', + ) + }) + + it('cuts in a nested container holding the start marker', async () => { + const body = '

a

b

c

d

' + const result = await spliced(body, '.c', { fromMarker: 'p.s' }) + expect(result.body).toBe('

a

b

NEW
') + }) + + it('closes an ancestor whose end tag the page left out', async () => { + const body = '

a x y

z

after

' + const { body: out, path } = await run(body, '.c', { fromMarker: '.s' }) + expect(out).toBe('

a xNEW

after

') + expect(path).toBe('wrapper') + // Removing the `p` left open also takes the end tag that closes it + expect((await run(body, '.c', { fromMarker: '.s' }, {}, true)).body).toBe( + '

a xNEW

after

', + ) + }) + + describe('other components', () => { + const around: Others = { + container: { selector: '.c', content: { prepend: [html('P')], append: [html('A')] } }, + start: { selector: 'p.s', content: { before: [html('B')], after: [html('F')] } }, + holder: { + selector: 'section', + content: { + before: [html('B')], + prepend: [html('P')], + append: [html('A')], + after: [html('F')], + }, + }, + } + + it('keep what they insert before the cut, and lose what they insert inside it', async () => { + for (const othersFirst of [false, true]) { + const body = await wrapped(nested, '.c', { fromMarker: 'p.s' }, { others: around, othersFirst }) + expect(body).toBe( + '
P

t

B
P
B

a

FNEW
A
f
', + ) + } + }) + + it('do not move the cut when they remove a marker', async () => { + const body = nested.replace('

d

', '

d

') + for (const othersFirst of [false, true]) { + const others = { + start: { selector: 'p.s', content: { remove: true } }, + end: { selector: 'hr.e', content: { remove: true } }, + } + expect(await wrapped(body, '.c', { fromMarker: 'p.s', toMarker: 'hr.e' }, { others, othersFirst })).toBe( + '

t

NEW

d

f
', + ) + } + }) + }) + }) + it('cuts element by element when a marker selector has a combinator', async () => { const body = '

t

a

b

c

' const { path } = await bothWays(body, '.c', { fromMarker: 'section .s' }) @@ -427,17 +543,26 @@ describe('range replacement by wrapper', () => { let seed = 7 const rnd = () => (seed = (seed * 1664525 + 1013904223) >>> 0) / 2 ** 32 const pick = (items: readonly T[]): T => items[Math.floor(rnd() * items.length)] as T - const tree = (depth: number): string => { + let unclosed = false + // Left open, so the end tag of what holds it closes it. Never a start marker: one left open never gets what + // is inserted after it. + let openTags = ['p', 'li'] + const tree = (depth: number, parent = ''): string => { let out = '' for (let i = 0, n = 1 + Math.floor(rnd() * 5); i < n; i++) { const r = rnd() - if (r < 0.2) out += pick(['text ', 'a b ', '& ']) + const open = openTags.length ? pick(openTags) : parent + // In one of its own kind, the parent's end tag would close it and leave the parent open + if (i === n - 1 && depth > 0 && open !== parent && rnd() < 0.1) { + unclosed = true + out += `<${open}${rnd() < 0.4 ? ` class="${pick(['e', 'x'])}"` : ''}>${tree(depth - 1, open)}` + } else if (r < 0.2) out += pick(['text ', 'a b ', '& ']) else if (r < 0.35) out += `<${pick(['img', 'br', 'hr', 'input'])}${rnd() < 0.4 ? ` class="${pick(['s', 'e', 'x'])}"` : ''}>` else if (r < 0.4) out += `` else if (depth > 0) { const tag = pick(['div', 'p', 'span', 'section', 'ul', 'li', 'a', 'h2']) const cls = pick(['s', 'e', 'x', 'c', '']) - out += `<${tag}${cls ? ` class="${cls}"` : ''}>${tree(depth - 1)}` + out += `<${tag}${cls ? ` class="${cls}"` : ''}>${tree(depth - 1, tag)}` } else out += 'leaf ' } return out @@ -445,11 +570,20 @@ describe('range replacement by wrapper', () => { // No element any range selector matches, so the only container in MathML is the `mi` const mathTree = () => `x1` const insert = () => ({ [pick(['append', 'prepend', 'before', 'after'])]: [html('O')] }) - const counts = { wrapper: 0, fallback: 0, elements: 0 } + /** + * A nested start marker's cut also takes what other components insert at the end of what holds it, and closes + * what holds it as the page reads with their inserts: one in `svg` takes the `path` after it out of SVG, where + * `` stays open. Where the page left an element open in the cut, the element path removing it also takes + * the end tag that closes it, so what is inserted at that end tag moves; the cut closes what was left open. + */ + const afterReplacement = /(NEW<\/i>)((?:<\/[a-z0-9]+>|O<\/u>)*)/g + const normalized = (body: string, spliced: boolean) => + (unclosed ? body.replaceAll('O', '') : body).replace(afterReplacement, (_, replacement: string, run: string) => { + const kept = spliced ? run.replaceAll('O', '').replaceAll('', '') : run + return replacement + (unclosed ? kept.replace(/<\/[a-z0-9]+>/g, '') : kept) + }) + const counts = { wrapper: 0, spliced: 0, fallback: 0, elements: 0 } for (let i = 0; i < 300; i++) { - const container = pick(['div', 'div', 'div', 'textarea', 'mi']) - const containerHtml = `<${container} class="c">${container === 'mi' ? mathTree() : tree(3)}` - const body = `${tree(2)}${container === 'mi' ? `${containerHtml}` : containerHtml}${rnd() < 0.3 ? `
${tree(2)}
` : ''}${tree(1)}` const range = { fromMarker: pick([undefined, '.s', 'p', 'h2', 'img', 'h2, .s']), toMarker: pick([undefined, undefined, '.e', 'hr', 'li']), @@ -479,14 +613,30 @@ describe('range replacement by wrapper', () => { } const selector = pick(['.c', 'div', 'article', 'body', '.c, article', 'article, div', 'section, .c']) const decisions = decision(selector, range, options) + // Removing an element left open also removes what follows it up to the end tag that closes it + const removes = !!others.second || JSON.stringify(options).includes('"remove"') + unclosed = false + openTags = removes ? [] : range.fromMarker === 'p' ? ['li'] : ['p', 'li'] + const container = pick(['div', 'div', 'div', 'textarea', 'mi']) + const containerHtml = `<${container} class="c">${container === 'mi' ? mathTree() : tree(3)}` + const extraTag = pick(['article', 'div']) + const extra = rnd() < 0.3 ? `<${extraTag} class="c">${tree(2)}` : '' + const body = `${tree(2)}${container === 'mi' ? `${containerHtml}` : containerHtml}${extra}${tree(1)}` + const result = await transform(body, decisions) - counts[result.path as keyof typeof counts]++ + counts[result.spliced ? 'spliced' : (result.path as keyof typeof counts)]++ const expected = result.path === 'fallback' ? (await transform(body, decisions, true)).body : await expectedByElements(body, decisions, ':not(textarea):not(mi)') - expect(result.body, JSON.stringify({ body, selector, range, options, path: result.path })).toBe(expected) + const context = JSON.stringify({ body, selector, range, options, path: result.path, spliced: result.spliced }) + if (result.path === 'wrapper' && (result.spliced || unclosed)) { + expect(normalized(result.body, result.spliced), context).toBe(normalized(expected, result.spliced)) + } else { + expect(result.body, context).toBe(expected) + } } - expect(counts.wrapper).toBeGreaterThan(counts.fallback) + expect(counts.wrapper + counts.spliced).toBeGreaterThan(counts.fallback) + expect(counts.spliced).toBeGreaterThan(0) }) }) From da0e6424b3d4c0b027f66aae72d1acb51fe3efa4 Mon Sep 17 00:00:00 2001 From: Ben Kennedy Date: Thu, 5 Nov 2026 14:33:38 +0000 Subject: [PATCH 3/6] Reset container depth when the outermost container ends --- src/stages/wrappedRange.ts | 3 ++- test/stages/wrappedRange.spec.ts | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/stages/wrappedRange.ts b/src/stages/wrappedRange.ts index 8451aaad..7f329be2 100644 --- a/src/stages/wrappedRange.ts +++ b/src/stages/wrappedRange.ts @@ -201,7 +201,8 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi try { onEnd(element, () => { state.phase = 'closed' - state.depth-- + // Not a decrement: an inner container left open never runs its own callback + state.depth = 0 }) } catch { unsupported = true diff --git a/test/stages/wrappedRange.spec.ts b/test/stages/wrappedRange.spec.ts index fd15e8fe..e77aa02d 100644 --- a/test/stages/wrappedRange.spec.ts +++ b/test/stages/wrappedRange.spec.ts @@ -336,6 +336,11 @@ describe('range replacement by wrapper', () => { expect(out).toBe('
xNEW
') expect(path).toBe('wrapper') }) + + it('still cut a later container after an inner one left without its end tag', async () => { + const body = '
  • x
  • y
' + expect(await wrapped(body, '.c', {})).toBe('
    NEW
    NEW
') + }) }) it('cuts each container of a selector list', async () => { From 61d5e86a0755a52d3657c250f3329663de2f408a Mon Sep 17 00:00:00 2001 From: Ben Kennedy Date: Thu, 5 Nov 2026 14:57:43 +0000 Subject: [PATCH 4/6] Find the splice sentinels with a native search, from both ends of the page --- src/stages/wrappedRange.ts | 94 +++++++++++++++++++++++++++----------- 1 file changed, 68 insertions(+), 26 deletions(-) diff --git a/src/stages/wrappedRange.ts b/src/stages/wrappedRange.ts index 7f329be2..bf6dd150 100644 --- a/src/stages/wrappedRange.ts +++ b/src/stages/wrappedRange.ts @@ -345,21 +345,47 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi splice: async (page, rewriter, headers) => { // Only taken with one range, so every sentinel is that range's const needle = new TextEncoder().encode(sentinel(0, 'o').slice(0, -1)) - const find = searcher(needle) + const { forward, backward } = finders(page, needle) + const kindAt = (at: number) => String.fromCharCode(page[at + needle.length] ?? 0) + const end = (at: number) => at + needle.length + 1 + const closeTag = ` r ? end(before) + COMMENT_CLOSE : -1 + const closers = resume === -1 ? null : await closingTags(rewriter, page.subarray(end(o) + 1, s - COMMENT_OPEN), headers) + if (!closers) { + return null + } + return concat([ + page.subarray(0, s - COMMENT_OPEN), + page.subarray(end(s) + COMMENT_CLOSE, r - COMMENT_OPEN), + closers, + page.subarray(resume), + ]) + } + const find = (from: number) => forward(from) const out: Uint8Array[] = [] let copyFrom = 0 let contentStart = -1 let closers: Uint8Array | null = null let cutting = false let cuts = 0 - for (let at = find(page, 0); at !== -1; at = find(page, at + 1)) { - const kind = String.fromCharCode(page[at + needle.length] ?? 0) + for (let at = find(0); at !== -1; at = find(at + 1)) { + const kind = kindAt(at) const end = at + needle.length + 1 if (cutting) { if (kind === 'e') { copyFrom = end + COMMENT_CLOSE } else if (kind === 'c') { - copyFrom = at - ` length + chunk.length, 0)) - let offset = 0 - for (const chunk of out) { - spliced.set(chunk, offset) - offset += chunk.length - } - return spliced + return concat(out) }, } } -/** Finds `needle` in a page, skipping ahead by up to its length on a mismatch rather than reading every byte. */ -const searcher = (needle: Uint8Array) => { - const last = needle.length - 1 - const skip = new Uint8Array(256).fill(needle.length) - for (let i = 0; i < last; i++) { - skip[needle[i] ?? 0] = last - i +const concat = (chunks: Uint8Array[]) => { + const joined = new Uint8Array(chunks.reduce((length, chunk) => length + chunk.length, 0)) + let offset = 0 + for (const chunk of chunks) { + joined.set(chunk, offset) + offset += chunk.length } - return (page: Uint8Array, from: number) => { - for (let at = from; at + last < page.length; at += skip[page[at + last] ?? 0] ?? 1) { - let i = last - while (i >= 0 && page[at + i] === needle[i]) { - i-- - } - if (i < 0) { - return at + return joined +} + +/** + * Finds `needle`, which starts with the rare `MARK` byte, from either end of a page. The native search for that byte + * keeps the cost low and steady; a byte loop in JavaScript was as fast only while the engine kept it optimised. + */ +const finders = (page: Uint8Array, needle: Uint8Array) => { + const matches = (at: number) => { + for (let i = 1; i < needle.length; i++) { + if (page[at + i] !== needle[i]) { + return false } } - return -1 + return true + } + return { + forward: (from: number) => { + for (let at = page.indexOf(MARK, from); at !== -1; at = page.indexOf(MARK, at + 1)) { + if (matches(at)) { + return at + } + } + return -1 + }, + backward: (from: number) => { + for (let at = from < 0 ? -1 : page.lastIndexOf(MARK, from); at !== -1; at = at === 0 ? -1 : page.lastIndexOf(MARK, at - 1)) { + if (matches(at)) { + return at + } + } + return -1 + }, } } From d3e9ac742ff506ace519188003b47f2db9f763dd Mon Sep 17 00:00:00 2001 From: Ben Kennedy Date: Fri, 6 Nov 2026 10:50:43 +0000 Subject: [PATCH 5/6] Skip void start markers before registering end tags, and say why each catch fires --- src/stages/wrappedRange.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/stages/wrappedRange.ts b/src/stages/wrappedRange.ts index bf6dd150..2a4e2aff 100644 --- a/src/stages/wrappedRange.ts +++ b/src/stages/wrappedRange.ts @@ -152,6 +152,7 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi foreign[tag]-- }) } catch { + // Self-closing, so nothing is inside it foreign[tag]-- } }, @@ -205,6 +206,7 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi state.depth = 0 }) } catch { + // Self-closing foreign element (``), so there is no end tag to close the cut at unsupported = true return } @@ -297,15 +299,16 @@ export const planRangeWrapper = (ctx: HtmlStageContext, decisions: SurfaceDecisi nested++ } state.phase = 'inStart' + if (!element.onEndTag || isVoidElement(element)) { + state.phase = 'open' + return + } try { - if (!element.onEndTag || isVoidElement(element)) { + onEnd(element, () => { state.phase = 'open' - } else { - onEnd(element, () => { - state.phase = 'open' - }) - } + }) } catch { + // Self-closing foreign element, so nothing is inside it state.phase = 'open' } }, From 31c289d7ad3bbf08424ddea1f18f86133474c448 Mon Sep 17 00:00:00 2001 From: Ben Kennedy Date: Fri, 6 Nov 2026 14:01:43 +0000 Subject: [PATCH 6/6] Keep selector lists with an empty part invalid, and test lists spaced any way --- src/stages/wrappedRange.ts | 13 +++++++++---- test/stages/wrappedRange.spec.ts | 28 ++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/stages/wrappedRange.ts b/src/stages/wrappedRange.ts index 2a4e2aff..31520add 100644 --- a/src/stages/wrappedRange.ts +++ b/src/stages/wrappedRange.ts @@ -41,10 +41,15 @@ export const splitSelectorList = (selector: string): string[] => { } /** `${container}${combinator}${inner}` for every pair of parts, so a list on either side keeps the combinator. */ -export const combineSelectors = (container: string, combinator: ' ' | ' > ', inner: string) => - splitSelectorList(container) - .flatMap((outer) => splitSelectorList(inner).map((part) => `${outer}${combinator}${part}`)) - .join(', ') +export const combineSelectors = (container: string, combinator: ' ' | ' > ', inner: string) => { + const outers = splitSelectorList(container) + const inners = splitSelectorList(inner) + // An empty part (`.a,,.b`, `.a,`) is invalid; kept as is so the rewriter still rejects it + if (outers.includes('') || inners.includes('')) { + return `${container}${combinator}${inner}` + } + return outers.flatMap((outer) => inners.map((part) => `${outer}${combinator}${part}`)).join(', ') +} export type RangeWrapper = { componentKeys: string[] diff --git a/test/stages/wrappedRange.spec.ts b/test/stages/wrappedRange.spec.ts index e77aa02d..83460ec9 100644 --- a/test/stages/wrappedRange.spec.ts +++ b/test/stages/wrappedRange.spec.ts @@ -115,6 +115,16 @@ describe('selector lists', () => { expect(splitSelectorList("[data-x='a\\'s,'], .b")).toEqual(["[data-x='a\\'s,']", '.b']) }) + it('splits whatever the whitespace around the commas', () => { + expect(splitSelectorList('.a,.b')).toEqual(['.a', '.b']) + expect(splitSelectorList(' .a ,\n\t.b ')).toEqual(['.a', '.b']) + }) + + it('keeps a list with an empty part invalid', () => { + expect(combineSelectors('.a,', ' ', 'p')).toBe('.a, p') + expect(combineSelectors('.a', ' > ', 'p,,hr')).toBe('.a > p,,hr') + }) + it('combines every part of both sides', () => { expect(combineSelectors('.a, .b', ' > ', 'p, hr')).toBe('.a > p, .a > hr, .b > p, .b > hr') }) @@ -352,6 +362,24 @@ describe('range replacement by wrapper', () => { expect(path).toBe('wrapper') }) + it.each(['.x,.c', '.x , .c'])('cuts each container of the selector list %j', async (selector) => { + const body = `

a

b

${page}` + const { body: out, path } = await bothWays(body, selector, { fromMarker: 'p.x,p.s', toMarker: 'hr.e , hr.x' }) + expect(out).toBe( + '

a

NEW

t

a

NEW

d

f
', + ) + expect(path).toBe('wrapper') + }) + + it.each([ + ['.x,,.c', 'p.s'], + ['.c,', 'p.s'], + ['.c', 'p.s,'], + ])('leaves the page alone when a selector list has an empty part (%j, %j)', async (selector, fromMarker) => { + const { body } = await bothWays(page, selector, { fromMarker }) + expect(body).toBe(page) + }) + it('starts the range at the first marker of a list', async () => { const { body, path } = await bothWays(page, '.c', { fromMarker: 'section, p.s', toMarker: 'footer, hr' }) expect(body).toBe('

t

a

NEW

d

')