-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JENKINS-70730 Don't remove id inside symbol #7689
Conversation
Do these element IDs have different semantics than other IDs? Because short IDs like in the Jira issue look like they'd just invite ID collisions. |
They do not have different semantics you do have to make them unique (if they are different SVGs), ids are not that common and seem to be mostly used for gradients. You could post-process an svg to make the ids unique if needed. Here's a blog with a bit more info on it. |
@@ -94,7 +94,6 @@ private static String loadSymbol(String namespace, String name) { | |||
.replaceAll("(class=\").*?(\")", "") | |||
.replaceAll("(tooltip=\").*?(\")", "") | |||
.replaceAll("(data-html-tooltip=\").*?(\")", "") | |||
.replaceAll("(id=\").*?(\")", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that can lead to a duplicated id for the svg itself.
Assume the svg has an id at the root <svg id="symbolid" .../>
Now I want to use that svg with
<l:icon sec="mysymbol" id="myid"/>
The result will be 2 id attributes I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could but why would the svg have an id at the root baked into it?
Otherwise to fix it this string replace hack needs replacing with a dom library that only operates on the svg tag and not children.
/label ready-for-merge This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback. Thanks! |
See JENKINS-70730.
I can't see why we would need to remove existing ids in symbols, they're highly unlikely to be in an svg and this part of the code is before the caching is done.
Far better that we don't remove an id here than we do and break an svg completely....
Testing done
Unit test + interactively tested with https://issues.jenkins.io/browse/JENKINS-70729 and the azure vm agents plugin.
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
@Restricted
or have@since TODO
Javadocs, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
, if applicable.eval
to ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Maintainer checklist
Before the changes are marked as
ready-for-merge
:upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).