fix(client): anchor annotation selector so highlighting works on nested inline elements#3
Closed
auspham wants to merge 1 commit into
Closed
fix(client): anchor annotation selector so highlighting works on nested inline elements#3auspham wants to merge 1 commit into
auspham wants to merge 1 commit into
Conversation
…ed inline elements cssPath emitted an unrooted selector (e.g. "div:nth-of-type(1) > ul > li:nth-of-type(2) > strong"). document.querySelector matches such a path against any subtree at any depth, so when an earlier element shared the same tag/nth-of-type skeleton (e.g. a <strong> nested in a card grid) it resolved to that decoy instead of the intended element. resolveRange's text guard then failed, pendingRects returned empty, and openSelMenu called clearPending - closing the annotation menu and wiping the selection. The result was that some lines (a <strong> inside a list, a link, any id-less nested inline element) silently could not be highlighted or commented on. Root the path at an ancestor id, otherwise <body>, and climb the full ancestor chain instead of capping at six segments, so it always resolves absolutely to the correct element. Adds a browser regression test whose fixture reproduces the colliding skeleton; it fails (menu display:none) before the fix and passes (flex) after. Signed-off-by: Austin (Ngoc Thang) Pham <austinpham@microsoft.com>
Owner
Author
|
Folded into #2 to keep everything in a single PR (the selector-anchor commit was cherry-picked onto that branch). Closing this one. |
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
1 participant
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
Some lines in a review could not be highlighted or commented on at all: selecting them just flashed and cleared, with no annotation menu. This happened for any id-less nested inline element (a
<strong>inside a list item, a link, etc.) when an earlier part of the document shared the same tag/nth-of-typeskeleton.The cause was in
cssPath(client/util.js). It built an unrooted selector for a selection's anchor element, for examplediv:nth-of-type(1) > ul > li:nth-of-type(2) > strong.document.querySelectormatches an unrooted path against any subtree at any depth, so when an earlier subtree matched the same skeleton (e.g. a<strong>nested in a card grid) the path re-resolved to that decoy element instead of the intended one.resolveRange's text guard then correctly detected the text mismatch and returned null,pendingRectscame back empty, andopenSelMenucalledclearPending- which closed the menu and wiped the selection. The net effect was a silent "can't highlight this line".The fix roots the selector at an ancestor
idwhen there is one, otherwise at<body>, and climbs the full ancestor chain instead of capping at six segments, so the path always resolves absolutely to the correct element. Lines that used to resolve correctly are unaffected; only the previously-wrong resolutions are corrected.Type of change
How was this tested?
Added a browser regression test (
tests/client/selector-anchor.test.mjs+fixtures/selector-collision.html) whose fixture reproduces the colliding skeleton: a decoy<strong>nested in a card grid earlier in document order, then the real target<strong>in a list. Selecting the target asserts the annotation menu opens. It fails before the fix (menudisplay:none) and passes after (flex). Also verified end-to-end against the real artifact that surfaced this, where the target line now opens the menu.python -m pytest tests/python(45 passed)ruff check skills/htmlit tests/python(clean)npm test(browser client - 20 passed, including the new regression test)Checklist
git commit -s).