Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

escape-svg fails for unquoted URLs #30835

Closed
jrdutton opened this issue May 14, 2020 · 4 comments · Fixed by #31653
Closed

escape-svg fails for unquoted URLs #30835

jrdutton opened this issue May 14, 2020 · 4 comments · Fixed by #31653

Comments

@jrdutton
Copy link

jrdutton commented May 14, 2020

When using the SCSS version of Bootstrap, I am experiencing problems when providing an unquoted URL for the $breadcrumb-divider variable.

This is because the escape-svg function expects the URL to be quoted. Therefore, it removes the first five url(' and last two characters '). However, in the case of an unquoted URL, this fails. My understanding is the unquoted URLs are valid SCSS for the url function.

File poc.scss.txt shows the escape-svg function and other helper functions taken from the Bootstrap SCSS.
It also includes two other variations, my attempts at fixing the issue.
poc.scss.txt

File poc.css shows the output when compiled.
poc.css.txt
.breadcrumb-item shows the CSS using the existing function. In the case of url(data, the data part is truncated to ata
.breadcrumb-item2 and breadcrumb-item3 show the outputs with the adjusted function.

I am happy to create a pull request if agreed that this is an issue to be fixed.

@XhmikosR XhmikosR added the css label May 15, 2020
@XhmikosR
Copy link
Member

/CC @twbs/css-review (also please add v4/v5 labels where this applies)

@ffoodd
Copy link
Member

ffoodd commented May 29, 2020

So, I'm able to reproduce this on both v5 and v4.

Please have a look at a dedicated SassMeister. I took @jrdutton examples and added one of our own SVGs.

FWIW, this function expects quoted URL since SVG as data URi won't work without quotes (simply remove the one in $breadcrumb-divider4 to see what happens). Your case is somehow invalid since you're using base64—which Bootstrap doesn't.

@jrdutton in fact, if you're using base64 encoded SVGs you don't need to use escape-svg() function at all since it's meant to URL-encode datas. Please have a look at Taylor Hunt's excellent post "Optimizing SVGs in data URIs".

That being said, using the second suggestion —replacing str-slice($string, 6, -3) with str-slice($string, 5, -2)—shouldn't harm, except for people using unquoted URL-encoded SVG data URis.

Two things to consider then:

  1. using this (small) improvement to allow unquoted base64 usage in customization
  2. improving our docs to make things obvious, whatever we decide with 1.

@mdo @MartijnCuppens any thought?

@jrdutton
Copy link
Author

Thank you @ffoodd for looking at this. I will take a look at the articles you suggest.

@mdo
Copy link
Member

mdo commented Sep 14, 2020

Opened #31653 to leave a comment in the source code. Don't think we need too much more here for now so we don't risk breaking behaviors for others.

mdo added a commit that referenced this issue Sep 14, 2020
 Closes #30835 by leaving a comment in the source that the escape-svg() function must have quotes around data URIs.
XhmikosR pushed a commit that referenced this issue Sep 15, 2020
 Closes #30835 by leaving a comment in the source that the escape-svg() function must have quotes around data URIs.
XhmikosR pushed a commit that referenced this issue Sep 16, 2020
 Closes #30835 by leaving a comment in the source that the escape-svg() function must have quotes around data URIs.

(cherry picked from commit 849fea5)
XhmikosR pushed a commit that referenced this issue Sep 18, 2020
 Closes #30835 by leaving a comment in the source that the escape-svg() function must have quotes around data URIs.

(cherry picked from commit 849fea5)
XhmikosR pushed a commit that referenced this issue Sep 21, 2020
 Closes #30835 by leaving a comment in the source that the escape-svg() function must have quotes around data URIs.

(cherry picked from commit 849fea5)
XhmikosR pushed a commit that referenced this issue Sep 21, 2020
 Closes #30835 by leaving a comment in the source that the escape-svg() function must have quotes around data URIs.

(cherry picked from commit 849fea5)
olsza pushed a commit to olsza/bootstrap that referenced this issue Oct 3, 2020
 Closes twbs#30835 by leaving a comment in the source that the escape-svg() function must have quotes around data URIs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants