Un-inline SVGs in pcss.css files, add build tool to inline them when compiled

I think we better insert the svg icons for the action links in the html. This way we'll need a lot less images since we can set the color with css (see https://css-tricks.com/change-color-of-svg-on-hover/#article-header-id-0)

This patch uses postcss-url Nothing inlined has been un-inlined yet. Just providing this here for evaluation. Not entirely sure if other alternatives exist but I'll do some investigating before I jump into a dependency evaluation on this one.

The other plugin option would be https://www.npmjs.com/package/postcss-inline-svg, this also inlines SVGs, with pretty distinct pros and cons against postcss-url:
CON: inlining requires additional non-CSS syntax. The SVG Urls need to be wrapped in a plugin-provided function. svg-load('img/arrow-up.svg'), while postcss-url acts on standard CSS syntax
PRO: it is possible to add args that make it possible to control attributes with that function svg-load('img/arrow-up.svg', fill=#000, stroke=#fff), so that could reduce the total amount of SVG files needed (although of course it would not reduce the size of the compiled CSS files).

For now, postcss-url seems fine. Since it's using standard CSS syntax, it should be easy for us to convert to svg-load if we deem that useful in the future.

Dependency Evaluation
The majority of the dependencies required by postcss-url are already part of Drupal core. These are the dependencies added:
Note that I mention weekly NPM download numbers as a fuzzy gauge of how widely used these packages are. This is obviously a fuzzy figure since a single project can have hundreds of builds per week, each one pulling in the library again, but it can help distinguish wide use vs limited.
postcss-url (PostCSS plugin)

• Maintainership of the package: postcss-url is part of the PostCSS project. PostCSS project itself has multiple maintainers but it seems that only one of them is active in the core. Full list of maintainers. PostCSS repositories are active and PR's seem to receive feedback very quickly. PostCSS is used by at least 2.7 million other packages. PostCSS is also used by some large organizations including Wikipedia, Twitter, Alibaba, and JetBrains.
• Security policies of the package: Policy is part of the main PostCss policy. Security issues can be reported privately using Tidelift. This library is only used in by core developers in development environments.
• Expected release and support cycles:Minor/patch releases as needed. Major release frequency is slowing down - a beta for a new major version (9) was released almost a year ago. Could benefit from a refresh of current dependencies.
• Code quality:Plugin has good test coverage. Is part of the PostCSS project, which has already been vetted for code quality in #3060153: Use PostCSS in core, initially only for Claro

cuint

• Maintainership of the package:The package is maintained by a single maintainer. The maintainer is active on Github, and it is difficult to gauge their activity on this package as it has very few issues overall - 7 total since it was created in 2013. There is one open issue filed a month ago that is something other than a support request: https://github.com/pierrec/js-cuint/issues/11, which may be a misdiagnosis or config issue by the issuer based on the output they pasted in
• Security policies of the package: The package does not have an explicit security policy documented on GitHub or in the codebase
• Expected release and support cycles:No release since Oct 5, 2016. No issues requiring a new release have been reported since then.
• Code quality:Based on the small number of overall issues since creation in 2016 vs the 850k weekly download in NPM , this seems like a stable package. It serves a very specific purpose and has good test coverage.

mime

• Maintainership of the package:The package is actively maintained by a single maintainer. The issue queue is spotless and past issues were addressed promptly.
• Security policies of the package: The package does not have an explicit security policy documented on GitHub or in the codebase
• Expected release and support cycles:Several minor/patch releases a year for the past several years. They appear to be as-needed as opposed to scheduled.
• Code quality: Package serves a specific purpose and has good test coverage. The ratio of weekly npm downloads (27 million -- which suggests very widespread use ) to issues filed suggests this package is quite stable.

xxhashjs

• Maintainership of the package: The package is maintained by a single maintainer. The maintainer is active on Github, and it is difficult to gauge their activity on this package as it has very few issues overall - 15 total since it was created in 2016. Open issues are either feature requests or tech support where the user didn't respond.
• Security policies of the package: The package does not have an explicit security policy documented on GitHub or in the codebase
• Expected release and support cycles: No release since Jun 27, 2016. No issues requiring a new release have been reported since then.
• Code quality:Based on the small number of overall issues since creation in 2016 vs the 850k weekly download in NPM , this seems like a stable package. It serves a very specific purpose and has good test coverage.

This un-inlines all inlined CSS in Claro. The SVGs were also manually optimized with svgo. This optimization could eventually be added to the build process, via something like cssnano , but that doesn't need to weigh down this issue and its already sizeable patch. No interdiff as this is substantially different from the prior patch

Also attached is output from wraith regressions testing, which has found no difference between HEAD and the changes here. Wraith was supplemented with manual testing since it can't account for states like hover and active.

Potentially addressing this one (in claro at least)?

Patch re-rolled for 9.1.x as comment #14.

Thanks for #16, this is not fully rerolled unfortunately. The purpose of this issue is to refactor all inline SVGs in pcss.css files, and one was added in dialog.pcss.css since patch #12

.ui-dialog .ui-icon.ui-icon-closethick {
  width: 100%;
  height: 100%;
  margin: 0;
  transform: translate(-50%, -50%);
  background: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='28px' height='28px' viewBox='0 0 28 28'%3E%3Cpath stroke='%23D4D4D8' stroke-width='1.5' d='M19,9L9,19'/%3E%3Cpath stroke='%23D4D4D8' stroke-width='1.5' d='M19,19L9,9'/%3E%3C/svg%3E%0A") 50% 50% / 100% 100% no-repeat;
}

This needs to reference a file instead of data:image/svg+xml,. If an SVG of this image is not yet in core it will need to be added.

@bnjmnm, thank you for reviewing. here are some points:
1- As you said

one was added in dialog.pcss.css since patch #12

, I could not find that file in patch #12.
2- This one

This needs to reference a file instead of data:image/svg+xml,. If an SVG of this image is not yet in core it will need to be added.

, how to find a file reference or to add svg file if not present.

Please share your feedback.
Thanks again.

I could not find that file in patch #12.

That's because It's not in the patch :) The purpose of this issue is to un-inline all SVGs. Since the creation of patch #12, a new inlined svg was added to the Claro theme in dialog.pcss.css, as seen in the snippet in #17 This inlined SVG needs to be addressed in the same manner as the other ones in this patch.

