Fix/word hyperlink format reset#3209
Conversation
…ted-link regression
![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: 528ee2bb8b
ℹ️ 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 hasUnderline = node.marks.some((mark) => mark.type === underlineMarkType); | ||
| if (hasUnderline) return; |
There was a problem hiding this comment.
When the selected text carries an underline mark whose attrs explicitly disable underlining (for example imported w:u val="none" or a cascade negation with underlineType: 'none'), this check treats it as an existing underline and skips adding the hyperlink underline. That leaves the new link with direct no-underline formatting, whereas the previous code removed any underline mark and applied a normal one, so links created over no-underline text can render/export as not underlined.
Useful? React with 👍 / 👎.
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@christos8333 thanks for the follow-up. the wider cleanup catches more imported Word docs (good), but it goes too far, and the test from PR 3190 fails under the new logic.
two bugs inline + one small thing. happy to pair or take it - whichever you'd prefer.
| .unsetMark('link', { extendEmptyMarkRange: true }) | ||
| .command(({ tr }) => { | ||
| if (underlineMarkType) { | ||
| tr.doc.nodesBetween(from, to, (node, pos) => { |
There was a problem hiding this comment.
unlink wipes formatting that wasn't part of the link.
repro 1: type "RED LINK PLAIN" → color "RED" red → link only "LINK" → select the whole word → Remove Link. "RED" loses its color.
repro 2: type some text → set its style to "Emphasis" → link it → unlink. the style comes back as null.
the check fires on any text in range with a color or a style ID, then clears all three (color, underline, style). fix: only run this on the hyperlinkRunPositions set you already compute, or only clear the Hyperlink/FollowedHyperlink style IDs the way the old code did.
There was a problem hiding this comment.
hello @caio-pizzol . Yes, you are right SuperDoc has a color tool. Will be available later today if you would like and have time to take a look together. I think the issue here as far as i can remember is the clearedAttrs which unconditionally removes color attr on al ink and there was an unsetColor which was part of the original code(unsure if that is needed, will have to take a look). Do you guys have something like a chat channel i could contact you directly?
Will take a look also to the other comments later today!
There was a problem hiding this comment.
hello @caio-pizzol . I think i am missing something. I checked the issue with the color and , when i apply color + link from within the editor yeah you are right i can do the previous check or even if i have an attrs.color i guess.
The problem i have is from the docx import.
So that's a sample i create with 2 links. One is a docx link the other is again a docx link and then i applied a color. How in one instance i will remove both the color and the link and on the other just the link and have the text as red to have the same experience as the link from within the editor?
| }); | ||
|
|
||
| const commandChain = chain(); | ||
| if (selection.empty && linkMarkType && from !== to) { |
There was a problem hiding this comment.
ci will fail. pnpm --filter @superdoc/super-editor test relationships shows "keeps imported inline underline mark when removing link" failing with expected 0 to be greater than 0.
the new branch removes plain underlines from imported hyperlinks, but PR 3190 added that test to lock in the opposite behavior. if you meant to change it, flip the test. if not, only remove underlines tagged with autoAdded: true like before.
|
|
||
| // Fallback link as node mark on run nodes. | ||
| if (from === to) { | ||
| // TODO |
There was a problem hiding this comment.
empty // TODO block. either delete it (the sweep below at L421-441 already handles this) or add an issue id per the comment policy.
2 participants
That's a branch out of fix/unset-link-clear-transient-hyperlink-styleid which fixes unlink of imported doc files.
resolving hyperlink range more defensively on empty selection
forcing a non-empty TextSelection over resolved link range before unsetting marks,
removing link marks from both inline text and run node marks,
clearing hyperlink-derived formatting from participating runs/text:
styleId, color, underline,
plus removing hyperlink-derived run key metadata arrays.