Problem/Motivation

In #3218479: Site logo image is missing width and height we added logo width and height support for regular image. But often our logo is SVG image and that code will not work for SVG logo image case.

Let's try to cover this case. For sure SVG image is without dimension, it is a vector. But we can try to extract width and height dimensions from either width and height SVG attributes or viewBox attribute.

CommentFileSizeAuthor
#5 3389127-svg-logo-width-height-5.patch2.12 KBpivica
#5 interdiff-3389127-4-5.txt1.29 KBpivica
#4 3389127-svg-logo-width-height-4.patch1.83 KBpivica
#4 interdiff-3389127-2-4.txt1.43 KBpivica
#2 3389127-svg-logo-width-height-2.patch1.78 KBpivica

Comments

pivica created an issue. See original summary.

pivica’s picture

pivica commented
Status: Active » Needs review
StatusFileSize
new 3389127-svg-logo-width-height-2.patch1.78 KB

Here is a patch. Note that for this patch to work you first need a patch from #3344954: Logo accessibility problems, I guess it is a time for a new release ;)

This also needs a patch from #3389133: Add service for extraction dimensions from SVG image.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
  1. +++ b/themes/bs_bootstrap/bs_bootstrap.theme
@@ -382,9 +381,25 @@ function bs_bootstrap_preprocess_block(&$variables) {
+          if (file_exists(DRUPAL_ROOT . $variables['site_logo'])) {
+            $svg_content = file_get_contents(DRUPAL_ROOT . $variables['site_logo']);

    this assumes that the logo is svg, should we check the file extension? 

  2. +++ b/themes/bs_bootstrap/bs_bootstrap.theme
@@ -382,9 +381,25 @@ function bs_bootstrap_preprocess_block(&$variables) {
+              /** @var \Drupal\bs_lib\SvgTools $svg_tools */
+              $svg_tools = \Drupal::service('bs_lib.svg_tools');

    I think it maks sense to do a getContainer()->hasService(), because if someone for some reason didn't update the module, it's going to be very hard to recover from this. 

  3. +++ b/themes/bs_bootstrap/bs_bootstrap.theme
@@ -382,9 +381,25 @@ function bs_bootstrap_preprocess_block(&$variables) {
+              if (isset($dimensions['width'])) {

    I assume we don't want to set only width or height, so we should make sure we either return both or nothing and then you can just do if ($dimensions).

pivica’s picture

pivica commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-3389127-2-4.txt1.43 KB
new 3389127-svg-logo-width-height-4.patch1.83 KB

> 1. this assumes that the logo is svg, should we check the file extension?

381: if ($logo_path_info['extension'] === 'svg') {

This we are already doing here, right?

2. and 3. should be done, needed to do a new patch for related service in bs_lib https://www.drupal.org/project/bs_lib/issues/3389133#comment-15253540.

pivica’s picture

pivica commented
StatusFileSize
new interdiff-3389127-4-5.txt1.29 KB
new 3389127-svg-logo-width-height-5.patch2.12 KB

Removed one extra if and added rounding to int.

  • pivica committed 9af4863f on 8.x-1.x 
    Issue #3389127 by pivica, Berdir: SVG logo is missing width and height
pivica’s picture

pivica commented
Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.