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

seiyab · 2023-03-09T21:20:26Z

Description

Fix #14237

Checklist

I’ve added tests to confirm my change works.
~~(If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).~~
(If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
I’ve read the contributing guidelines.

✨Try the playground for this PR✨

…commas"

seiyab · 2023-03-09T21:32:29Z

src/language-css/utils/stringify-node.js

-      .join(node.groups[0]?.type === "comma_group" ? "," : "");
+      .join(node.type === "paren_group" ? "," : "");


Commas had disappeard if the first comma group is flattened.
Groups in paren group should always be separated by comma.

src/language-css/parser-postcss.js

fisker · 2023-03-10T01:43:40Z

Fix #14476

Wrong link

changelog_unreleased/css/14476.md

fisker · 2023-03-10T02:01:29Z

Can you try get original text instead of using stringifyNode? There are still edge cases broken.

Prettier pr-14476
Playground link

--parser css

Input:

@font-face {
  src: url(foo.ttf?query=foo,bar,);
  src: url(foo.woff2?foo=rgb(255,255,0));
}

Output:

@font-face {
  src: url(foo.ttf?query=foo,bar);
  src: url(foo.woff2?foo=rgb(255, 255, 0));
}

Co-authored-by: fisker Cheung <lionkay@gmail.com>

seiyab · 2023-03-10T04:00:50Z

Thanks for pointing edge cases.
And throwing error for @import url(https://example.com/image\).png) also seems a bug.

If you choose to write the URL without quotes, use a backslash (\) before any parentheses, whitespace characters, single quotes (') and double quotes (") that are part of the URL.

seiyab · 2023-03-11T03:52:57Z

Trying to the original text isn't going smoothly.

Approach 1. use text at value-root
- It contains some texts other than url() node such as comment. Reveaing only url() seems to be hard.
Approach 2. use toString() of postcss-value-parser
- It throws in prettier css parser because it removes nodes from parsed value.
- Edit: Removing delete statement made this approach work. 🌵

I'm still trying 🚀

seiyab · 2023-03-11T04:52:52Z

Following edge case still goes wrong...

https://deploy-preview-14476--prettier.netlify.app/playground/#N4Igxg9gdgLgprEAuEBDABMAOld7KwIxLoCuATgDYAUAFjDAA4DOSA9G3AB6oC2jlOADpIvNliwBKCegnUhAK2bRJAbhwBfEABoQERjACW0ZslCpy5CAHcAChYSmUqStdQBPU7oBG5VGABrOBgAZT44ABlDKDhkADMXZjgfP0DgkMZ-aIBzZBhyUmSQOF5vOAATcoqI1Chs0lRsuAAxCHJeVAYc5DRSGAgdEHpeSgB1WkN4ZkywOBDHScMAN0n3HrBmLxBopPIYWz9sjvjEoqUuEJzBAEVSCHgTyiTdTPJd9c3BxnJomFHDcowWjIAAcAAYXlYkqM-Iwet84LslrFdABHO7wA76JxoZgAWhiFQqg3IcHRhlJB0axyQCSeRSSvEMeQKDKucAAggwft4+nBbHByFEYo9niBmOzbvdYrTTroYKhvP9AcCkAAmeV+QyUHIAYQgvBpxWYAFZBqQkgAVRVOOlipaFACSUCqsBCYB+Bg5LpCMHcglFcA0GiAA

Input

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

Output

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

seiyab · 2023-03-11T04:57:19Z

↑postcss-value-parser seems to go wrong.
Can't we give up this case at least in this PR?

Part of parse result against `url(https://example.com/\)\ \(.json)`

nodes: [
  FunctionNode { 
    raws: [Object],
    value: 'url',
    source: [Object],
    sourceIndex: 0,
    nodes: [Array],
    type: 'func',
    unbalanced: 0,
    parent: [Circular *1]
  },
  Parenthesis { 
    raws: [Object],
    value: '(',
    source: [Object],
    sourceIndex: 26,
    type: 'paren',
    parenType: '',
    parent: [Circular *1]
  },
  Word { 
    raws: [Object],
    value: '.json',
    source: [Object],
    sourceIndex: 27,
    type: 'word',
    isHex: false,
    isColor: false,
    parent: [Circular *1]
  },
  Parenthesis { 
    raws: [Object],
    value: ')',
    source: [Object],
    sourceIndex: 32,
    type: 'paren',
    parenType: '',
    parent: [Circular *1]
  } 
],

src/language-css/parser-postcss.js

@@ -197,7 +194,6 @@ function parseNestedValue(node, options) {
        parseNestedValue(node[key], options);
        if (key === "nodes") {
          node.group = flattenGroups(parseValueNode(node, options));
-          delete node[key];


seiyab · 2023-03-13T22:47:55Z

I'm sorry for annoying commits.
I welcome to change the approach because I'm not sure which approach is good enough.

seiyab · 2023-03-13T22:52:10Z

src/language-css/utils/stringify-func-param.js

+ * @param {*} node
+ * @returns {string}
+ */
+function stringifyFuncParam(node) {


We can also use this here.

prettier/src/language-css/parser-postcss.js

Line 54 in 4d4d2a6

getHighestAncestor(valueNode).text.slice(

I'll change it if you want.

seiyab · 2023-03-17T13:24:50Z

Can I rerun CI?

[2023-03-13T23:07:46.123Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}
Error: Codecov: Failed to properly upload: The process '/home/runner/work/_actions/codecov/codecov-action/v3.1.0/dist/codecov' failed with exit code 255

fisker · 2023-03-21T05:36:02Z

tests/format/css/url/__snapshots__/jsfmt.spec.js.snap

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


Should we trim here?

I thought yes, at the first time.
But I feel I need to know CSS Specification 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.

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)

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).

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

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

Last attempt by other maintainer. #11667

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

@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)?

fisker

Thank you!

fisker · 2023-03-28T10:31:09Z

Can you fix the conflicts?

…correctly-stripped-of-commas-2

fisker · 2023-03-29T01:12:54Z

Good job @seiyab

Co-authored-by: fisker Cheung <lionkay@gmail.com>

seiyab added 4 commits March 3, 2023 07:13

test for prettier#14237 "@font-face urls are incorrectly stripped of …

44ca4fc

…commas"

fix prettier#14237

dda6511

update snapshot for prettier#14237

4a92d92

add changelog

3f86644

seiyab mentioned this pull request Mar 9, 2023

Fix some CSS problems: incorrectly stripped of commas, incorrectly removed empty lines. #14472

Closed

4 tasks

fix typo in changelog

5e96943

seiyab marked this pull request as ready for review March 9, 2023 21:29

seiyab commented Mar 9, 2023

View reviewed changes

fisker reviewed Mar 10, 2023

View reviewed changes

src/language-css/parser-postcss.js Outdated Show resolved Hide resolved

fisker reviewed Mar 10, 2023

View reviewed changes

changelog_unreleased/css/14476.md Outdated Show resolved Hide resolved

Update changelog_unreleased/css/14476.md

3e31769

Co-authored-by: fisker Cheung <lionkay@gmail.com>

seiyab added 2 commits March 11, 2023 13:45

add test for broken edge case

0f60624

fix some edge cases

3dccc3e

seiyab requested a review from fisker March 11, 2023 04:59

Add edge cases

2ca6512

seiyab force-pushed the 14237-css-urls-are-incorrectly-stripped-of-commas-2 branch from 220064f to 2ca6512 Compare March 11, 2023 04:59

add trailing triple backquotes into changelog

2977c89

seiyab commented Mar 12, 2023

View reviewed changes

seiyab added 2 commits March 13, 2023 22:05

prevent AST change false positive

2fcf543

change approach

4d4d2a6

seiyab commented Mar 13, 2023

View reviewed changes

Minor refactor

dd9158b

fisker added 4 commits March 21, 2023 13:26

Rename

aaf0417

Simplify

d76003f

Rename function

234fe6c

Test space

4186594

fisker reviewed Mar 21, 2023

View reviewed changes

More test

b5fa585

seiyab mentioned this pull request Mar 24, 2023

Format TypeScript expressions in Vue template attributes #14506

Merged

4 tasks

fisker added 2 commits March 24, 2023 14:01

Rename functions

aa52707

Fix path

b7751a8

fisker approved these changes Mar 24, 2023

View reviewed changes

Fix

d0b816e

fisker added this to the 3.0 milestone Mar 28, 2023

seiyab and others added 3 commits March 28, 2023 22:59

Merge remote-tracking branch 'origin/main' into 14237-css-urls-are-in…

3579c7e

…correctly-stripped-of-commas-2

revert line unexpectedly written by editor

4935c44

Update 14476.md

ff86851

fisker merged commit 9500403 into prettier:main Mar 29, 2023
15 checks passed

seiyab deleted the 14237-css-urls-are-incorrectly-stripped-of-commas-2 branch March 29, 2023 05:26

medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Feb 13, 2024

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

6363947

Co-authored-by: fisker Cheung <lionkay@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

seiyab commented Mar 9, 2023 •

edited

seiyab Mar 9, 2023

fisker commented Mar 10, 2023

fisker commented Mar 10, 2023 •

edited

seiyab commented Mar 10, 2023

seiyab commented Mar 11, 2023 •

edited

seiyab commented Mar 11, 2023

seiyab commented Mar 11, 2023 •

edited

This comment was marked as outdated.

seiyab commented Mar 13, 2023

seiyab Mar 13, 2023

seiyab commented Mar 17, 2023

fisker Mar 21, 2023

seiyab Mar 22, 2023

fisker Mar 22, 2023

seiyab Mar 23, 2023 •

edited

seiyab Mar 23, 2023 •

edited

seiyab Mar 30, 2023

fisker Mar 30, 2023 •

edited

fisker Mar 30, 2023

seiyab Mar 30, 2023

seiyab May 1, 2023

fisker left a comment

fisker commented Mar 28, 2023

fisker commented Mar 29, 2023

		.join(node.groups[0]?.type === "comma_group" ? "," : "");
		.join(node.type === "paren_group" ? "," : "");

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

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

Conversation

seiyab commented Mar 9, 2023 • edited

Description

Checklist

Choose a reason for hiding this comment

fisker commented Mar 10, 2023

fisker commented Mar 10, 2023 • edited

seiyab commented Mar 10, 2023

seiyab commented Mar 11, 2023 • edited

seiyab commented Mar 11, 2023

seiyab commented Mar 11, 2023 • edited

This comment was marked as outdated.

seiyab commented Mar 13, 2023

Choose a reason for hiding this comment

seiyab commented Mar 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seiyab Mar 23, 2023 • edited

Choose a reason for hiding this comment

seiyab Mar 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisker Mar 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisker left a comment

Choose a reason for hiding this comment

fisker commented Mar 28, 2023

fisker commented Mar 29, 2023

seiyab commented Mar 9, 2023 •

edited

fisker commented Mar 10, 2023 •

edited

seiyab commented Mar 11, 2023 •

edited

seiyab commented Mar 11, 2023 •

edited

seiyab Mar 23, 2023 •

edited

seiyab Mar 23, 2023 •

edited

fisker Mar 30, 2023 •

edited