fix(icon): handle references for pages with base tag #12428
Conversation
googlebot
added
the
cla: yes
|
I can only see it being a breaking change if somebody was unit testing it and checking explicitly that it doesn't have a prefix, otherwise it should work exactly the same. |
| const funcIriAttributeSelector = funcIriAttributes.map(attr => `[${attr}]`).join(', '); | ||
|
|
||
| /** Regex that can be used to extract the id out of a FuncIRI. */ | ||
| const funcIriPattern = /^url\(['"]?#(.*?)['"]?\)$/; |
There was a problem hiding this comment.
Is there a link we could put here for some spec for this format? Should the # be optional?
There was a problem hiding this comment.
We could link to the SVG spec or MDN, but the link is longer than the description of the format: https://developer.mozilla.org/en-US/docs/Web/SVG/Content_type#FuncIRI. As for the #, it shouldn't be optional, because we only want to target references by ID.
| */ | ||
| private _prependCurrentPathToReferences(element: SVGElement) { | ||
| // @breaking-change 8.0.0 Remove this null check once `_location` parameter is required. | ||
| if (!this._location) { |
There was a problem hiding this comment.
Last thing: should we just restrict this to webkit, then? I suspect it's not hugely costly, but if we can easily/reliably cut out the extra work, we might as well.
There was a problem hiding this comment.
I went with doing it on all browsers since it shouldn't be too costly (unless an individual icon has hundreds of elements that all have one of the attributes) and to avoid having too many of these forks in the logic where we sniff out the browser and do special things just for one vendor.
There was a problem hiding this comment.
Looking a little closer- could we do this prepending at the icon-registry level so that it only has to happen once per svg? I didn't realize earlier it was happening for every directive instance.
There was a problem hiding this comment.
I left a comment for it a bit further down. Having it in the registry would be ideal, but we want the path at the time the icon was created, rather than when it was fetched. If we did it in the registry, it would be cached with the initial path.
There was a problem hiding this comment.
Does the path in <base> normally change?
There was a problem hiding this comment.
Doesn't the icon registry not request an SVG until it's used anyway?
I'm just hesitant to go forward with adding the extra work for pages that might have multiple icons in something like a large data table or option list.
There was a problem hiding this comment.
It does, but it also caches it afterwards. If performance is a concern, an alternative can be to have some kind of inexpensive check against the string representation of the icon before we do the querySelector and we walk through all the attributes. E.g. adding a MatIconRegistry.hasFuncIRI(iconName) method which just does a regex match or an indexOf.
There was a problem hiding this comment.
It's not the check so much as actually updating each uri that I'm worried about.
There was a problem hiding this comment.
I'm not sure whether it would be that expensive, compared to what we're doing for each icon already (cloning the cached SVG node).
|
Hi @crisbeto! This PR has merge conflicts due to recent upstream merges. |
crisbeto
force-pushed
the
9276/svg-references-prepend
branch
from
4911d4b to
301fd78
Compare
Prepends the current path to any SVG elements with attributes pointing to something by id. If the reference isn't prefixed, it won't work on Safari if the page has a `base` tag (which is used by most Angular apps that are using the router). Fixes angular#9276.
crisbeto
force-pushed
the
9276/svg-references-prepend
branch
from
301fd78 to
6b8868d
Compare
Prepends the current path to any SVG elements with attributes pointing to something by id. If the reference isn't prefixed, it won't work on Safari if the page has a `base` tag (which is used by most Angular apps that are using the router). Fixes angular#9276.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Prepends the current path to any SVG elements with attributes pointing to something by id. If the reference isn't prefixed, it won't work on Safari if the page has a
basetag (which is used by most Angular apps that are using the router).Fixes #9276.