Skip to content

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

Merged
YusukeHirao merged 4 commits into
devfrom
fix/main-content-selector-priority
Jul 31, 2026
Merged

fix(beholder,anatomist): resolve main-content selectors by priority, not DOM order#935
YusukeHirao merged 4 commits into
devfrom
fix/main-content-selector-priority

Conversation

@YusukeHirao

Copy link
Copy Markdown
Member

Summary

  • Fix main-content resolution in @d-zero/beholder and @d-zero/anatomist: both resolved a priority-ordered selector list via a single querySelector(selectors.join(',')) call, which matches by DOM document order, not by the array's intended priority. An ancestor wrapper matching a lower-priority selector (e.g. #contents) could win over a descendant matching a higher-priority one (e.g. #main) just because it appeared earlier in the DOM.
  • Found while manually verifying @d-zero/anatomist against real crawled pages (sampled from nitpicker archives of public corporate sites): a real-world article page had its main-content resolved to div#contents — which wraps both the article body (div#main) and the sidebar (div#aside) — instead of div#main alone, causing the layout classifier to see a 2-column region and misclassify it as complex-grid instead of the article body's actual vertical-stack.
  • Both packages now query each selector individually, in array order, so the array's order is the actual priority (matching the existing fallbackSelectors loop pattern in both files).
  • Added regression tests to both packages' specs covering: priority winning over later DOM order, a nested higher-priority match beating its lower-priority ancestor wrapper (the exact article+sidebar shape above), an invalid selector being skipped without aborting the rest of the priority list, and documented the existing (unrelated, unchanged) DOM-order-first behavior among multiple elements matching the same selector.
  • Documented in both files' module JSDoc that this resolution logic is intentionally duplicated (Puppeteer's page.evaluate can't ship closures) and points at the matching spec cases in the other file, so a future fix to one side surfaces as a spec gap in the other instead of a silent divergence.

Test plan

  • yarn test — 142 test files / 1862 tests pass
  • yarn build — all 29 packages build
  • yarn lint — no new errors (pre-existing warnings only, unrelated to this change)
  • Manually re-ran @d-zero/anatomist's CLI against the page that surfaced the bug: mainSelector changed from div#contents (misclassified complex-grid, confidence 0.6) to div#main (correctly classified vertical-stack, confidence 0.8)

querySelector(selectors.join(',')) matches whichever selector is first
in document order, ignoring the array's intended priority. An ancestor
wrapper matching a low-priority selector (e.g. #contents) could win
over a descendant matching a higher-priority one (e.g. #main) just
because it appears earlier in the DOM.

Query each selector individually instead, so the array order is the
actual priority.
…rder

querySelector(selectors.join(',')) matches whichever selector is first
in document order, ignoring the array's intended priority. An ancestor
wrapper matching a low-priority selector (e.g. #contents) could win
over a descendant matching a higher-priority one (e.g. #main) just
because it appears earlier in the DOM.

Query each selector individually instead, so the array order is the
actual priority.
… note

QA follow-up on the selector-priority fix:
- Document the same-priority-different-selector and fallback-selector
  DOM-order-first behavior with explicit tests.
- Note in the module JSDoc that this resolution logic is intentionally
  duplicated in @d-zero/anatomist's capture-layout-tree.ts, and point at
  the matching spec cases in both files so a future fix to one side
  shows up as a spec gap in the other, not a silent divergence.
…c note

QA follow-up on the selector-priority fix:
- Cover the previously-unreachable-from-mainContentSelector catch path by
  injecting an invalid selector directly into the priority list.
- Document the same-priority-different-selector and fallback-selector
  DOM-order-first behavior with explicit tests.
- Note in the module JSDoc that this resolution logic is intentionally
  duplicated in @d-zero/beholder's get-main-contents.ts, and point at the
  matching spec cases in both files so a future fix to one side shows up
  as a spec gap in the other, not a silent divergence.
@YusukeHirao
YusukeHirao requested a review from yusasa16 as a code owner July 31, 2026 04:11
@YusukeHirao
YusukeHirao merged commit 768187d into dev Jul 31, 2026
6 checks passed
@YusukeHirao
YusukeHirao deleted the fix/main-content-selector-priority branch July 31, 2026 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant