fix(beholder,anatomist): resolve main-content selectors by priority, not DOM order - #935
Merged
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@d-zero/beholderand@d-zero/anatomist: both resolved a priority-ordered selector list via a singlequerySelector(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.@d-zero/anatomistagainst real crawled pages (sampled from nitpicker archives of public corporate sites): a real-world article page had its main-content resolved todiv#contents— which wraps both the article body (div#main) and the sidebar (div#aside) — instead ofdiv#mainalone, causing the layout classifier to see a 2-column region and misclassify it ascomplex-gridinstead of the article body's actualvertical-stack.fallbackSelectorsloop pattern in both files).page.evaluatecan'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 passyarn build— all 29 packages buildyarn lint— no new errors (pre-existing warnings only, unrelated to this change)@d-zero/anatomist's CLI against the page that surfaced the bug:mainSelectorchanged fromdiv#contents(misclassifiedcomplex-grid, confidence 0.6) todiv#main(correctly classifiedvertical-stack, confidence 0.8)