Fix/unset link clear transient hyperlink styleid#3190
Fix/unset link clear transient hyperlink styleid#3190christos8333 wants to merge 2 commits intosuperdoc-dev:mainfrom
Conversation
![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: 374cf73ce7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| const linkAttrs = { text: finalText, rId }; | ||
| const linkAttrs = { text: finalText, rId, hadUnderline }; |
There was a problem hiding this comment.
When a selection mixes underlined and plain text, this stores one hadUnderline value on the entire new link mark as soon as any text in the range was underlined. In that scenario (for example, underline only the first word, select the whole phrase, set a link, then unset it), unsetLink treats the whole link as pre-underlined and skips removing underline everywhere, leaving link-added underline on text that was originally plain. Preserve this state per link segment or remove underline only from segments that did not have it before linking.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@christos8333 valid - i reproduced this through the toolbar flow. flagged it in the review at line 237 with a fix sketch.
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
hey @christos8333! thanks for the PR. the main fix (clearing the Hyperlink styleId on unlink) looks right, and the tests for it are solid.
three issues with the new hadUnderline flag, all reachable through the toolbar - left them inline. they share one fix: figure out the value at unlink time, not link time.
one more edge case worth a regression test (imported link carrying its underline as an inline mark loses underline on unlink): [attach fixture].
happy to pair or pick up the fix.
|
|
||
| let hadUnderline = false; | ||
| if (underlineMarkType) { | ||
| state.doc.nodesBetween(from, to, (node) => { |
There was a problem hiding this comment.
repro:
- type "hi", select all
- click the link icon
- change "Text" in the dialog to something longer than "hi"
- paste a URL, click Apply
the editor crashes: TypeError: Cannot read properties of undefined (reading 'nodeSize').
fix: do the hadUnderline scan before line 234 reassigns to, or scan tr.doc after the new text is inserted.
| to = from + finalText.length; | ||
| } | ||
|
|
||
| let hadUnderline = false; |
There was a problem hiding this comment.
repro:
- type "abcdef"
- underline only "abc"
- select all
- click the link icon, paste a URL, Apply
- click the link, click Remove
result: "abcd" stays underlined. "d" was plain to start with, and there's no easy way to clear it.
the flag is one true/false for the whole selection, so any partial underline turns it on. fix: track it per character, or drop the underline-removal step and let the styleId cleanup handle the visual.
| } | ||
|
|
||
| const linkAttrs = { text: finalText, rId }; | ||
| const linkAttrs = { text: finalText, rId, hadUnderline }; |
There was a problem hiding this comment.
repro:
- type "hi", select all, link to
https://first.example.com, Apply - click the link, change URL to
https://second.example.com, Apply - click the link, Remove
result: "hi" is still underlined, even though it was plain to start with.
editing a link re-runs setLink, which scans and sees the underline the first link added. the flag flips to true and unlink leaves it alone.
2 participants
Unlink now fully removes transient hyperlink styling instead of only removing the link mark.
What this fixes
Removing a link could leave formatting “stuck” (underline/color and underline tool was shown as active in toolbar).
This happened because hyperlink style metadata (styleId: Hyperlink / FollowedHyperlink) could remain on text/run state after unsetLink().
What changed in link.js
During unsetLink(), we now clear hyperlink styleId only when it is one of the transient link style IDs.
We clear it in both places where link-derived style may persist:
textStyle mark attrs (mark.attrs.styleId)
run node attrs (runProperties.styleId)
Why this is safe
We only target transient hyperlink style IDs, so non-link character styles (e.g. Emphasis, custom styles) are preserved.
Other unrelated formatting is not broadly removed by this cleanup.
Tests
Added regression tests for unsetLink() to lock in the transient hyperlink cleanup behavior.
The tests verify that when unlinking text, transient hyperlink styleId values (Hyperlink, FollowedHyperlink) are cleared in both state locations where they can persist: textStyle.attrs.styleId and runProperties.styleId.
They also verify underline behavior is correct in both directions: pre-existing underline is preserved, while link-added underline is removed.
Together, these tests prevent regressions where link formatting appears “stuck” after unlink while ensuring non-link formatting behavior remains intact.
Closes #3189