Decoration range incorrectly expands to run boundaries; highlight can be lost after mark changes#2173
Conversation
VladaHarbour
requested review from
caio-pizzol,
harbournick and
mattConnHarbour
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b257e52bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
good approach - splitting runs at decoration boundaries feels like the right approach.
please check inline comments especially collectDecorationRanges one
|
@VladaHarbour mind addressing Caio's comments first? thank you! |
Hi @harbournick! Added my fixes |
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@VladaHarbour all 6 comments from round 1 are addressed, nice work on the shared helper and the split-runs tests. left two inline comments: one real bug in the !restoreEmptyDecorations branch that causes redundant repaints (easy one-liner), and one question about test coverage for the same-position guard.
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@VladaHarbour all feedback from rounds 1 and 2 is addressed. added behavior tests (Playwright) covering the core SD-1963 scenarios — comment highlights and track-change decorations surviving bold/italic/underline application. lgtm, approving.
…ack replacement and mark split
Playwright tests covering SD-1963: comment highlights and track-change decorations must persist when the user applies bold/italic/underline to overlapping or partially-overlapping text.
caio-pizzol
force-pushed
the
vlada/sd-1963
branch
from
c18f701 to
4ee0d5a
Compare
When italic is applied to part of a commented range, the run splits and the highlight spans multiple DOM elements. Assert by commentId and individual text fragments instead of the full contiguous text.
|
🎉 This PR is included in superdoc v1.17.0-next.37 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.2.0-next.32 The release is available on GitHub release |
… be lost after mark changes (superdoc-dev#2173) * fix: separate logic for decoration sync in presentation editor * fix: move from overlay approach to paragraph splitting * fix: clear selection * fix: prevent highlighted decoration position shift * fix: remove console.log * fix: decoration restore condition * fix(super-editor): compose decoration remap so external highlights track replacement and mark split * fix: logic improvements * fix: create shared helper for repeated code * fix: update tests and address comments * test: add behavior tests for decoration survival after mark changes Playwright tests covering SD-1963: comment highlights and track-change decorations must persist when the user applies bold/italic/underline to overlapping or partially-overlapping text. * fix(test): check comment highlight by id after run split When italic is applied to part of a commented range, the run splits and the highlight spans multiple DOM elements. Assert by commentId and individual text fragments instead of the full contiguous text. --------- Co-authored-by: Matthew Connelly <matthew@harbourshare.com> Co-authored-by: Nick Bernal <nick@superdoc.dev> Co-authored-by: Caio Pizzol <caio@harbourshare.com>
|
🎉 This PR is included in superdoc v1.17.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.2.0 The release is available on GitHub release |
5 participants
No description provided.