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

fix(build): use base64 for inline SVG if it contains both single and double quotes #15271

Merged
merged 15 commits into from
Dec 15, 2023

Conversation

chaejunlee
Copy link
Contributor

@chaejunlee chaejunlee commented Dec 7, 2023

Description

fixes #15257

After series of experimentation, just escaping single-quote with encoded strings doesn't work (i.e. ' -> %27). I suspect that after HTML URL Decoding, it is again treated as single-quote, causing error again. So I tried to solve it by detecting attribute values with inner single-quote and changing the double-quote to %22. Then I changed the single-quote to %27.

In other words, normal attributes like "......" will be changed to '......'. Attributes like "...'...''..." will be changed to %22...%27...%27%27...%22. %22...%27...%27%27...%22 will be decoded and work as "...'...''...".

Added a test as well.

This problem happens because the svg is being inlined.

However, it looks like just setting assetsInlineLimit: 0 makes the svgs to be imported which makes the problem go away.

export default defineConfig({
  build: {
    assetsInlineLimit: 0,
  },
})

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Dec 7, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@chaejunlee chaejunlee changed the title Fix: escape single-quote within attribute values of SVG URI fix: escape single-quote within attribute values of SVG URI Dec 7, 2023
@chaejunlee chaejunlee changed the title fix: escape single-quote within attribute values of SVG URI fix(svg): escape single-quote within attribute values of SVG URI Dec 7, 2023
@chaejunlee chaejunlee requested a review from bluwy December 7, 2023 11:05
bluwy
bluwy previously approved these changes Dec 7, 2023
@chaejunlee
Copy link
Contributor Author

I am getting this error on node-18, ubuntu.

I am not sure if this is because of my fix or this ci is being flaky.

Screenshot 2023-12-12 at 01 31 13

@patak-dev
Copy link
Member

@chaejunlee that is flaky test, sorry about that. You can ignore it.

@chaejunlee
Copy link
Contributor Author

The ci is passing now but I had this error at the 7ca9ce3 .

Screenshot 2023-12-12 at 01 59 47

If these are not one of the flaky ci tests, I'll look into fixing them.

@sapphi-red sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Dec 12, 2023
@chaejunlee
Copy link
Contributor Author

Because my fix kept failing on the ci, I thought my branch was behind the updates. Out of habit, I rebased and force pushed instead of merging upstream. Now the commit log looks a little weird. I should've merged and will stick to that from next time.

@chaejunlee chaejunlee changed the title fix(svg): escape single-quote within attribute values of SVG URI fix(svg): escape nested quote(s) within attribute values of SVG URI Dec 13, 2023
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@sapphi-red sapphi-red changed the title fix(svg): escape nested quote(s) within attribute values of SVG URI fix(build): use base64 if SVG contains both single and double quotes Dec 13, 2023
@sapphi-red sapphi-red changed the title fix(build): use base64 if SVG contains both single and double quotes fix(build): use base64 for inline SVG if it contains both single and double quotes Dec 13, 2023
@patak-dev patak-dev merged commit 1bbff16 into vitejs:main Dec 15, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: build p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SVG data-url built with errors
4 participants