Fix a bug: css urls are incorrectly stripped of commas by seiyab · Pull Request #14476 · prettier/prettier

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

Description

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.

…commas"

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

    const groups = node.groups
      .map((currentValue) => stringifyNode(currentValue))
-      .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.

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

Fix #14476

Wrong link

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 <[email protected]>

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]
  } 
],

Sign in to view

        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

+ * @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

+  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 )');

Details

<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 <[email protected]>

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

Comment thread src/language-css/parser-postcss.js Outdated

fisker reviewed Mar 10, 2023

View reviewed changes

Comment thread changelog_unreleased/css/14476.md Outdated

Update changelog_unreleased/css/14476.md

3e31769

Co-authored-by: fisker Cheung <[email protected]>

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

Comment thread src/language-css/parser-postcss.js Outdated

parseNestedValue(node[key], options);

if (key === "nodes") {

node.group = flattenGroups(parseValueNode(node, options));

delete node[key];

This comment was marked as outdated.

Sign in to view

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

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 <[email protected]>

costajohnt mentioned this pull request Jan 20, 2026

Commas separating arguments in interpolated function call are being removed when used inside url #13580

Open

Uh oh!

Conversation

seiyab commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fisker commented Mar 10, 2023

Uh oh!

Uh oh!

fisker commented Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seiyab commented Mar 10, 2023

Uh oh!

seiyab commented Mar 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seiyab commented Mar 11, 2023

Uh oh!

seiyab commented Mar 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

seiyab commented Mar 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seiyab commented Mar 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seiyab Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seiyab Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker left a comment

Choose a reason for hiding this comment

Uh oh!

fisker commented Mar 28, 2023

Uh oh!

fisker commented Mar 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

seiyab commented Mar 9, 2023 •

edited

Loading

fisker commented Mar 10, 2023 •

edited

Loading

seiyab commented Mar 11, 2023 •

edited

Loading

seiyab commented Mar 11, 2023 •

edited

Loading

seiyab Mar 23, 2023 •

edited

Loading

seiyab Mar 23, 2023 •

edited

Loading

fisker Mar 30, 2023 •

edited

Loading