Support rect property and nested render arrays in html_tag for dynamic SVGs

Worked on this a bit for #2660124: Dynamically build layout icons based on well formed config. I added a render element for the outer < svg > and for a < g > and a theme function for a < rect >

This is still alpha-level but would love to get some input on this.

This is a great start! Not really my area of expertise, but it works great.

This is awesome, and enables work I started and @timplunkett continued in #2660124: Dynamically build layout icons based on well formed config.

Some review of the alpha patch:

1. +++ b/core/includes/theme.inc
   @@ -1708,6 +1708,84 @@ function _field_multiple_value_form_sort_helper($a, $b) {
   +  $variables['height'] = $element['#height'];
   

   If we have attributes as variables like width and height, setting width and height as a part of #attributes would override those variables.

   I would remove every template variable that could be represented as a part of the attributes array and have defaults for width and height instead.

2. +++ b/core/includes/theme.inc
   @@ -1708,6 +1708,84 @@ function _field_multiple_value_form_sort_helper($a, $b) {
   + * Prepares variables for SVG group templates.
   + *
   + * Default template: svg-g.html.twig
   + *
   + * @param array $variables
   + *   An associative array containing:
   + *   - width:
   + *   - height:
   + *   - attributes:
   + *   - children:
   

   Width and height are not used in the template for svg_g.

3. + * Default template: svg-g.html.twig
   + *
   + * @param array $variables
   + *   An associative array containing:
   + *   - width:
   + *   - height:
   + *   - attributes:
   + *   - children:
   + */
   +function template_preprocess_svg_rect(array &$variables) {
   

   s/svg-g/svg-rect

4. +++ b/core/includes/theme.inc
   @@ -1708,6 +1708,84 @@ function _field_multiple_value_form_sort_helper($a, $b) {
   + * @param array $variables
   + *   An associative array containing:
   + *   - width:
   + *   - height:
   + *   - attributes:
   + *   - children:
   + */
   +function template_preprocess_svg_rect(array &$variables) {
   

   Missing x, y, stroke_width, stroke, and fill.

   I also have the same note about having attribute variables regarding x/y as above.

5. +++ b/core/includes/theme.inc
   @@ -1708,6 +1708,84 @@ function _field_multiple_value_form_sort_helper($a, $b) {
   +  $variables['attributes']['style'] = implode(';', $styles);
   

   Dynamic inline styles aren't super common in Drupal - are there any security concerns with inlining Javascript here?

Currently working through this at DrupalCon

Currently working through this at DrupalCon

Here's an updated patch. I'm not quite sure it's "complete" but at least is a step towards the right direction.

For some reason the patch didn't get attached on that previous message. Attempting again.

Removing mistakenly added file in previous commit.

Fixing tags

Funny how I was just about to start a sandbox for rendering SVG elements and then I bump into this issue. Some great progress here!

A few points:

1. As mentioned before in #8.1, lets not use any special handling for a select few attributes. The current #image suffers from this implementation, for example setting when setting both a #width and #attributes['width'], the #attributes['width'] is overwritten.
2. The patch in #14 uses a Render+theme implementation for svg and g, but not rect. I would like to suggest using a Render (#type) implementation for every element, since in svg every element can have a non-graphical child such as <title>
3. Svg presentational attributes are preferred instead of inline styles, since presentational attributes can be overridden using external css. See the svg styling cascade overview at https://sarasoueidan.com/blog/svg-style-inheritance-and-FOUSVG/ For example, preferably use <rect fill="blue" /> instead of <rect style="fill: blue;" />
4. Let's add in support for the svg use element out of the box as well, since this is used as a best practice for implementing an svg icon system. For reference see this example: https://css-tricks.com/svg-sprites-use-better-icon-fonts/

@WidgetsBurritos Please let me know if you are interested in finishing this patch. I would be happy to help with reviews; or I could assist with coding the patch.

@idebr I'm cool doing either. I probably won't be on this for the next week or so as I'm gonna be catching up on work from when I was at DrupalCon, so if you wanna continue from where I stopped that's fine by me. Otherwise I'm happy to jump back on it as soon as I get some downtime.

Awesome to see progress on this. My patch really was just a proof of concept for something I was playing with, please feel free to build on it, rip it apart, etc. #8 and #18 are all valid to me. In general it seems that at least @idebr and presumably @samuel.mortenson and @WidgetsBurritos as well know a lot more about SVG than me and I would love to see this being pushed forward.

Thank you all!

Attached patch implements the following Render elements:

• Svg for <svg>
• SvgG for <g>
• SvgRect for <rect>
• SvgUse for <use>

The interdiff addresses the following feedback:

1. #8.1, #8.2, #8.4 #18.1, #18.2: All svg attributes now generated through twig attributes without any special handling for width or any other presentational attribute.
2. #8.5, #18.3: Removed the inline styles in favor of svg presentational attributes, since presentational attributes can be overridden with external css.
3. #8.3: Updated the template file docblock with the correct element references.
4. #18.4: Implemented support for
   through the SvgUse Render element.

I still need to pull this down and test it, but at first glance:

1. +++ b/core/modules/system/templates/svg-g.html.twig
   @@ -0,0 +1,22 @@
   +<svg width="32" height="32" xmlns="http://www.w3.org/2000/svg">
   +  <rect x="8" y="8" width="16" height="16"></rect>
   +  <use xlink:href="#configure" xmlns:xlink="http://www.w3.org/1999/xlink"></use>
   +  <g><rect x="0" y="0" width="16" height="16"><title>Rectangle</title></rect>
   +  </g>
   +
   +</svg>
   

   Was this supposed to be here? I'm assuming that's some test code that never got removed?

2. +++ b/core/themes/stable/templates/misc/svg-g.html.twig
   @@ -0,0 +1,22 @@
   +<svg width="32" height="32" xmlns="http://www.w3.org/2000/svg">
   +  <rect x="8" y="8" width="16" height="16"></rect>
   +  <use xlink:href="#configure" xmlns:xlink="http://www.w3.org/1999/xlink"></use>
   +  <g><rect x="0" y="0" width="16" height="16"><title>Rectangle</title></rect>
   +  </g>
   +
   +</svg>
   

   Same here.

This seems to be working well and I definitely think it makes sense to just let the form api handle all the attributes. Based on my previous feedback in 22.1 and 22.2, here is an updated patch.

@WidgetsBurritos Ah yes, that was the code I used for the render element example code. Thanks for the updated patch!

@idebr Not a problem.

Assuming others are okay with this implementation strategy, I guess from here we have a couple questions to answer:

1. Per @metzlerd's original question, which SVG tags do we need to provide support for?
Here's a nice long list: https://developer.mozilla.org/en-US/docs/Web/SVG/Element

Is it something we want to start with a small group of tags and then just let people add them as they need to (maybe someone could create a separate "SVG extended" contrib module as needed), or do we try to provide support for as many out of the box as possible?

We're just talking about a lot of tags, so I'd imagine before contributing that many tags to core it should at least be discussed.

2. Also, should there be some sort of automated testing around the form rendering as well?

It may very well be worth confirming that something like this actually produces the desired output:

$form['icon'] = [
  '#type' => 'svg',
  '#attributes' => [
    'width' => 32,
    'height' => 32,
  ],
];
$form['icon']['rect'] = [
  '#type' => 'svg_rect',
  '#attributes' => [
    'x' => 8,
    'y' => 8,
    'width' => 16,
    'height' => 16,
  ],
];

Following up on #25.1, I wonder if the html_tag element, which is already in core, would provide all the needed functionality that the patches so far provide. Looking into \Drupal\Core\Render\Element\HtmlTag, I think the only thing we would need to add is a "#children" option, or edit '#value' to support nested render arrays.

Here's an example of how this could look:

$svg = [
  '#type' => 'html_tag',
  '#tag' => 'svg',
  '#attributes' => [
    'width' => 100,
    'height' => 100,
  ],
  // We would need to patch core to add this, today you would need to use #value and pass a rendered Markup element.
  '#children' => [
    [
      '#type' => 'html_tag',
      '#tag' => 'circle',
      '#attributes' => [
        'cx' => 50,
        'cy' => 50,
        'r' => 40,
      ],
    ],
  ],
];

In regards to #26, it appears the original use case of this issue had been modified to address other use cases along the way. I didn't realize that initially, as I didn't read that close enough. I'm not entirely sure the best approach to solve your specific use case, but I do think it's a valid concern. In regards to your comment about this approach being more of a module, than as part of core, I believe part of the emphasis is that it's a prerequisite for #2660124: Dynamically build layout icons based on well formed config, which is attributed to the Layout Initiative.

In regards to #27, I think that is a very valid point. My concern with trying to implement every possible tag up front, is that it's only valid until the spec changes. And while it's not extremely frequent that happens, it's still something the community would be responsible for maintaining. By taking the generic #html_tag approach, it's as scaleable as we need it to be.

@samuel.mortenson I've taken an initial stab at #27.

May need a little bit of work, but it does seem to work from a POC stand point anyway. Thoughts?

FYI: Since this is a brand new approach, I didn't include an interdiff.

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -92,7 +97,15 @@ public static function preRenderHtmlTag($element) {
+          $markup .= self::preRenderHtmlTag($child)['#markup'];

This assumes that each child contains #markup, which may not be the case for various render array elements.

I think we would need to handle children in the Twig template by calling {{ children }} if children was set.

Re #27

Following up on #25.1, I wonder if the html_tag element, which is already in core, would provide all the needed functionality that the patches so far provide.

There is a plan to remove #type => html_tag in #2544318: Remove #type => html_tag usages, so this implementation might be counterproductive for that issue.

@idebr From reading that issue, I think the plan is to remove the core usages of html_tag, not the render element itself. On my Drupal 8 site ten of the contrib modules I use rely on html_tag, so I doubt it will go away completely at any point before Drupal 9.

Re #33

@idebr From reading that issue, I think the plan is to remove the core usages of html_tag, not the render element itself. On my Drupal 8 site ten of the contrib modules I use rely on html_tag, so I doubt it will go away completely at any point before Drupal 9.

From the proposed resolution:

remove the 6 places where non-head-type tags are using #type => 'html_tag'
and
Remove \Drupal\Core\Render\Element\HtmlTag

and subsequently in #12:

Before we can remove the element type, we need to remove the usages and deprecate it. So moving this back to 8.x for that.

The issue summary you're quoting is two years old: https://www.drupal.org/node/2544318/revisions/view/8741274/8745098, and #12 is still talking about removing usages in 8.x and removing the render element in 9.x. Removing the render element in Drupal 8 would constitute a BC-breaking API change.

I updated #2544318: Remove #type => html_tag usages with links to relevant contributed module code, and a reference to this issue to try and make a case not to remove the html_tag element in Drupal 9.

Just now getting caught up on this issue. When I get a chance, I will work through the recommendation from #30 under the assumption that #2544318: Remove #type => html_tag usages is not an actively pursued priority. If that issues changes, obviously this strategy may change as well, but I'll try to jump on this sometime this weekend, unless someone else wants to beat me to the punch.

Okay, so here is an attempt to accomplish @samuel.mortenson's recommendation from #30. It all seems to work locally, but I admittedly haven't thrown the gauntlet at it yet.

Here is a quick explanation of what I've done.
- There was currently no template for the generic 'html_tag' type, so I created one.
- Almost all of the markup that was generated in HtmlTag::preRenderHtmlTag() got moved into html-tag.html.twig.
- Only thing remaining in HtmlTag::preRenderHtmlTag() is the check whether or not we have a void element.
- Because HtmlTag::preRenderHtmlTag() is no longer rendering markup, this means tests related to markup generation had to be removed. Instead, we're only testing whether or not void elements get marked correctly.
- Because of the previous item, I'm not entirely sure what the best way to test for rendered twig markup is but if someone can give me some direction there, I'll be happy to implement.
- This change isn't without it's own issues. A bunch of other tests fail. I'm still looking into those, but really the point of this patch was to make sure this is the right direction, not to get everything perfect quite yet.

Once again, treating this as a brand new approach, so no interdiff supplied for this patch.

Cleaned up the undefined index exceptions. Not sure if this is all of the errors but should significantly shorten the list anyway.

I'm seeing this upon applying the patch, which essentially breaks the site:
@import url("/core/themes/bartik/css/components/comments.css?oqeccj");

This would be fixed by rendering the {{ value|raw }}, but this would be opening the door to attacks, so that's not a good way to fix it.

I'm thinking we should treat the @import statements differently, perhaps a new render element that would print this?
@import url("{{ value }}")

Though this change does not fix the above issue, the attached patch removes the escaping from the tag on template_preprocess_html_tag, since twig does this for us.

Manuel,

Thanks for that. I'm definitely seeing this issue as well so thanks for the confirmation. I had tested using the raw filter locally as a proof of concept and when I did that it caused other stuff to fail, and given the security implications I didn't dive too deeply into that.

That's a good approach to solve the imports there. I'll look into it, or if you wanna take a shot at it, as it may be a few days until I can jump back on this.

@WidgetsBurritos no problem, about my suggestion: I had a go at it for a while, and the main hurdle would be modifying core/lib/Drupal/Core/Asset/CssCollectionRenderer.php which is a complex beast. All to avoid escaping those quotes... there has to be a better approach.

Here is what i did to follow the route of rendering the import statements on their own. Seems to work for CSS files, but JS is broken on the site for some reason.

Again, this may not be the best way to resolve the double escaping explained on #42...

Great. Thank you. I'll check it out when I get a chance.

@Manuel,

Had a brief moment to checkout your updates. In addition to the imported style issues, it seems JSON rendering is a bit messed up here too , and is one of the main reasons why so many tests are currently failing (i.e. Exception: No #ajax path specified. failures)

For example:

<script type="application/json" data-drupal-selector="drupal-settings-json">

    {&quot;path&quot;:{&quot;baseUrl&quot;:&quot;\/&quot;,&quot;scriptPath&quot;:null,&quot;pathPrefix&quot;:&quot;&quot;,&quot;currentPath&quot;:&quot;user\/1&quot;,&quot;currentPathIsAdmin&quot;:false,&quot;isFront&quot;:false,&quot;currentLanguage&quot;:&quot;en&quot;,&quot;currentQuery&quot;:{&quot;foo&quot;:&quot;bar&quot;,&quot;q&quot;:&quot;user\/1&quot;}},&quot;pluralDelimiter&quot;:&quot;\u0003&quot;,&quot;ajaxPageState&quot;:{&quot;libraries&quot;:&quot;classy\/base,classy\/messages,core\/drupal.active-link,core\/drupal.ajax,core\/html5shiv,core\/jquery.form,core\/normalize,system\/base&quot;,&quot;theme&quot;:&quot;classy&quot;,&quot;theme_token&quot;:null},&quot;ajaxTrustedUrl&quot;:{&quot;form_action_p_pvdeGsVG5zNF_XLGPTvYSKCf43t8qZYSwcfZl2uzM&quot;:true,&quot;\/user\/1?foo=bar\u0026q=user\/1\u0026ajax_form=1&quot;:true},&quot;ajax&quot;:{&quot;edit-test1&quot;:{&quot;callback&quot;:&quot;::updateOptions&quot;,&quot;wrapper&quot;:&quot;edit-test1-wrapper&quot;,&quot;event&quot;:&quot;change&quot;,&quot;url&quot;:&quot;\/user\/1?foo=bar\u0026q=user\/1\u0026ajax_form=1&quot;,&quot;dialogType&quot;:&quot;ajax&quot;,&quot;submit&quot;:{&quot;_triggering_element_name&quot;:&quot;test1&quot;}}},&quot;user&quot;:{&quot;uid&quot;:&quot;1&quot;,&quot;permissionsHash&quot;:&quot;c7f3a715d51582d5dddd4a694aac3bba8073765b1fbfca949db5705a86accaee&quot;}}
    
  </script>

This issue is starting to feel a little bit like a rabbit hole...

Here's a simplified take on allowing nested elements for the html_tag element. I basically moved the tag to the prefix/suffix keys, instead of doing markup juggling in the preRenderHtmlTag method. This allows for nested render arrays without any explicit code for SVG (besides the related $voidElements I added), which I think is fairly clean. With this patch, you can render an SVG element using a render array like:

$element = [
  '#type' => 'html_tag',
  '#tag' => 'svg',
  '#attributes' => [
    'width' => 150,
    'height' => 150,
  ],
  [
    '#type' => 'html_tag',
    '#tag' => 'rect',
    '#attributes' => [
      'width' => 50,
      'height' => 50,
      'style' => 'fill:rgb(0,0,255)',
    ],
  ],
];

which outputs

<svg width="150" height="150"><rect width="50" height="50" style="fill:rgb(0,0,255)" /></svg>

(notice how rect auto closes)

I think this is preferable to maintaining Twig templates and theme hooks for every possible SVG element, of which in 1.1 are many.

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -84,21 +85,27 @@ public static function preRenderHtmlTag($element) {
+      $prefix .= " />\n";

I realize it isn't a concern of this issue, but why are void elements being given a closing slash when Drupal 8 is using HTML5? XHTML backward compatibility for those who chose to implement it in a custom theme?

I see that the closing slash has been taken up on other issues, namely #1388926: Remove all references to "self-closing" void elements in core

@samuel.mortenson #50 is definitely working for me. You can even abuse the hell out of it with nested and different tag types:

For example:

$form['html_tag'] = [
    '#type' => 'html_tag',
    '#tag' => 'form',
    '#attributes' => [
      'class' => 'xyz',
    ],
    [
      [
        '#type' => 'password',
        '#title' => 'your password',
        '#size' => 25,
      ],
      [
        '#type' => 'textfield',
        '#title' => 'some text field',
      ],
      [
        '#type' => 'html_tag',
        '#tag' => 'div',
        '#attributes' => [
          'class' => 'abc',
        ],
        [
          [
            '#type' => 'html_tag',
            '#tag' => 'svg',
            '#attributes' => [
              'width' => 100,
              'height' => 100,
              'x' => 20,
              'y' => 30,
            ],
            [
              [
                '#type' => 'html_tag',
                '#tag' => 'circle',
                '#attributes' => [
                  'cx' => 50,
                  'cy' => 50,
                  'r' => 4,
                ],
              ],
              [
                '#type' => 'html_tag',
                '#tag' => 'rect',
                '#attributes' => [
                  'width' => 25,
                  'height' => 25,
                  'x' => 5,
                  'y' => 10,
                ],
              ],
            ],
          ],
        ]
      ],
    ],
  ];

^ Now obviously some of that isn't a good idea in practice, but from a conceptual standpoint, I like it.

And only 2 tests to resolve instead of 65, seems a bit easier to swallow.

This should resolve the remaining errors.

@bighappyface No idea on #52, but I think it's for backwards compatibility as you mentioned.

I have tested this locally and it seems to work great.

+1 RTBC

The patch itself looks OK (didn't do a full review) but this could really use a change record (to explain how to use the new feature) and an issue summary update (since the title is completely different from the solution, changing the title now though).

Updated the issue summary, @catch should a core contributor write a change record or can anyone do it? If I should write one at https://www.drupal.org/node/add/changenotice?field_project=3060 I will.

Sam,
Anyone could write the CR

//Naveen

Filed https://www.drupal.org/node/2887146, moving back to needs review.

+1 CR looks good to me

Absolutely love this new approach @samuel.mortenson, brilliant!!
+1 to RTBC

Looking good, this will be very useful.

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -41,6 +41,7 @@ class HtmlTag extends RenderElement {
+    'rect', 'circle', 'polygon', 'ellipse', 'stop', 'use', 'path',

There is no test coverage for these new elements.

If we want to ensure they're not accidentally lost, I think we need some.

Especially given the work that's gone into it so far.

Good call @larowlan, having a go at this.

Added coverage for all the added void elements.

I also thought about adding coverage for rendering a linearGradient element, which has nested elements, so as to make sure we can actually do this, but it's failing on me...

Had another look at my failing test, looks like the current patch actually does not support nesting html_tags within html_tags, just #markup elements?

My test oputs:

--- Expected
+++ Actual
@@ @@
-'<linearGradient><stop offset="5%" stop-color="#F60" /><stop offset="95%" stop-color="#FF6" /></linearGradient>
+'<linearGradient></linearGradient>
 '

I've even tried the example given on #50, with the same results. Looks like the failing test is actually finding a bug in the current patch.

@Manuel Garcia,

Sorry I'm writing this from my phone on the road so I can't test it myself, but does it have anything to do with the '#value' => NULL, in your test? Or maybe the lack of a type in that same section?

Thanks @WidgetsBurritos for replying, that's not it though.. we have another test that passes just fine for:

   $element = [
      '#tag' => 'p',
      '#value' => NULL,
      [
        ['#markup' => '<b>value1</b>'],
        ['#markup' => '<b>value2</b>'],
      ],
    ];
    $tags[] = [$element, "<p><b>value1</b><b>value2</b></p>\n"];

If I omit the #value => NULL, on the parent element I get error: Undefined index: #value. If I add it to the nested elements it has no effect.

Having the #type seems to have no effect either, so we could perhaps omit it from the nested elements as well in a future patch... once we figure out why this fails ;)

@manuel-garcia I didn't realize you could omit #type for this kind of element, cool! I'll update the change notice as well. From manually testing this on a standard profile install, nested html_tag elements render just fine. It's only in tests that I'm seeing failures. Not sure what that means yet...

Ah - since we're nesting html_tag elements we'll need to run the pre-render on everything, I've updated the test and things are passing locally. The newlines appended to void elements are a bit awkward, but we shouldn't change those as that may negatively effect existing html_tag users.

Ah perfect @samuel.mortenson that makes sense, thanks!

#63 has been addressed and we've expanded the coverage a bit more. So back to RTBC (assuming test comes back green).

Removing tag added in #49, new approach doesn't need it =)

+++ b/core/tests/Drupal/Tests/Core/Render/Element/HtmlTagTest.php
@@ -102,6 +102,103 @@ public function providerPreRenderHtmlTag() {
     $tags[] = [$element, "<p><b>value1</b><b>value2</b></p>\n"];
...
+    $tags[] = [$element, '<rect width="25" height="25" x="5" y="10" />' . "\n"];
...
+    $tags[] = [$element, '<circle cx="100" cy="100" r="100" />' . "\n"];
...
+    $tags[] = [$element, '<polygon points="60,20 100,40 100,80 60,100 20,80 20,40" />' . "\n"];

Can we get a follow-up to name these test cases.

i.e. each test case has a key in the array, instead of being numeric.

+++ b/core/tests/Drupal/Tests/Core/Render/Element/HtmlTagTest.php
@@ -92,6 +96,112 @@ public function providerPreRenderHtmlTag() {
+    $tags[] = [$element, '<circle cx="100" cy="100" r="100" />' . "\n"];

You mean like making this $tags['circle'] = [$element, '<circle cx="100" cy="100" r="100" />' . "\n"]; instead? Should be very doable in this patch I think.

Let's do it then.

Actually, after going for it, I realized why a followup makes more sense... the existing test cases need to have their array keys as well, so it makes sense to do these all in one go after this patch gets in.

Created #2889378: Name test cases on HtmlTagTest as a follow up for this purpose.

+1 on resolving in a separate issue.

Committed/pushed to 8.4.x, thanks!

Agreed with adding the key names to the test cases in a follow-up.

Awesome, thanks all!

Just published the CR

imho :)

This issue caused some regression #2563069-12: Unclosed conditional comments in html tag result in broken html head.

So sad we cannot do the same with Drupal 7.

I opened an issue about this: #2981726: Backport D8 html_tag element changes