fix: support hyperlinks on DrawingML images (a:hlinkClick)#2552
fix: support hyperlinks on DrawingML images (a:hlinkClick)#2552iguit0 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: 7bea148896
ℹ️ 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 anchor = this.doc.createElement('a'); | ||
| anchor.href = sanitized.href; | ||
| anchor.classList.add('superdoc-link'); |
There was a problem hiding this comment.
Wrapping image elements with a.superdoc-link causes EditorInputManager.#handlePointerDown to take the link fast-path (closest('a.superdoc-link')) and return before the inline/block image selection paths run (#handleInlineImageClick / #handleFragmentClick). As a result, in editable documents, clicking a hyperlinked image no longer produces image node selection or resize-overlay activation, so linked images become effectively non-selectable by pointer.
Useful? React with 👍 / 👎.
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@iguit0 nice contribution, thanks for tackling this! the approach looks good — checked the spec and the dual placement, relationship handling, and tooltip are all done right.
one small thing on tooltip encoding (left a suggestion). one optional cleanup on shared code.
tests: block-level is well covered. the inline paths (imageNodeToRun and the 3 renderer return paths) don't have hyperlink tests yet — would be great to add those.
left a few inline comments.
| } | ||
| if (hyperlink.tooltip) { | ||
| anchor.title = hyperlink.tooltip; | ||
| } |
There was a problem hiding this comment.
text links run the tooltip through encodeTooltip() which trims whitespace and caps it at 500 chars. this skips that — a crafted docx with a huge tooltip would render it in full. worth matching the text link behavior?
| } | |
| if (hyperlink.tooltip) { | |
| const tooltipResult = encodeTooltip(hyperlink.tooltip); | |
| if (tooltipResult?.text) { | |
| anchor.title = tooltipResult.text; | |
| } | |
| } |
| ...(flipH !== undefined && { flipH }), | ||
| ...(flipV !== undefined && { flipV }), | ||
| // Image hyperlink from OOXML a:hlinkClick | ||
| ...(() => { |
There was a problem hiding this comment.
same parsing logic exists in inline-converters/image.ts:170. a small shared helper in utilities.ts would keep them in sync and also get rid of the IIFE here.
| const fragment = { | ||
| kind: 'image' as const, | ||
| blockId: 'linked-img', | ||
| x: 20, |
There was a problem hiding this comment.
layout is created here but never used — the tests build their own. can be removed.
|
Nice work on this. I independently implemented the same fix on my fork while digging through #2065, and I converged on the same root cause. One thing I found useful was reusing the existing shared link pipeline ( / / ) rather than introducing an image-specific hyperlink path, since it automatically inherits the current blocked-link handling, dataset propagation, tooltip accessibility, and other shared link behavior. I also added explicit inline-image coverage in both the PM adapter and DOM painter tests, since the inline paths were the main gap I noticed while reviewing. If any of that is helpful, happy to share or compare against my branch here: https://github.com/PeterHollens/superdoc/tree/feature/drawingml-image-hyperlinks |
|
Nice work on this. I independently implemented the same fix on my fork while One thing I found useful was reusing the existing shared link pipeline If any of that is helpful, happy to share or compare against my branch here: |
3 participants
Summary
DOCX images with
a:hlinkClickhyperlinks rendered correctly in Word but clicking them in SuperDoc did nothing. The importer already parsed the relationship and storedattrs.hyperlinkon the ProseMirror image node — but the hyperlink was silently dropped further down the pipeline, so the renderer never had a URL to render.Root cause: a three-step gap between import and rendering:
ImageBlock/ImageRuncontract types had nohyperlinkfieldimageNodeToBlockandimageNodeToRundid not forward the attrDomPainter.renderImageFragment/renderImageRunnever created an anchorWhat Changed
hyperlink?: { url: string; tooltip?: string }to bothImageBlockandImageRunin the shared contracts packageimageNodeToBlockandimageNodeToRun, with the same validation (non-empty string URL) used for other optional fieldsDomPainter.buildImageHyperlinkAnchor()— a private helper that wraps an image element in<a class="superdoc-link">when a valid hyperlink is present. UsessanitizeHref(same guard as text links) and appliestarget/relfor external URLs with accessibility attributes mirroringapplyLinkAttributesrenderImageFragment(block images) and at all three return points ofrenderImageRun(inline images with/without clip-path wrapper)The existing
EditorInputManagerclick-delegation already detectsa.superdoc-linkon pointer-down and dispatchessuperdoc-link-click.LinkClickHandlerthen opens the URL in viewing mode — no changes needed in either component. Unlinked images are unaffected:buildImageHyperlinkAnchoris a no-op whenhyperlinkis undefined, and image resize/selection behavior is preserved because the fragment<div>(which carriesdata-image-metadata) remains the outermost element.Test Plan
pm-adapter/src/converters/image.test.ts— verifieshyperlinkis forwarded with tooltip, without tooltip, and is omitted for null/empty casespainters/dom/src/index.test.ts— verifies linked images render as<a class="superdoc-link">with correcthref, tooltip becomestitle, unlinked images have no anchor, and unsafe URLs (e.g.javascript:) are blockedFixes #2065