Make referencing IDs in inline rendered SVGs unique #4225
Conversation
|
@petschki thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
|
@jenkins-plone-org please run jobs |
|
IIRC correctly, IDs were stripped to avoid duplicated IDs in DOM, which lead to failures in W3C site validation. |
|
Can't you just can replace/prepend them with a random string? |
|
Yeah that also came into my mind. Let me try something this weekend. |
petschki
changed the title
|
I've refactored this to make the referencing IDs unique. I use a hash of I've also fixed the let me know what you think. |
|
@jenkins-plone-org please run jobs |
|
Also worth noting: the |
| del el.attrib["id"] | ||
| def _unique_id(svgtree, cfg): | ||
| # for <use (xlink:)href="#someid" /> references to work | ||
| # the ids in the svg must be unique in the document |
There was a problem hiding this comment.
How do you actually use this svg in a <use /> reference if its id includes an unpredictable hash?
There was a problem hiding this comment.
well, the idea was to:
- create a unique hash on each
tag()call - replace all
id="xyz"withid="xyz-<hash>"inside the SVG - replace all referencing
#xyzoccurrences with#xyz-<hash>inside the SVG node attributes
while this worked in my local tests it seems, that there are breaking CI tests, because calling the same template with a delay isn't equal anymore (see https://github.com/plone/Products.CMFPlone/blob/6.1.x/Products/CMFPlone/tests/testBrowserDefault.py#L67)
so, to make this hash more predictable, we could create a hash from the svg filename including a counter of the occurrence on the page.
Products/CMFPlone/browser/icons.py
Outdated
| modifier_cfg = { | ||
| "cssclass": tag_class, | ||
| "title": tag_alt, | ||
| "hash": hashlib.md5(str(time.time()).encode()).hexdigest(), |
There was a problem hiding this comment.
time.time() is not guaranteed to have resolution higher than 1 second on all platforms (https://docs.python.org/3/library/time.html#time.time)
I recommend using time.perf_counter() instead -- but see my other comment
also fix the "aria-labelledby" by adding a unique id to the title tag
petschki
force-pushed
the
svg-remove-stripid-svgmodifier-6.1.x
branch
from
713afa3 to
33e9d3a
Compare
|
@jenkins-plone-org please run jobs |
|
@jenkins-plone-org please run jobs |
fixes #4224