Problem/Motivation
When CSS aggregation is enabled and SVG is imported in css with a fragment URL i.e. #
or %23
eg: background: url(#SVGID_1_);
Here, url(#SVGID_1_)
gets converted to url(http://domain.com/path/path/#SVGID_1_1)
resulting in a broken SVG.
Steps to reproduce
Enable CSS aggregation from /admin/config/development/performance
Use an SVG with a fragment URL. eg: background: url(#SVGID_1_);
Proposed resolution
Ignore fragment URL when building and returning the path of the aggregated stylesheets.
Remaining tasks
Review
Commit
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
None
Comment | File | Size | Author |
---|---|---|---|
#98 | 2362643-98.patch | 13.35 KB | _utsavsharma |
#98 | interdiff_d10.txt | 13.35 KB | _utsavsharma |
#96 | drupal-10.0.10-fix_broken_svg-2362643-96.patch | 13.31 KB | wsantell |
#94 | 2362643-nr-bot.txt | 150 bytes | needs-review-queue-bot |
Issue fork drupal-2362643
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 2362643-drupal-alters-svg changes, plain diff MR !1522
Comments
Comment #1
GijsRoge CreditAttribution: GijsRoge commentedComment #2
GijsRoge CreditAttribution: GijsRoge commentedComment #3
mikeytown2 CreditAttribution: mikeytown2 commentedDoes this broken svg issue happen when advagg is disabled but core aggregation is enabled?
Comment #4
GijsRoge CreditAttribution: GijsRoge commentedYes, I noticed yesterday it doesn't have anything to do with advagg. (Icouldn't update my post anymore because of the anti-spam protection)
I'll move it.
Comment #5
GijsRoge CreditAttribution: GijsRoge commentedComment #6
mikeytown2 CreditAttribution: mikeytown2 commentedIt is a core issue, but it cam be fixed in advagg. We'll need to fix in advagg, then 7.x so it can be tested then see if 8.x needs the fix and then fix it there so it can be committed in 7.x. Long story short it will be fixed in advagg before it's fixed in core.
Comment #7
GijsRoge CreditAttribution: GijsRoge commentedAlright, thats good news. Thanks!
Comment #8
mikeytown2 CreditAttribution: mikeytown2 commentedMoving this back to AdvAgg; will then move to core once it's fixed here. Will also ping the CDN module as this will likely be an issue there as well.
Code in AdvAgg: _advagg_build_css_path()
Related issues
#1514182: IE @font-face CSS hack URL broken by CDN module's overridden CSS aggregation
#1961340: CSS aggregation breaks file URL rewriting because it abuses it
Comment #9
mikeytown2 CreditAttribution: mikeytown2 commentedComment #11
mikeytown2 CreditAttribution: mikeytown2 commentedCommitted this patch; I edited the regex so that URL's that start with # do not get altered.
Comment #12
GijsRoge CreditAttribution: GijsRoge commentedBrilliant, thanks!Issue is still present after applying the patch, rebuilding aggregates and clearing cache
output
<path xmlns="http://www.w3.org/2000/svg" fill="url(/sites/all/themes/elipse/css/#a)" d="M1256.4 386h1500v21h-1500v-21z"/>
Should be
<path xmlns="http://www.w3.org/2000/svg" fill="url(#a)" d="M1256.4 386h1500v21h-1500v-21z"/>
Comment #13
GijsRoge CreditAttribution: GijsRoge commentedComment #14
mikeytown2 CreditAttribution: mikeytown2 commentedCan I get the full context in how this svg is used. I was thinking it was inside a css file.
This is the test code with the assumption that
url(#a)
is inside a css file.Output:
Comment #15
GijsRoge CreditAttribution: GijsRoge commentedIts used inside a css file within data-url(''); (so perhaps you could just ignore everything inside a data url as they do not have to contain relative paths) wich is a more elegant way of fixing this problem perhaps.
And your new code output seems to be correct.
This is the result of the applied patch http://take.ms/5R0TC (seems to be correct, so its weird that it works for you but not for me)
I have the exact same use case as you described in the post above.
Is it possible that this
fill="url('#SVGID_1_')"
(notice the single quotes inside) causes the regex to fail?Comment #16
mikeytown2 CreditAttribution: mikeytown2 commentedSingle/double or no quotes doesn't affect the regex matching from my testing.
url('data:
gets ignored because the regex skips all strings inside of url that start with{scheme}:
([a-z]+:), slashes (\/+) and now the number sign (\#+).I still can't repo the bug since I committed the patch in #11. If you're worried about posting the full CSS code here that contains the SVG, you can use my contact info to point me toward the file so I can fix the bug.
Comment #17
mikeytown2 CreditAttribution: mikeytown2 commentedMoving this to core.
Following patch should fix SVG issues in D8 css.
Comment #18
mikeytown2 CreditAttribution: mikeytown2 commented7.x patch
Comment #19
mikeytown2 CreditAttribution: mikeytown2 commented7.x & 8.0.x pass; moving to D8 for core inclusion.
@GijsRoge
Try applying the D7 patch in #18; that might fix it for you.
Comment #20
GijsRoge CreditAttribution: GijsRoge commented@Mikeytown2 Issue still exists after patch from post #18
Link to the full css file -> https://dl.dropboxusercontent.com/u/15198233/main.css ( line: 859 )
Comment #21
mikeytown2 CreditAttribution: mikeytown2 commentedfound it!
%23 is also #. Will need to adjust the regex to account for this and write a test as well.
Comment #23
mikeytown2 CreditAttribution: mikeytown2 commentedCommitted to AdvAgg. moving back to core.
Comment #24
Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commentedComment #27
kevinquillen CreditAttribution: kevinquillen commentedCan confirm this is a bug. Here is the CSS on an SVG element:
Without core aggregation of CSS enabled, it works fine. When I enable it, it tacks a URL on to the beginning #diagonal-up, breaking it.
The patch in #17 worked for me.
Comment #28
parkecag CreditAttribution: parkecag commentedPosting a re-roll of patch from #17.
Comment #29
RainbowArrayWe will need to update this patch to include the check for %23 that was noted in #21.
Comment #30
parkecag CreditAttribution: parkecag commentedNew patch that checks for %23 also.
Comment #31
RainbowArrayReviewed the patch and tested it, appears to work. Great work parkecag!
Comment #32
Wim LeersThis needs test coverage, to prevent this from regressing again in the future.
Comment #35
ericclaeren CreditAttribution: ericclaeren at ezCompany commentedThe patch in #2362643-18: Drupal alters svg fill paths with base url -> broken svg didn't work for me, the updated D8 regex into D7 did, will try to create an updated patch asap.
Comment #36
Wim LeersWhy did you change the version? This needs to be fixed in Drupal 8 first.
Comment #37
ericclaeren CreditAttribution: ericclaeren at ezCompany commentedExcuse me, didn't do that on purpose. Thought it was for my reply only.
I've attached a patch for Drupal 7 based on the fix in #2362643-30: Drupal alters svg fill paths with base url -> broken svg
Comment #39
Pls CreditAttribution: Pls as a volunteer commented#30 patch works great for Drupal 8.2.3. Would love to push and get this in core, as this is pretty major for specific use cases, where svg is used.
@Wim Leers, I would love to create missing tests here. But I need more info how they should be properly created. Have not much experience with writing core tests really, but want to learn. Thanks :)
Comment #40
Wim Leers@Pls: Thanks for taking this on! Here's some guidance for you:
These changes can be tested by expanding the test coverage in
\Drupal\Tests\Core\Asset\CssOptimizerUnitTest
.Plenty of examples to look at.
Testing this one is going to be a bit more difficult.
But I think you should be able to expand the test coverage in
\Drupal\Tests\color\Functional\ColorTest::_testColor()
:)Comment #41
Pls CreditAttribution: Pls as a volunteer commented@Wim Leers: Thanks for your pointers, really helped me starting up, appreciate your help!
Added patch with test dummy data added for CssOptimizerUnitTest. Let me know what you think.
Haven't figured out how to write tests for Color module. It looks like it compares whole stylesheet with test data from first look. Do you guys have any ideas or suggestions how to implement this better? :)
Comment #42
Pls CreditAttribution: Pls as a volunteer commentedComment #44
Wim Leers#41 looks great :)
No, not without refactoring all of its test coverage. I'd just continue the existing way of testing: use
$this->assertTrue(strpos(…
or$this->assertPattern(…
.Comment #45
Wim LeersComment #47
mbrc CreditAttribution: mbrc at Unic commentedSame patch as #41, but applies to 8.3. Still needs additional tests.
Comment #49
kala4ek#47 works perfectly for me
:+1::skin-tone-2:
Comment #51
Rob230 CreditAttribution: Rob230 commented#47 is working great for me. What do we need to get this fix in core?
Comment #52
johnny5th CreditAttribution: johnny5th at Encore Multimedia commented#47 fixed my issue!
Comment #53
idebr CreditAttribution: idebr at ezCompany commentedClosed #2994490: CssOptimizer doesn't handle SVG clipPath urls properly as a duplicate issue.
Comment #54
idebr CreditAttribution: idebr at iO commentedClosed #2784107: SVG CSS marker-start: url(#id) properties break when aggregated as a duplicate issue.
Comment #56
jcsparks CreditAttribution: jcsparks commentedTesting with Drupal 8.6.0 - #47 still works for me. How do we get this rolled into core?
Comment #57
andypostComment #58
kala4ekJust checked, #47 perfectly applies to current 8.7.x.
According to the latest comments, mark it is rtbc.
Comment #59
andypostComment #60
alexpottIs it an SVG path? I think we should use the language from CSS documentation. These appear to be
fragment URLs
. See https://drafts.csswg.org/css-values-3/#local-urlsGoing down that path I think we also have a few more bugs here because we should not prefix the idents
inherit
,initial
andunset
- let's open a follow-up for that.Why are we doing
%23
as well? This is not tested. We need to add test coverage if this is necessary.Comment #61
bendev CreditAttribution: bendev at WebstanZ commentedtested #47 successfully.
Thanks for the patch !
Comment #63
acbramley CreditAttribution: acbramley at PreviousNext commentedThis is to catch any paths using the HTML encoded version of "#someID". It is tested in the .url .inline-svg definition with:
....url(%23SVGID_1_)....
It's a bit hard to see but it's in there ;)
This patch fixes #60.1. Also adding a test only patch.
Opened #3077821: [PP-1] Do not prefix the idents inherit, initial and unset during CSS optimisation as a follow up as per #60
Comment #65
Guietc CreditAttribution: Guietc commentedThe patch #63 works great. Thanks.
Comment #68
RaphaelBriskie CreditAttribution: RaphaelBriskie commentedCan confirm this patch works to fix this issue.
Comment #70
quietone CreditAttribution: quietone as a volunteer commentedReviewing RTBC for Bug Smash Initiative, thus adding tag.
The first thing I notice is that the issue summary is an explanation of the problem and nothing about the proposed resolution. Of course, it is kind of implied, that we don't want to have broken svg. Still it would be helpful to include how one is going to fix the problem in the IS.
Reading the patch there are two references to this issue, neither with the usual format of 'See https...'. I wonder if pointing to this issue is necessary. Why not just expand the comment to say what it is doing to keep the url correct? Keep in mind, this is an area I know little about so I could be way off the track here.
Should this be 'See https://www.drupal.org/project/drupal/issues/2362643' or an expanded comment?
Again, same as above
Comment #72
raman.b CreditAttribution: raman.b at OpenSense Labs commentedApplying IS template
Comment #73
raman.b CreditAttribution: raman.b at OpenSense Labs commentedComment #74
Ruuds CreditAttribution: Ruuds at Groowup Digital Agency commentedThanks, the patch of #72 works great, even on 8.9.11.
Comment #75
lauriiiThere are some failures in the CI run for #72.
Comment #76
raman.b CreditAttribution: raman.b at OpenSense Labs commentedFixing CI failures
Comment #77
W01F CreditAttribution: W01F commentedWould it be possible to backport this patch to be compatible with D8? I have a number of sites with this issue and haven't yet made the leap to get the sites upgraded to D9 >< - even though I know it's supposed to be a small leap!
Comment #78
aspilicious CreditAttribution: aspilicious commentedWorks great all the comments are adressed
Comment #79
Ruuds CreditAttribution: Ruuds at Groowup Digital Agency commentedThanks, still works great for me too!
Comment #80
larowlanBack in #40 Wim asked for tests for the color module changes, but they seemed to have slipped through, i.e. we're missing coverage for those changes.
Needs work for that
Comment #82
hswong3i CreditAttribution: hswong3i commentedPatch re-roll via 9.3.x-dev, also works with 9.2.8.
Comment #85
hswong3i CreditAttribution: hswong3i at PantaRei Design Limited (Hong Kong) commentedPR created from #82
Comment #86
agoradesign CreditAttribution: agoradesign commented#82 works for me :)
Comment #87
b_sharpe CreditAttribution: b_sharpe at ImageX commentedAlthough this appears to fix references such as
url(#someSVG);
, I'm finding it is breaking inline SVGs by removing spaces.Pre-patch:
Post-patch:
Comment #88
b_sharpe CreditAttribution: b_sharpe at ImageX commentednvm, was YUI compression on adv/agg: #3069151: Compatibility problem between YUI compressor and @supports CSS property
Comment #89
W01F CreditAttribution: W01F commentedI can also confirm the latest #82 patch is working on latest 9.3.12 with both core and advagg aggregation enabled.
Comment #90
malcomio CreditAttribution: malcomio as a volunteer and at Capgemini commentedI was encountering the clip-path issue described in #2994490: CssOptimizer doesn't handle SVG clipPath urls properly, and the patch in #82 fixes that for me.
Comment #92
tdnshah CreditAttribution: tdnshah as a volunteer commentedI was facing the issue with clip-path with the core version 9.4.5 and #82 fixes that for me.
Comment #94
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #96
wsantell CreditAttribution: wsantell commentedThe patch for #81 fails due to Color being removed from Core. The attached patch is a rewrite of #81 with the color.module patch removed. It works against the 10.0.10 branch; not tested against 10.1.x
Comment #97
xaa CreditAttribution: xaa as a volunteer commentedhi, #96 doesn't seems apply on 10.1.x
Comment #98
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedPatch for 10.1.x that failed in #96.
Comment #99
xaa CreditAttribution: xaa as a volunteer commentedAwesome, thank you! Patch is working on 10.1.x using php 8.1
Comment #101
mstrelan CreditAttribution: mstrelan at PreviousNext commented