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.

Sign up for GitHub

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

Jump to bottom

Fix a bug: css urls are incorrectly stripped of commas #14476

Merged
merged 24 commits into from
Mar 29, 2023
+129 −15
Merged

Fix a bug: css urls are incorrectly stripped of commas #14476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
44ca4fc
test for #14237 "@font-face urls are incorrectly stripped of commas"
seiyab Mar 2, 2023
dda6511
fix #14237
seiyab Mar 9, 2023
4a92d92
update snapshot for #14237
seiyab Mar 9, 2023
3f86644
add changelog
seiyab Mar 9, 2023
5e96943
fix typo in changelog
seiyab Mar 9, 2023
3e31769
Update changelog_unreleased/css/14476.md
seiyab Mar 10, 2023
0f60624
add test for broken edge case
seiyab Mar 10, 2023
3dccc3e
fix some edge cases
seiyab Mar 11, 2023
2ca6512
Add edge cases
seiyab Mar 11, 2023
2977c89
add trailing triple backquotes into changelog
seiyab Mar 12, 2023
2fcf543
prevent AST change false positive
seiyab Mar 13, 2023
4d4d2a6
change approach
seiyab Mar 13, 2023
dd9158b
Minor refactor
fisker Mar 21, 2023
aaf0417
Rename
fisker Mar 21, 2023
d76003f
Simplify
fisker Mar 21, 2023
234fe6c
Rename function
fisker Mar 21, 2023
4186594
Test space
fisker Mar 21, 2023
b5fa585
More test
fisker Mar 21, 2023
aa52707
Rename functions
fisker Mar 24, 2023
b7751a8
Fix path
fisker Mar 24, 2023
d0b816e
Fix
fisker Mar 24, 2023
3579c7e
Merge remote-tracking branch 'origin/main' into 14237-css-urls-are-in…
seiyab Mar 28, 2023
4935c44
revert line unexpectedly written by editor
seiyab Mar 28, 2023
ff86851
Update 14476.md
fisker Mar 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 19 additions & 0 deletions changelog_unreleased/css/14476.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#### Fix url contains comma (#14476 by @seiyab)

<!-- prettier-ignore -->
```css
/* Input */
@font-face {
src: url(RobotoFlex-VariableFont_GRAD,XTRA,YOPQ,YTAS,YTDE,YTFI,YTLC,YTUC,opsz,slnt,wdth,wght.ttf);
}

/* Prettier stable */
@font-face {
src: url(RobotoFlex-VariableFont_GRADXTRAYOPQYTASYTDEYTFIYTLCYTUCopszslntwdthwght.ttf);
}

/* Prettier main */
@font-face {
src: url(RobotoFlex-VariableFont_GRAD,XTRA,YOPQ,YTAS,YTDE,YTFI,YTLC,YTUC,opsz,slnt,wdth,wght.ttf);
}
```
18 changes: 4 additions & 14 deletions src/language-css/parse/parse-value.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,12 @@
import PostcssValuesParser from "postcss-values-parser/lib/parser.js";
import getFunctionArgumentsText from "../utils/get-function-arguments-text.js";
import getValueRoot from "../utils/get-value-root.js";
import hasSCSSInterpolation from "../utils/has-scss-interpolation.js";
import hasStringOrFunction from "../utils/has-string-or-function.js";
import isSCSSVariable from "../utils/is-scss-variable.js";
import stringifyNode from "../utils/stringify-node.js";
import parseSelector from "./parse-selector.js";
import { addTypePrefix } from "./utils.js";

const getHighestAncestor = (node) => {
while (node.parent) {
node = node.parent;
}
return node;
};

