Skip to content

fix: prevent signifier double-wrap and selector overlap (#3) - #7

Open
SmileyChris wants to merge 1 commit into
frankolson:masterfrom
SmileyChris:fix/signifier-wrap-and-selectors
Open

SmileyChris wants to merge 1 commit into
frankolson:masterfrom
SmileyChris:fix/signifier-wrap-and-selectors

Conversation

@SmileyChris

Copy link
Copy Markdown

Summary

Fixes two bugs in the reading-mode post-processor reported in #3:

  • Double-wrapelement.findAll('ul > li') and element.findAll('.task-list-item') both match the same <li> for tasks, so the concatenated list iterated task items twice and wrapped their signifiers in nested <span class=\"bujo-bullet-signifier\"><span class=\"bujo-bullet-signifier\">?</span></span> elements. Query ul > li once and derive the checkbox subset via .filter.
  • Mid-text replacementbullet.innerHTML.replace(signifierText, ...) replaced the first occurrence anywhere in the HTML, not only at the start of the item's visible text. A line like "is this ? okay" had its ? wrapped as if it were a leading signifier. Replace with a text-node DOM split using createTreeWalker + createTextNode + insertBefore, scoped to the first meaningful text node of each list item.

textContent assignment on the new span makes HTML injection impossible, so the isomorphic-dompurify runtime dependency is dropped.

Supersedes #4 — thanks @davidstosik for the original diagnosis of the selector overlap. This PR takes a slightly wider approach (also fixes the mid-text innerHTML.replace bug and drops the sanitiser dep).

Single-file change in src/index.ts (+ package.json / package-lock.json for the dropped dep).

Test plan

  • npm run build succeeds.
  • In a test vault with default signifiers (!, ?):
    • - ! priority task renders with a single red ! in reading mode.
    • - [ ] ? follow up renders with a single red ?, no nested span.
    • - is this ? okay shows no red ? (regression case).
    • Existing checkbox types (- [x], - [-], - [>], - [<], - [o]) still render with their icons.
    • Right-click menu on a rendered checkbox still offers "Change to: ...".

`element.findAll('ul > li')` and `element.findAll('.task-list-item')`
both match the same `<li>` for tasks, so the concatenated list
iterated task items twice and wrapped their signifiers in nested
`<span class="bujo-bullet-signifier">` elements. Query `ul > li`
once and derive the checkbox subset via `.filter`.

`bullet.innerHTML.replace(signifierText, ...)` also replaced the
first occurrence anywhere in the HTML, not only at the start of
the item's user-visible text, so lines like "is this ? okay" got
their `?` wrapped. Replace with a text-node DOM split using
`createTreeWalker` + `createTextNode` + `insertBefore`, scoped to
the first meaningful text node of each list item.

`textContent` assignment on the new span makes HTML injection
impossible, so the `isomorphic-dompurify` runtime dependency is
dropped.

Fixes frankolson#3. Supersedes frankolson#4.
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