Prefer href to xlink href #1059
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR modernizes SVG attribute usage by replacing deprecated xlink:href with href throughout the codebase—adding fallbacks in utilities, updating sanitization and paint logic, adjusting dynamic image handling, updating tests and UI fixtures, and bumping related dependency versions. Sequence Diagram: utilities.getHref LogicsequenceDiagram
participant C as Caller
participant U_getHref as "utilities.getHref"
participant E as Element
C->>U_getHref: getHref(E)
U_getHref->>E: getAttribute("href")
alt Has 'href' attribute
E-->>U_getHref: href_value
U_getHref-->>C: href_value
else No 'href' attribute
E-->>U_getHref: null
U_getHref->>E: getAttributeNS(XLINK_NAMESPACE, "href")
alt Has 'xlink:href' attribute
E-->>U_getHref: xlink_href_value
U_getHref-->>C: xlink_href_value
else No 'xlink:href' attribute
E-->>U_getHref: null
U_getHref-->>C: null
end
end
Sequence Diagram: utilities.setHref LogicsequenceDiagram
participant C as Caller
participant U_setHref as "utilities.setHref"
participant E as Element
C->>U_setHref: setHref(E, "new_value")
U_setHref->>E: setAttribute("href", "new_value")
E-->>U_setHref:
U_setHref-->>C:
Sequence Diagram: Paint Constructor Gradient Linking LogicsequenceDiagram
participant C as Caller
participant PC as "Paint.constructor"
participant GO as "options.gradientElement"
participant Doc as "document"
C->>PC: new Paint({ gradient: GO_value })
PC->>GO: getAttribute("href")
GO-->>PC: hrefResult
alt hrefResult is not null (href attribute exists)
PC->>Doc: getElementById(hrefResult)
Doc-->>PC: referencedGradient
PC->>referencedGradient: cloneNode(true)
referencedGradient-->>PC: clonedGradient
PC->>PC: this.gradient = clonedGradient
else hrefResult is null (href attribute does not exist)
PC->>GO: getAttributeNS(XLINK_NAMESPACE, "href")
GO-->>PC: xlinkHrefResult
alt xlinkHrefResult is not null (xlink:href attribute exists)
PC->>Doc: getElementById(xlinkHrefResult)
Doc-->>PC: referencedGradient
PC->>referencedGradient: cloneNode(true)
referencedGradient-->>PC: clonedGradient
PC->>PC: this.gradient = clonedGradient
else No link attributes found (neither href nor xlink:href)
PC->>GO: cloneNode(true)
GO-->>PC: clonedGradient
PC->>PC: this.gradient = clonedGradient
end
end
Sequence Diagram: sanitizeSvg Local href Reference ChecksequenceDiagram
participant SanitizeFn as "sanitizeSvg()"
participant Node as "Element (e.g., <use>)"
participant U_getHref as "utilities.getHref"
participant U_setHref as "utilities.setHref"
SanitizeFn->>U_getHref: getHref(Node)
U_getHref-->>SanitizeFn: currentHrefValue
alt currentHrefValue exists AND is non-local AND Node type requires local href
SanitizeFn->>U_setHref: setHref(Node, "")
U_setHref-->>SanitizeFn:
SanitizeFn->>Node: removeAttributeNS(XLINK_NAMESPACE, "href")
Node-->>SanitizeFn:
SanitizeFn->>Node: removeAttribute("href")
Node-->>SanitizeFn:
SanitizeFn->>SanitizeFn: Log warning (non-local href removed)
end
alt Node is <use> AND getHref(Node) is now falsy
SanitizeFn->>U_getHref: getHref(Node) // Re-check or use updated state
U_getHref-->>SanitizeFn: null_or_empty_href
SanitizeFn->>Node: remove()
Node-->>SanitizeFn:
SanitizeFn->>SanitizeFn: Log warning (<use> element removed)
end
Updated Class Diagram: SVG Attribute Handling ModulesclassDiagram
namespace svgcanvas.core {
class utilities {
<<Module>>
+getHref(elem: Element) : string
+setHref(elem: Element, val: string) : void
}
class Paint {
+Paint(options: Object)
#linearGradient: Element
#radialGradient: Element
}
class sanitize {
<<Module>>
+svgWhiteList_ : Object
+sanitizeSvg(node: Element) : void
}
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull Request Overview
This pull request replaces all remaining uses of xlink:href with href in SVG elements to follow modern SVG standards. The key changes include updates in modules that create or manipulate SVG elements, modifications to the sanitizer utility and test cases, and dependency bumps for supporting libraries.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/editor/extensions/ext-overview_window/ext-overview_window.js | Updated the element to use href instead of xlink:href. |
| src/editor/components/jgraduate/jQuery.jGraduate.js | Replaced setAttributeNS with setAttribute to update image href attributes. |
| packages/svgcanvas/core/utilities.js | Refactored getHref and setHref functions to prefer href while maintaining backward compatibility. |
| packages/svgcanvas/core/svg-exec.js | Updated attribute modifications in image elements, though one usage remains inconsistent. |
| packages/svgcanvas/core/sanitize.js | Updated whitelist definitions and warning messages to support href. |
| packages/svgcanvas/core/paint.js | Modified gradient extraction logic to support both href and xlink:href. |
| package.json | Bumped versions for svgcanvas, i18next, babel, cypress and related dependencies. |
| cypress/e2e/unit/.cy.js & cypress/e2e/ui/.cy.js | Updated tests to verify changes from xlink:href to href. |
Comments suppressed due to low confidence (1)
packages/svgcanvas/core/svg-exec.js:445
- For consistency with other changes that use setAttribute for the href attribute, consider using setAttribute('href', url) instead of setAttributeNS with NS.XLINK.
image.setAttributeNS(NS.XLINK, 'href', url)
There was a problem hiding this comment.
Hey @jfhenon - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Incorrect
setAttributeusage with namespace URI as name (link)
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 3 other issues
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const seAttrNS = (attrName.startsWith('se:')) ? NS.SE : ((attrName.startsWith('oi:')) ? NS.OI : null) | ||
| seAttrs.push([attrName, attr.value, seAttrNS]) | ||
| } else { | ||
| console.warn(`sanitizeSvg: attribute ${attrName} in element ${node.nodeName} not in whitelist is removed: ${node.outerHTML}`) |
There was a problem hiding this comment.
suggestion (performance): Logging outerHTML on every warning may be heavy
Logging the entire outerHTML may generate excessive output and affect performance. Suggest logging only the tag name or a shortened snippet instead.
Suggested change
| console.warn(`sanitizeSvg: attribute ${attrName} in element ${node.nodeName} not in whitelist is removed: ${node.outerHTML}`) | |
| const attrValSnippet = attr.value && attr.value.length > 30 | |
| ? attr.value.slice(0, 30) + '...' | |
| : attr.value; | |
| console.warn( | |
| `sanitizeSvg: attribute ${attrName} (value: "${attrValSnippet}") in <${node.nodeName}> not in whitelist and is removed` | |
| ) |
| } else if (options.linearGradient) { | ||
| this.type = 'linearGradient' | ||
| this.solidColor = null | ||
| this.radialGradient = null | ||
| if (options.linearGradient.hasAttribute('xlink:href')) { | ||
| const xhref = document.getElementById(options.linearGradient.getAttribute('xlink:href').substr(1)) | ||
| const hrefAttr = |
There was a problem hiding this comment.
suggestion: Duplicate attribute‐lookup logic
Consider extracting the repeated attribute-lookup logic into a helper function or reusing getHref to reduce duplication and maintain consistent fallback behavior.
Suggested implementation:
const hrefAttr = getHref(options.linearGradient)
if (hrefAttr) {
const xhref = document.getElementById(hrefAttr.replace(/^#/, ''))
this.linearGradient = xhref.cloneNode(true)
} else {
this.linearGradient = options.linearGradient.cloneNode(true)
} // create linear gradient paint
} else if (options.radialGradient) {
this.type = 'radialGradient'/**
* Helper to get the 'href' or 'xlink:href' attribute from an element.
* Returns null if neither is present.
*/
function getHref(element) {
return element.getAttribute('href') || element.getAttribute('xlink:href')
}
packages/svgcanvas/core/svg-exec.js
Outdated
| @@ -443,7 +443,7 @@ const setSvgString = (xmlString, preventUndo) => { | |||
| // const url = decodeURIComponent(m.groups.url); | |||
| const iimg = new Image() | |||
| iimg.addEventListener('load', () => { | |||
| image.setAttributeNS(NS.XLINK, 'xlink:href', url) | |||
| image.setAttribute(NS.XLINK, 'href', url) | |||
There was a problem hiding this comment.
issue (bug_risk): Incorrect setAttribute usage with namespace URI as name
setAttribute is being used incorrectly here. To set a namespaced href, use setAttributeNS(NS.XLINK, 'href', url). For a non-namespaced attribute, use setAttribute('href', url) instead.
1c5300b to
c9e1668
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR description
Summary
This pull request replaces all remaining uses of xlink:href with href in accordance with modern SVG standards. The xlink:href attribute has been deprecated and is no longer necessary in SVG 2, so this change improves compatibility and future-proofs the codebase.
Details![]()
) still work as expected.
Refactored all occurrences of xlink:href to use href instead.
Updated relevant code, documentation, and test cases where applicable.
Ensured backward compatibility with browsers that support only href.
Testing
Manually tested the UI to verify that SVG elements referencing other elements (e.g., ,
Ran automated tests to ensure no regressions were introduced.
Summary by Sourcery
Replace deprecated xlink:href attributes with modern href across the codebase to comply with SVG2 standards and maintain backward compatibility.
Enhancements:
Build:
Tests: