Problem/Motivation

The CSS reset renders SVGs as invisible.

Proposed resolution

Exclude them from the reset.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#4 interdiff-2907420-3-4.txt577 bytesDyanneNova
#4 2907420-4.patch488 bytesDyanneNova
#3 2907420-3.patch492 bytestedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented 
diff --git a/core/modules/settings_tray/css/off-canvas.reset.css b/core/modules/settings_tray/css/off-canvas.reset.css
index 573d8c7..f047056 100644
--- a/core/modules/settings_tray/css/off-canvas.reset.css
+++ b/core/modules/settings_tray/css/off-canvas.reset.css
@@ -12,7 +12,8 @@
 #drupal-off-canvas *:not(div),
 #drupal-off-canvas *:after,
 #drupal-off-canvas *:before {
-  all: initial;
+  /* @todo This breaks SVGs. */
+  /*all: initial;*/
   box-sizing: border-box;
   text-shadow: none;
   -webkit-font-smoothing: antialiased;
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Active » Needs review
FileSize
2907420-3.patch492 bytes

This fixes it for me.

UPDATE: nope I was wrong. Fixes the svg issue but causes others.

DyanneNova’s picture

DyanneNova
she/her
CreditAttribution: DyanneNova at Acquia commented
FileSize
2907420-4.patch488 bytes
interdiff-2907420-3-4.txt577 bytes

This removes the reset on anything inside the svg element.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Reviewed & tested by the community

@DyanneNova thanks!

I tested this manually and it works and does not introduce the other problems that my patch in #3 caused.

If this gets committed before #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it then it will need to be added to that patch.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer commented

@tedbow could you document the steps you used for testing this manually?

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

@lauriii I am using this test module https://github.com/tedbow/D8-Tester

It has a configuration form that adds circle using <svg>

Without the patch the circle is not visible, with the patch in #4 it is.

I tried to write test to prove this using similar code as the test module but <svg> and the <circle> both show up as isVisible() returns true regardless of weather the patch is applied. So I am not sure this something we can test for.

  • lauriii committed 2c77fff on 8.5.x 
    Issue #2907420 by DyanneNova, tedbow: The Off-Canvas CSS reset prevents...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer commented
Version: 8.5.x-dev » 8.4.x-dev

I tested this manually using the module linked on #7. Thank you @tedbow!

Committed 2c77fff and pushed to 8.5.x. Thanks!

Since this only affects settings tray which is currently in beta, we most likely would like to backport this to 8.4.x branch as well. I will ask feedback on that from another committer.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2907420-4.patch, failed testing. View results

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Reviewed & tested by the community

@lauriii thanks for committing!

Failed because against 8.5.x. Testing against 8.4.x. Switching back to RTBC

  • xjm committed 0d82b52 on 8.4.x authored by lauriii 
    Issue #2907420 by DyanneNova, tedbow: The Off-Canvas CSS reset prevents...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Oops, looks like missed the request for a backport check. This looks patch-safe to me; it's a straightforward bug, it doesn't touch the public "API" CSS, and after all Settings Tray itself is still beta.

So I cherry-picked this to 8.4.x as well. Thanks!

Status: Fixed » Closed (fixed)

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