- 🇺🇸United States brobert7
I've verified that I'm seeing the same behavior - my
clip-path: url(#something)
gets rewritten to a relative link and breaks the SVG reference.Adding
{ preprocess: false }
to the stylesheet reference in the THEME.libraries.yml file DOES work, but{ minified: true }
DOES NOT. - 🇬🇧United Kingdom DanLew Birmingham
I've just run into this on core 10.0.7.
Turning off the aggregation stops the problem, but is definitely a workaround, not a fix.
The original suggestion of testing the path to see if it starts with `#` seems like a sensible idea. I can't think of any reason for that not working? Can anyone smarter than me think of any reason that would fall over?
- 🇩🇪Germany danielehrenhofer Frankfurt
The problem also exists on core 10.1.x-dev.
The workaround described here, switching off aggregation, still solves the problem.
- Status changed to Needs review
over 1 year ago 10:37am 17 July 2023 - last update
over 1 year ago 29,446 pass - Status changed to Needs work
over 1 year ago 2:25pm 17 July 2023 - 🇺🇸United States smustgrave
As a bug can we get a test case showing this problem?
- 🇩🇪Germany mkalkbrenner 🇩🇪
I just can see the issue on our website that some "parts" of SVGs embedded in CSS are missing. The patch in #7 fixes this issue and the SVGs are "completely visible" again.
I can't tell if the fix suggested in the issue description here is correct for all use-cases, but it fixes our issue. So I shared the patch here as a beginning.
I would leave the addition of tests to someone who has better knowledge about SVG and who can judge if the suggested patch might cause other issues instead of just solving the clip-path issue.
- Status changed to Needs review
over 1 year ago 6:10am 21 July 2023 - 🇮🇳India heykarthikwithu Bengaluru 🌍
Hi @smustgrave, in regards to
As a bug can we get a test case showing this problem?
Below is the test case shows the problem.
karthik.dk@drupal_contribute % git diff core diff --git a/core/lib/Drupal/Core/Asset/CssOptimizer.php b/core/lib/Drupal/Core/Asset/CssOptimizer.php index bc2aef7238..413106cf98 100644 --- a/core/lib/Drupal/Core/Asset/CssOptimizer.php +++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php @@ -90,8 +90,8 @@ protected function processFile($css_asset) { // Store base path. $this->rewriteFileURIBasePath = $css_base_path . '/'; - // Anchor all paths in the CSS with its base URL, ignoring external and absolute paths. - return preg_replace_callback('/url\(\s*[\'"]?(?![a-z]+:|\/+)([^\'")]+)[\'"]?\s*\)/i', [$this, 'rewriteFileURI'], $contents); + // Anchor all paths in the CSS with its base URL, ignoring external and absolute paths and paths starting with '#'. + return preg_replace_callback('/url\(\s*[\'"]?(?![a-z]+:|\/+|#|%23)([^\'")]+)[\'"]?\s*\)/i', [$this, 'rewriteFileURI'], $contents); } /** diff --git a/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php b/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php index 2c49928416..347759527f 100644 --- a/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php +++ b/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php @@ -222,6 +222,18 @@ public function providerTestOptimize() { ], file_get_contents($absolute_path . 'quotes.css.optimized.css'), ], + [ + [ + 'group' => -100, + 'type' => 'file', + 'weight' => 0.013, + 'media' => 'all', + 'preprocess' => TRUE, + 'data' => $path . 'import3.css', + 'basename' => 'import3.css', + ], + file_get_contents($absolute_path . 'import3.css.optimized.css'), + ], ]; } karthik.dk@drupal_contribute % ddev exec phpunit -c core core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php PHP Warning: file_get_contents(/var/www/html/core/tests/Drupal/Tests/Core/Asset/css_test_files/import3.css.optimized.css): Failed to open stream: No such file or directory in /var/www/html/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php on line 235 Warning: file_get_contents(/var/www/html/core/tests/Drupal/Tests/Core/Asset/css_test_files/import3.css.optimized.css): Failed to open stream: No such file or directory in /var/www/html/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php on line 235 PHPUnit 9.6.10 by Sebastian Bergmann and contributors. Testing Drupal\Tests\Core\Asset\CssOptimizerUnitTest ............F.. 15 / 15 (100%) Time: 00:00.518, Memory: 4.00 MB There was 1 failure: 1) Drupal\Tests\Core\Asset\CssOptimizerUnitTest::testOptimize with data set #12 (array(-100, 'file', 0.013, 'all', true, 'core/tests/Drupal/Tests/Core/...t3.css', 'import3.css'), false) Group of file CSS assets optimized correctly. Failed asserting that 'div{clip-path:url('#clip-cloud')}\n ' matches expected false. /var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96 /var/www/html/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php:255 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684 /var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651 /var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:144 /var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:97 FAILURES! Tests: 15, Assertions: 17, Failures: 1. Failed to execute command phpunit -c core core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php: exit status 1 karthik.dk@drupal_contribute %
Here's code within import3.css
div { clip-path: url('#clip-cloud') }
Thanks :)
- Status changed to Needs work
over 1 year ago 1:10pm 21 July 2023 - Status changed to Needs review
over 1 year ago 3:12am 24 July 2023 - last update
over 1 year ago 29,451 pass - 🇮🇳India heykarthikwithu Bengaluru 🌍
Hi @smustgrave, in regards to
Can we add the test case to the patch.
Attached the patch file with test case & inter diff changes.
Log for reference:
karthik.dk@drupal_contribute % ddev exec phpunit -c core core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php PHPUnit 9.6.10 by Sebastian Bergmann and contributors. Testing Drupal\Tests\Core\Asset\CssOptimizerUnitTest ............... 15 / 15 (100%) Time: 00:00.521, Memory: 4.00 MB OK (15 tests, 17 assertions) karthik.dk@drupal_contribute %
Thanks :)
- last update
over 1 year ago 29,882 pass - Status changed to RTBC
over 1 year ago 2:50pm 27 July 2023 - 🇺🇸United States smustgrave
Thanks but you don't need to post the output of the test passing, that's shown by the patch.
Typically uploading a test-only patch would be good though.
But ran the test locally. and got
Failed asserting that two strings are equal.
Expected: 'div{clip-path:url('#clip-cloud');}\n
Actual: 'div{clip-path:url(generated-relative-url:core/tests/Drupal/Tests/Core/Asset/css_test_files/#clip-cloud);}\nSo the test appears to be covering
Good job!
- last update
over 1 year ago 29,455 pass - Status changed to Needs review
over 1 year ago 2:08pm 28 July 2023 - 🇬🇧United Kingdom catch
Do we have or need coverage for when the URL is relative but contains a
#
? - 🇮🇳India heykarthikwithu Bengaluru 🌍
Hi @catch I see, we don't need coverage for this scenario.
I have checked locally with below scenario
div { clip-path: url('#clip-cloud'); } div { clip-path: url('/abc/#clip-cloud'); }
div{clip-path:url('#clip-cloud');}div{clip-path:url('/abc/#clip-cloud');}
I have noticed, bug described in the issue summary will not show up with the case of relative url, like
clip-path: url('/abc/#clip-cloud');
So, we don't need coverage for this scenario. - 🇬🇧United Kingdom catch
@heykarthikwithu it's not the question of whether there's a bug now, it's adding test coverage to show that there isn't and preventing one being introduced in the future. I think that just adding an extra bit of CSS to the test file and assertion would be enough.
- last update
over 1 year ago 29,456 pass - 🇮🇳India heykarthikwithu Bengaluru 🌍
@catch,
I think that just adding an extra bit of CSS to the test file and assertion would be enough.
sure.. here's the patch.
- Status changed to RTBC
over 1 year ago 7:27am 31 July 2023 - Status changed to Fixed
over 1 year ago 9:14am 1 August 2023 - 🇬🇧United Kingdom catch
Committed/pushed to 11.x and cherry-picked to 10.1.x, thanks!
Automatically closed - issue fixed for 2 weeks with no activity.