how to find a file reference or to add svg file if not present.

- Create a new empty .svg file in the folder that matches its hex color.
- Decode the CSS inlined SVG with a utility like https://mothereff.in/url
- Paste the decoded SVG markup into the SVG file you created
- Change background: url("data:image/svg+xml,...: to reference the file you just created, instead of the inlined SVG.
- When in doubt, there are over 80 examples of this process in patch #12, referencing those may help clear up any confusion.

Updated the patch for the file dialog.pcss.css, Please review.

I think the patch in #21 is missing some of the icons.

I'm wondering if we actually want to ship the icons in the core/misc folder or if they should be in the Claro theme directory. I know that historically we have shipped icons there, but I'm wondering if it's a pattern we want to continue. I feel conflicted because it isn't necessarily something we would have to decide as part of this issue, but on the other hand it would be more difficult to make the change after we've added these icons in the core/misc folder because we would have to ensure that none of the images are being referenced elsewhere.

@lauriii is correct, the patch appears to be missing icons. As it's not clear what was undone in #21, this is diffed from #12.

Re #22, I'm partial to continuing the pattern of using core/misc for icons to encourage reuse in contrib. Their location signifies it's available for use with anything that extends core as opposed to a specific theme. This helps facilitate a more consistent UI, and makes it easier for contributors to leverage existing resources instead of manifesting new ones. The point about it being difficult to revert such a change is noted, I'm trying to think of scenarios where this may be wanted - the one that comes to mind is it would be possible remove these images when Claro is removed sometime between Drupal 11 and 99. There may be additional reasons that haven't occurred to me.

Some of the icons in core/misc are being used by contrib but the usage is very limited because contrib cannot properly access the icons in CSS because it's not possible for them to know in which folder the module or theme will be installed. For the icons to be reusable, we probably need something like #1849712: Add theming template and prepare logic for rendering icons first. Main reason for not adding the icons to core/misc is that it will be make it harder to discover icons used by Claro. Also, one of the reasons for the icons to exist in the core/misc is core reusability. This would mostly mean modules which I believe goes against our decision of not having designs in modules.

Good point in #24, with uncertain paths there's not much use in offering the icons to all of core. The icons created in this patch have been moved to Claro.

Based on the thorough evaluation in #11, postcss-url seems like a great solution, both for its maintainership and use of standard syntax as @lauriii mentioned previously. Since core is already using PostCSS, adding this plugin should make for an efficient workflow.

#24 is also a logical approach, in my opinion.

I tested most of the changes manually by using cd_tools. On top of that, I tested manually breadcrumbs and off-canvas title icon. I also tested media library and status report even though their source CSS wasn't changed as part of the patch, just to make sure that pre-existing references to icons work correctly.

1. +++ b/core/package.json
   @@ -57,6 +57,7 @@
   +    "postcss-url": "^8.0.0",
   

   Seems like there's now 9.0.0. I think we should upgrade to that before committing this

2. +++ b/core/themes/claro/css/components/action-link.pcss.css
   @@ -184,203 +184,203 @@
   +    background-image: url(../../images/icons/windowText/hide.svg) !important;
   

   Why do we need to add !important here?

3. We should address the "Needs documentation" tag. I think we could also benefit from a change record.

#27.1

Seems like there's now 9.0.0. I think we should upgrade to that before committing this

. 9.0 is currently a beta that hasn't budged for 16 months. Is there an interest in vetting and adding this anyway to avoid the upgrade hassle later?

#27.2

Why do we need to add !important here?

We don't, looks like it was added by accident during the refactoring - fixed every instance in the file

#27.3

We should address the "Needs documentation" tag. I think we could also benefit from a change record.

Added a CR and updated the documentation here: https://www.drupal.org/docs/core-modules-and-themes/core-themes/claro-th...

Awesome. I didn't notice the postcss-url 9.0.0 was in beta when I checked their latest version. I think we can update to that once they have a supported release. I think this is ready 👍

1. +++ b/core/themes/claro/css/components/action-link.css
   @@ -259,34 +259,34 @@
   -  background-image: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='16' height='16' viewBox='0 0 16 16' stroke-width='2' stroke='%23545560'%3E%3Cpath d='m3 8h10'/%3E%3Cpath d='m8 3v10'/%3E%3C/svg%3E");
   +  background-image: url("data:image/svg+xml,%3Csvg height%3D%2216%22 stroke%3D%22%23545560%22 stroke-width%3D%222%22 width%3D%2216%22 xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%3E%3Cpath d%3D%22M3 8h10M8 3v10%22%2F%3E%3C%2Fsvg%3E");
   

   Why not decode special characters with the optimizeSvgEncode option?

2. +++ b/core/themes/claro/css/components/autocomplete-loading.module.css
   @@ -107,7 +107,7 @@
   -  background-image: url("../../images/spinner-ltr.gif");
   +  background-image: url("data:image/gif;base64,R0lGODlhKAAUAPQSAKbD9f7//OLu/LvO9QBA3ABK3FSO7ABG3x5o5Yis7+fr/FGA5jNr4gFW4FyK6vn//0N96ABQ4AA22WCQ6RtG29Pe+kqE6X2k78DT+enx+t7l+26Z7TZ15aG68pa18x1c4SH/C05FVFNDQVBFMi4wAwEAAAAh+QQEBAD/ACwAAAAAKAAUAEAFgGAgjmRpnmhqNoSiigrRvPRTEUWRuA/tmzEKBAWhtH6piQ7JRD0UiEOBQMUdEIpe0zQ4RGiFw2BLovq0WnIAURiq1YNJQ5J+vwYGlmwytpdsOBE7MAk5BBV1amxuRAUIfkoJNIUTdmY+EhJ2LC5+Pl0Fnj8CBEKiNVBSBJqnrSYhACH5BAUEAAgALAsAAQATABIAAAVKICKKmDMi5qkK6sm0Z9FGMHvAotW+Ou7/JwzCAFSlECxgsqgiEJijgDMARSgIDRS0wtEJq+CWQrQIiyIDKlXJIKTXTMAjQK/b6SEAIfkEBQQADAAsCwABABIAEgAABUogI4rDKBqmGSipibTMwURtMQZmscAMdMiiF4Qn2uWII1oJ2UIxn9BoSiB1PhMFQ4nGLByWzwvlJaIQAMgLi4GzBN7RN1wqr9sDIQAh+QQFBAAAACwLAAEAEgARAAAFOyAgjiTglOMjoOTKii76vOhCATGt7zx9AoqecEjc/YgEBtETCOgghVGz+VoUCDEqa0A4VGYiJ22jeDRDACH5BAUEAAAALAsAAQASABIAAAVCICCK2GieJoOO2epGaHYAkAsVbl6rIpybGMcvpxkaj0jApPNIBp5IxzNg9AGoySOEcmj9FIkI7sgpHHjGhAMzfYYAACH5BAUEAAYALAsAAQASABIAAAVKoCGKwzhippkh6cgIramIsHjIBhWbRCYyuxYLFWyVirELY4ZkBgLI1DNqekKRNap2R9QCqYuCIbvTjAhFgISANiB8wQu7YRhYnyEAIfkEBQQADwAsCwABABIAEgAABUDgI47GKA6mqaQmwj7EU7x0/SyiIDoHXvuPQABlk4mEgaIJqWw6n9BolKjEuaQpRmL1WM1oAEKBEDPpXgNHwxYCACH5BAUEAAAALAwAAQARABIAAAU8ICCOpEGOwakqKRAEE6XOb3DNao2LENnuQKApSCwajYqjcjY4bggLHOQASAIqh0IE+FMgqDPrCTMUlUchACH5BAUEAAYALAsAAQASABIAAAVNoCGKjzOK2HkGLKOKwqOyQSWchUG8RsAbEIpB8eNZDK7iK6JsOmHPqFSKGUgBBGYzl2IUjkXISUGgiI0Uws0g2+VUWvZIVkz+TCP8KQQAIfkEBQQAAAAsCgABABMAEgAABUcgII5B+UjANIxsWwboqLRu8AAzre+iwOsQAOKnKxB5kyMrdwM0lQBJDApoEHLQSSGRBQwORmLEKxoGqcczFk0d8gbJkYEWAgAh+QQFBAACACwKAAEAEwASAAAFX6AgjmNgBmSqCiZGMNmaosEgHnI+xnrvq5MF5idwCBAEjS8WCRwIKKLgKRV9BMperFD1UBaC4a9BAYgYvongOsIZVxNcljVqeHiPTkOEifYoFA0afikAFlwCFBAAJyYhACH5BAUEAAwALAsAAQASABIAAAVgICOOpBiUaBmsZ4qyFtMqlysCBKWICFXbpMGhABRFRIZCojgyNBi7ooDRIDBL1uvoOZVSDdpEwTBgHIuFQxnBWAAhDPboAE8tiF2SMqpIHA8VD0xpCFEoA04iDQ4DLCshACH5BAUEAAAALAsAAgASABEAAAU7YBA8yiaeaPpUBzGksCgQxRKnQF5AN+rlwKAwxyAMj0IHcslsOp9DRU4JrVqRggNlAX1gmQKw90hdhgAAIfkEBQQAAAAsCwABABIAEgAABUIgIIqYk4xoKjJHwakwUESJEo/ZQUF33wejiA8QKDqGoiIQCXh0JsyoVKWZihwYpnAF4EkLXhXvkFFtkeUYQ4pRFkMAIfkEBQQABgAsCwABABIAEgAABUmgIYqD0RDENa5jhoioBLAsMWo0KxjFkueM3w8jLBpzO2NgeVwxm6Kn4qhgqKCjEpb4agYNGVuRYpiODqOk+Sf40hCZIW2wrIcAACH5BAUEAA8ALAsAAQASABIAAAVA4COOZOMMJCmkBFEQQEoWjyIqCSPv/Ig8i14KJSwaj8ik0hhoJpsBEa04cIqCvcXBIVoBlzwaYffj2XbEkUEWAgAh+QQFBAAAACwLAAEAEQASAAAFPCAgjsYIGJg5Kup4IEogB60ZFUclK8cBzcDAgrCpGQfGpHLJWjqfUGgpGqWNfsGsdXmZPSmTYDO5NU2NIQAh+QQFBAAGACwLAAEAEgASAAAFTqAhjs5olGbKpOkzumLEFgZhwAJBWawIUQSFCdIbWQorjIFWHEUIgCZroJRar1ipIMtlybgrXlZIIfYCPRtTJKgE3q0ti/FG96qkR/0dAgA7");
   

   Are *.gif and *.png files inlined here on purpose or should we use a filter to inline only *.svg?

Good points! We should probably set a filter to leave *.gif and *.png files out.

Great feedback in #30. This should address both points.

#32 addresses feedback from #30

Test failure that kicked this back to "Needs Work" was an unrelated CKEditor test known to intermittently fail and is not at all touched by the changes in this patch. Switching back to RTBC.

This removes the hunk from form.css that was made obsolete by #3127466: Required fields are not identifiable on Internet Explorer 11 high contrast.

Adding reviewer credits.

Some of the files in this patch don't pass the spell checking that runs in our pre-commit hook:

From the core directory:

> yarn run -s spellcheck themes/claro/css/components/messages.pcss.css
messages.pcss.css:78:45 - Unknown word (crossout)

> yarn run -s spellcheck themes/claro/images/icons/004adc/spinner.svg
spinner.svg:1:92 - Unknown word (keyframes)
spinner.svg:1:208 - Unknown word (dashoffset)
spinner.svg:1:338 - Unknown word (dashoffset)
spinner.svg:1:364 - Unknown word (dasharray)

> yarn run -s spellcheck themes/claro/images/icons/004adc/spinner-rtl.svg
spinner-rtl.svg:1:92 - Unknown word (keyframes)
spinner-rtl.svg:1:209 - Unknown word (dashoffset)
spinner-rtl.svg:1:340 - Unknown word (dashoffset)
spinner-rtl.svg:1:366 - Unknown word (dasharray)

Should we add these to misc/cspell/dictionary.txt or do something else?

Added those terms to core/misc/cspell/dictionary.txt and tested that the spellcheck on these files pass now.

Thanks! Pushed to 9.1.x. I agree with the reasoning in #11 that the new JS dependencies are reasonable to add, considering that they're only dev dependencies.

publish change record