function parseValueNode(valueNode, options) {
const { nodes } = valueNode;
let parenGroup = {
Expand Down Expand Up @@ -48,7 +42,7 @@ function parseValueNode(valueNode, options) {
if (node.type === "func" && node.value === "selector") {
node.group.groups = [
parseSelector(
getHighestAncestor(valueNode).text.slice(
getValueRoot(valueNode).text.slice(
node.group.open.sourceIndex + 1,
node.group.close.sourceIndex
)
Expand Down Expand Up @@ -76,10 +70,7 @@ function parseValueNode(valueNode, options) {
(!hasStringOrFunction(groupList) &&
!isSCSSVariable(groupList[0], options))
) {
const stringifiedContent = stringifyNode({
groups: node.group.groups,
});
node.group.groups = [stringifiedContent.trim()];
node.group.groups = [getFunctionArgumentsText(node)];
}
}
if (node.type === "paren" && node.value === "(") {
Expand Down Expand Up @@ -162,7 +153,6 @@ function parseNestedValue(node, options) {
}
}
}
delete node.parent;
}
return node;
}
Expand Down
13 changes: 13 additions & 0 deletions src/language-css/utils/get-function-arguments-text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import getValueRoot from "./get-value-root.js";

/**
* @param {*} node
* @returns {string}
*/
function getFunctionArgumentsText(node) {
return getValueRoot(node)
.text.slice(node.group.open.sourceIndex + 1, node.group.close.sourceIndex)
.trim();
}

export default getFunctionArgumentsText;
9 changes: 9 additions & 0 deletions src/language-css/utils/get-value-root.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const getValueRoot = (node) => {
while (node.parent) {
node = node.parent;
}

return node;
};

export default getValueRoot;
6 changes: 5 additions & 1 deletion src/language-css/utils/has-string-or-function.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
function hasStringOrFunction(groupList) {
return groupList.some(
(group) => group.type === "string" || group.type === "func"
(group) =>
group.type === "string" ||
(group.type === "func" &&
// workaround false-positive func
!group.value.endsWith("\\"))
);
}

Expand Down
52 changes: 52 additions & 0 deletions tests/format/css/url/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,63 @@ div {
background: -fb-url(/images/bg.png);
}

@font-face {
src: url(RobotoFlex-VariableFont_GRAD,XTRA,YOPQ,YTAS,YTDE,YTFI,YTLC,YTUC,opsz,slnt,wdth,wght.ttf);
src: url(foo.ttf?query=foo,bar,);
src: url(foo.woff2?foo=rgb\\(255,255,0\\));
}

a {
content: url(https://example.com/\\)\\).jpg);
content: url(https://example.com/\\(\\(.jpg);
content: url(https://example.com/\\ \\ .jpg);
content: url( https://example.com/\\)\\).jpg );
content: url( https://example.com/\\(\\(.jpg );
content: url( https://example.com/\\ \\ .jpg );

background:
no-repeat url(https://example.com/\\)\\).jpg),
no-repeat url(https://example.com/\\(\\(.jpg),
no-repeat url(https://example.com/\\ \\ .jpg),
no-repeat url( https://example.com/\\)\\).jpg ),
no-repeat url( https://example.com/\\(\\(.jpg ),
no-repeat url( https://example.com/\\ \\ .jpg ),
no-repeat url(foo.ttf?query=foo,bar,),
no-repeat url(foo.woff2?foo=rgb\\(255,255,0\\))
no-repeat url(RobotoFlex-VariableFont_GRAD,XTRA,YOPQ,YTAS,YTDE,YTFI,YTLC,YTUC,opsz,slnt,wdth,wght.ttf);
;
}

=====================================output=====================================
div {
background: url(/images/bg.png);
background: -fb-url(/images/bg.png);
}

@font-face {
src: url(RobotoFlex-VariableFont_GRAD,XTRA,YOPQ,YTAS,YTDE,YTFI,YTLC,YTUC,opsz,slnt,wdth,wght.ttf);
src: url(foo.ttf?query=foo,bar,);
src: url(foo.woff2?foo=rgb\\(255,255,0\\));
}

a {
content: url(https://example.com/\\)\\).jpg);
content: url(https://example.com/\\(\\(.jpg);
content: url(https://example.com/\\ \\ .jpg);
content: url( https://example.com/\\)\\).jpg );
content: url( https://example.com/\\(\\(.jpg );
content: url(https://example.com/\\ \\ .jpg);
Copy link
Member

Choose a reason for hiding this comment

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

Should we trim here?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I thought yes, at the first time.
But I feel I need to know CSS Specification more...

Copy link
Member

Choose a reason for hiding this comment

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

As I tested with in chrome

<style>
body {
  background: url(              1.png?_=      )
}
</style>

and

<style>
body {
  background: url(              1.png      )
}
</style>

They all requesting urls without space.

Copy link
Sponsor Contributor Author

@seiyab seiyab Mar 23, 2023 •
edited

Choose a reason for hiding this comment

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

postcss-values-parser throws.
I think it should be a bug.
I'll try newer versions.

const valueParser = require("postcss-values-parser");
valueParser('url(   https://example.com/\\(\\(.jpg   )', {loose: true}).parse()
Uncaught ParserError: Expected closing parenthesis at line: 1, column 4
    at Parser.error (~/Projects/oss/prettier/node_modules/postcss-values-parser/lib/parser.js:127:11)
    at Parser.parenOpen (~/Projects/oss/prettier/node_modules/postcss-values-parser/lib/parser.js:279:12)
    at Parser.parseTokens (~/Projects/oss/prettier/node_modules/postcss-values-parser/lib/parser.js:235:14)
    at Parser.loop (~/Projects/oss/prettier/node_modules/postcss-values-parser/lib/parser.js:132:12)
    at Parser.parse (~/Projects/oss/prettier/node_modules/postcss-values-parser/lib/parser.js:51:17)

Copy link
Sponsor Contributor Author

@seiyab seiyab Mar 23, 2023 •
edited

Choose a reason for hiding this comment

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

v3.2.1 seems good.

const { parse } = require('postcss-values-parser');
parse('url(   https://example.com/\\(\\(.jpg   )');
<ref *1> Root {
  raws: { semicolon: false, after: '' },
  type: 'root',
  nodes: [
    Func {
      raws: [Object],
      name: 'url',
      type: 'func',
      isColor: false,
      isVar: false,
      nodes: [Array],
      parent: [Circular *1],
      source: [Object],
      params: '(   https://example.com/\\(\\(.jpg   )'
    }
  ],
  source: {
    input: Input {
      css: 'url(   https://example.com/\\(\\(.jpg   )',
      hasBOM: false,
      id: '<input css 1>'
    },
    start: { line: 1, column: 1 }
  },
  toString: [Function: bound toString]
}

But v3.0.0 is a big breaking release. https://github.com/shellscape/postcss-values-parser/releases
Which way do you like?

  • Bump postcss-values-parser on next branch via another PR first, and then work on this bug.
  • Leave 'url( https://example.com/\\(\\(.jpg )' and merge this PR first. And then try to bump postcss-values-parser on next branch. Finally, tackle 'url( https://example.com/\\(\\(.jpg )' (or, perhaps it will be resolved by update).

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Will it conflict with your work if I'll try to bump postcss-value-parser?
Referring following, I'm asking you.

I want to make some change to the value-comma_group part

#14480

Copy link
Member

@fisker fisker Mar 30, 2023 •
edited

Choose a reason for hiding this comment

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

No. Go a head, but it won't be easy, I've tried few times since maybe 2~3 years ago.

Copy link
Member

Choose a reason for hiding this comment

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

Last attempt by other maintainer. #11667

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Thank you for your replies.
Sounds so hard!
Any way, I'll try it, though I will be likely to give up 🔥

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

@fisker
Hi, I have tried migrating postcss-values parser from v2 to v6.
Eventually, I gave up because shellscape/postcss-values-parser#140 prevents us from migrating.
May I summarize what I investigated for migration somewhere (perhaps an issue or a discussion)?


background: no-repeat url(https://example.com/\\)\\).jpg),
no-repeat url(https://example.com/\\(\\(.jpg),
no-repeat url(https://example.com/\\ \\ .jpg),
no-repeat url( https://example.com/\\)\\).jpg ),
no-repeat url( https://example.com/\\(\\(.jpg ),
no-repeat url( https://example.com/\\ \\ .jpg ),
no-repeat url(foo.ttf?query=foo,bar,),
no-repeat url(foo.woff2?foo=rgb\\(255,255,0\\))
no-repeat url(RobotoFlex-VariableFont_GRAD,XTRA,YOPQ,YTAS,YTDE,YTFI,YTLC,YTUC,opsz,slnt,wdth,wght.ttf);
}

================================================================================
`;
27 changes: 27 additions & 0 deletions tests/format/css/url/url.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,30 @@ div {
background: url(/images/bg.png);
background: -fb-url(/images/bg.png);
}

@font-face {
src: url(RobotoFlex-VariableFont_GRAD,XTRA,YOPQ,YTAS,YTDE,YTFI,YTLC,YTUC,opsz,slnt,wdth,wght.ttf);
src: url(foo.ttf?query=foo,bar,);
src: url(foo.woff2?foo=rgb\(255,255,0\));
}

a {
content: url(https://example.com/\)\).jpg);
content: url(https://example.com/\(\(.jpg);
content: url(https://example.com/\ \ .jpg);
content: url( https://example.com/\)\).jpg );
content: url( https://example.com/\(\(.jpg );
content: url( https://example.com/\ \ .jpg );

background:
no-repeat url(https://example.com/\)\).jpg),
no-repeat url(https://example.com/\(\(.jpg),
no-repeat url(https://example.com/\ \ .jpg),
no-repeat url( https://example.com/\)\).jpg ),
no-repeat url( https://example.com/\(\(.jpg ),
no-repeat url( https://example.com/\ \ .jpg ),
no-repeat url(foo.ttf?query=foo,bar,),
no-repeat url(foo.woff2?foo=rgb\(255,255,0\))
no-repeat url(RobotoFlex-VariableFont_GRAD,XTRA,YOPQ,YTAS,YTDE,YTFI,YTLC,YTUC,opsz,slnt,wdth,wght.ttf);
;
}