Merge pull request #935 from d-zero-dev/fix/main-content-selector-priority

fix(beholder,anatomist): resolve main-content selectors by priority, not DOM order

Author: YusukeHirao

packages/@d-zero/anatomist/src/capture-layout-tree.spec.ts

@@ -60,6 +60,58 @@ describe('captureLayoutTree', () => {
 		expect(result.root?.children[0]?.tagName).toBe('P');
 	});
 
+	it('prefers a higher-priority selector even when it matches later in DOM order', () => {
+		const doc = createDocument(
+			'<body><div class="main" data-rect="0,0,1,1">Wrong</div>' +
+				'<main id="page" data-rect="0,0,800,600">Right</main></body>',
+		);
+
+		const result = captureLayoutTree(null, SELECTORS, FALLBACK_SELECTORS, 10, doc);
+
+		expect(result.mainSelector).toBe('main#page');
+	});
+
+	it('prefers a nested higher-priority match over its lower-priority ancestor wrapper', () => {
+		const doc = createDocument(
+			'<body><div class="main" data-rect="0,0,800,600">' +
+				'<p data-rect="0,0,100,20">breadcrumb</p>' +
+				'<main id="page" data-rect="0,20,400,580">Real main</main>' +
+				'<div id="aside" data-rect="400,20,400,580">Sidebar</div>' +
+				'</div></body>',
+		);
+
+		const result = captureLayoutTree(null, SELECTORS, FALLBACK_SELECTORS, 10, doc);
+
+		expect(result.mainSelector).toBe('main#page');
+	});
+
+	it('skips an invalid selector in the priority list and keeps trying the rest', () => {
+		const doc = createDocument(
+			'<body><main id="page" data-rect="0,0,800,600">Right</main></body>',
+		);
+
+		const result = captureLayoutTree(
+			null,
+			['[[[invalid', ...SELECTORS],
+			FALLBACK_SELECTORS,
+			10,
+			doc,
+		);
+
+		expect(result.mainSelector).toBe('main#page');
+	});
+
+	it('prefers the earlier-listed selector when two selectors both match different elements', () => {
+		const doc = createDocument(
+			'<body><div class="main" data-rect="0,0,1,1">Wrong</div>' +
+				'<div id="main" data-rect="0,0,800,600">Right</div></body>',
+		);
+
+		const result = captureLayoutTree(null, SELECTORS, FALLBACK_SELECTORS, 10, doc);
+
+		expect(result.mainSelector).toBe('div#main');
+	});
+
 	it('falls back to the fallback selector list when the priority list has no match', () => {
 		const doc = createDocument(
 			'<body><div id="primaryMain" data-rect="0,0,800,600">Fallback</div></body>',
@@ -70,6 +122,26 @@ describe('captureLayoutTree', () => {
 		expect(result.mainSelector).toBe('div#primaryMain');
 	});
 
+	it('still returns DOM-order-first match among fallback selectors (fallback list is not priority-ordered per element)', () => {
+		const doc = createDocument(
+			'<body><div id="wrapperMain" data-rect="0,0,800,600">' +
+				'<p data-rect="0,0,100,20">breadcrumb</p>' +
+				'<div id="innerMain" data-rect="0,20,800,580">Inner</div>' +
+				'</div></body>',
+		);
+
+		// Both #wrapperMain and #innerMain match the same fallback selector
+		// (`[id*="main" i]`), so — unlike the priority list above — there is
+		// no higher/lower priority to arbitrate between them. `querySelector`
+		// returns whichever matches first in document order, which is the
+		// outer wrapper here. This is accepted, existing behavior: the
+		// priority-list fix only orders *between* selectors in the array,
+		// not among multiple elements matching the *same* selector.
+		const result = captureLayoutTree(null, SELECTORS, FALLBACK_SELECTORS, 10, doc);
+
+		expect(result.mainSelector).toBe('div#wrapperMain');
+	});
+
 	it('tries the explicit mainContentSelector before the priority list', () => {
 		const doc = createDocument(
 			'<body><main data-rect="0,0,1,1">Wrong</main><section id="custom" data-rect="0,0,800,600">Right</section></body>',

packages/@d-zero/anatomist/src/capture-layout-tree.ts

@@ -30,6 +30,20 @@
  * can match a different element than the one actually found, so this
  * module resolves the element itself directly instead of round-tripping
  * through that string.
+ *
+ * WHY the priority-list resolution logic in `resolveMainElement` is
+ * duplicated rather than shared: same closure-free constraint as above.
+ * `extractMainContentsFromDocument` (`@d-zero/beholder/get-main-contents.ts`)
+ * independently re-implements the identical "try each selector in array
+ * order, first match wins" strategy for the same reason. The two copies
+ * have drifted out of sync before (one queried the array with a single
+ * `querySelector(selectors.join(','))` — which resolves by DOM document
+ * order, not array priority — while the other still tried selectors one at
+ * a time); `capture-layout-tree.spec.ts` and `get-main-contents.spec.ts`
+ * both carry matching regression cases (searchable by the test name
+ * "prefers a higher-priority selector...") specifically so that fixing one
+ * side's resolution logic without the other shows up as a spec gap, not a
+ * silent divergence.
  * @module
  */
 
@@ -117,12 +131,27 @@ export function captureLayoutTree(
 			}
 		}
 
+		// Tried one at a time, in priority order, rather than joined into a
+		// single `querySelector(selectors.join(','))` call: a CSS group
+		// selector matches whichever selector is first in *document order*,
+		// not whichever is first in this array — an ancestor wrapper matching
+		// a low-priority selector (e.g. `#contents`) would win over a
+		// descendant matching a higher-priority one (e.g. `#main`) just
+		// because it appears earlier in the DOM. Querying one selector at a
+		// time makes this array's order the actual priority.
 		let element: Element | null = null;
-		try {
-			element = doc.querySelector(selectors.join(','));
-		} catch {
-			// The built-in selector list is a fixed, known-valid constant, so
-			// this should be unreachable — but fail closed rather than throw.
+		for (const sel of selectors) {
+			try {
+				element = doc.querySelector(sel);
+			} catch {
+				// The built-in selector list is a fixed, known-valid constant,
+				// so this should be unreachable — but fail closed rather than
+				// aborting the whole priority list over one bad entry.
+				continue;
+			}
+			if (element) {
+				break;
+			}
 		}
 
 		if (element) {

packages/@d-zero/beholder/src/get-main-contents.spec.ts

@@ -203,18 +203,63 @@ describe('extractMainContentsFromDocument', () => {
 		expect(result.wordCount).toBe(15);
 	});
 
-	it('returns first matching element in DOM order when multiple Phase-1 selectors match', () => {
+	it('prefers a higher-priority selector even when it matches later in DOM order', () => {
 		const result = extract(`
 			<html><body>
-				<main>Semantic main</main>
 				<div id="content">ID content</div>
+				<main>Semantic main</main>
 			</body></html>
 		`);
 
 		expect(result.main?.nodeName).toBe('MAIN');
 		expect(result.wordCount).toBe(12);
 	});
 
+	it('prefers a nested higher-priority match over its lower-priority ancestor wrapper', () => {
+		const result = extract(`
+			<html><body>
+				<div id="contents">
+					<p>breadcrumb</p>
+					<div id="main">Real main</div>
+					<div id="aside">Sidebar</div>
+				</div>
+			</body></html>
+		`);
+
+		expect(result.main?.id).toBe('main');
+	});
+
+	it('prefers the earlier-listed selector when two selectors both match different elements', () => {
+		const result = extract(`
+			<html><body>
+				<div class="main">Wrong</div>
+				<div id="main">Right</div>
+			</body></html>
+		`);
+
+		expect(result.main?.id).toBe('main');
+	});
+
+	it('still returns DOM-order-first match among fallback selectors (fallback list is not priority-ordered per element)', () => {
+		// Both #wrapperMain and #innerMain match the same fallback selector
+		// (`[id*="main" i]`), so — unlike the priority list above — there is
+		// no higher/lower priority to arbitrate between them. `querySelector`
+		// returns whichever matches first in document order, which is the
+		// outer wrapper here. This is accepted, existing behavior: the
+		// priority-list fix only orders *between* selectors in the array,
+		// not among multiple elements matching the *same* selector.
+		const result = extract(`
+			<html><body>
+				<div id="wrapperMain">
+					<p>breadcrumb</p>
+					<div id="innerMain">Inner</div>
+				</div>
+			</body></html>
+		`);
+
+		expect(result.main?.id).toBe('wrapperMain');
+	});
+
 	it('uses Phase-2 class*=main when Phase-1 finds nothing', () => {
 		const result = extract(
 			'<html><body><div class="page-main-area">Phase two</div></body></html>',

packages/@d-zero/beholder/src/get-main-contents.ts

@@ -7,6 +7,19 @@
  *
  * WHY no `@medv/finder`: selector strings are diagnostic only; a tag+id+class // cspell:disable-line
  * path is enough and avoids a Node-only dependency inside the page realm.
+ *
+ * WHY the priority-list resolution logic below is duplicated rather than
+ * shared: same closure-free constraint as above. `@d-zero/anatomist`'s
+ * `capture-layout-tree.ts` independently re-implements the identical "try
+ * each selector in array order, first match wins" strategy for the same
+ * reason. The two copies have drifted out of sync before (one queried the
+ * array with a single `querySelector(selectors.join(','))` — which resolves
+ * by DOM document order, not array priority — while the other still tried
+ * selectors one at a time); this file's spec and
+ * `anatomist/capture-layout-tree.spec.ts` both carry matching regression
+ * cases (searchable by the test name "prefers a higher-priority
+ * selector...") specifically so that fixing one side's resolution logic
+ * without the other shows up as a spec gap, not a silent divergence.
  * @module
  */
 
@@ -118,14 +131,27 @@ export function extractMainContentsFromDocument(
 		selectors.unshift(mainContentSelector);
 	}
 
+	// Tried one at a time, in priority order, rather than joined into a
+	// single `querySelector(selectors.join(','))` call: a CSS group selector
+	// matches whichever selector is first in *document order*, not whichever
+	// is first in this array — an ancestor wrapper matching a low-priority
+	// selector (e.g. `#contents`) would win over a descendant matching a
+	// higher-priority one (e.g. `#main`) just because it appears earlier in
+	// the DOM. Querying one selector at a time makes this array's order the
+	// actual priority.
 	let $main: Element | null = null;
-	try {
-		$main = doc.querySelector(selectors.join(','));
-	} catch {
-		// Invalid custom selector: retry without it so built-in selectors still run.
-		if (mainContentSelector) {
-			selectors.shift();
-			$main = doc.querySelector(selectors.join(','));
+	for (const sel of selectors) {
+		try {
+			$main = doc.querySelector(sel);
+		} catch {
+			// Invalid selector — only reachable for the caller-supplied
+			// mainContentSelector unshifted above, since the built-in list is
+			// a fixed, known-valid constant. Skip it and keep trying the
+			// remaining selectors in priority order.
+			continue;
+		}
+		if ($main) {
+			break;
 		}
 	}