Skip to content

Add a check for urls in loose mode.#118

Closed
robert-j-webb wants to merge 1 commit intoshellscape:masterfrom
robert-j-webb:pull
Closed

Add a check for urls in loose mode.#118
robert-j-webb wants to merge 1 commit intoshellscape:masterfrom
robert-j-webb:pull

Conversation

@robert-j-webb
Copy link

Fixes #117

I do not like this solution but I think that it is somewhat close to a good answer.

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

If yes, please describe the breakage.

Not really breakages but it stops parsing splitting up a url into operators. It's just a fixing, although the behavior is different.

Please Describe Your Changes

See #117

Fixes shellscape#117

I do not like this solution but I think that it is somewhat close to a good answer.
@shellscape
Copy link
Owner

Good first attempt, but the solution should not modifying existing token snapshots.

@robert-j-webb
Copy link
Author

robert-j-webb commented May 26, 2020

Good first attempt, but the solution should not modifying existing token snapshots.

Not sure which snapshots you're referring to. I actually made sure that the modifications are improvements over what exists previously, ie:

Would you rather have http://domain.com/gfx/img/bg.jpg represented by:

        { type: 'word', value: 'http' },
        { type: 'colon', value: ':' },
        { type: 'operator', value: '/' },
        { type: 'operator', value: '/' },
        { type: 'word', value: 'domain.com/gfx/img/bg.jpg' },

or

        { type: 'word', value: ' http://domain.com/gfx/img/bg.jpg ' },

I think it's pretty straightforward that no one is parsing a URL and they want the punctuation in it to be represented as a colon and an operator.

If you mean this one:

{ type: 'string', value: '/gfx/img/bg.jpg', raws: { before: ' ', after: " ", quote: '"' } },

vs

{ type: 'word', value: ' "/gfx/img/bg.jpg" ', raws: { before: '', after: "" } },

IDK it seems identical to me, in that the main difference is my version has the raws inside of the value, whereas the repo version has them outside. Is this something that people really need to differentiate on? My main use case for this package is prettier and I'm curious as to what other things people are doing with this.

@shellscape
Copy link
Owner

IDK it seems identical to me,

Except that it's not

in that the main difference is my version has the raws inside of the value, whereas the repo version has them outside.

Exactly. That's a breaking change and introduces bugs, as raws should not be part of the token value. There's quite a bit to reference as far as how things should look, we've got pretty good tests.

Is this something that people really need to differentiate on?

Yes. Never assume that your use case is the only use case.

this.current.value === 'url' && this.currToken[0] !== 'string' &&
this.currToken[0] !== ')' && !this.options.loose) {

this.currToken[0] !== ')' && (checkForUrl(this.currToken[1]) || !this.options.loose)) {
Copy link

Choose a reason for hiding this comment

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

should the logic here be to automatically treat url() contents as a string? technically it should follow https://drafts.csswg.org/css-syntax-3/#consume-url-token for an unquoted string, if i'm reading it right (debatable) the alg is just to consume the content unless you hit a bad char or close parens? e.g. a var() in a url shouldn't be treated as an actual var

Copy link
Owner

Choose a reason for hiding this comment

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

agreed. I'm not sure what the spec says (I haven't looked yet) but if you try to use something like this: url(https://foo.bar/?=)bad char); in chrome and firefox, it'll mark it as invalid CSS and won't apply the rule. Even url(https://foo.bar/?=var(baz)bad char); is marked as bad after the closing var paren. I can't tell or not if either browser allow url(https://foo.bar/?=var(bad char); - there's some odd visuals there but neither will apply the rule.

Using the validator: https://jigsaw.w3.org/css-validator/#validate_by_input

fail - url(https://foo.bar/?=var)bad char);
fail - url(https://foo.bar/?=var()bad char);
fail - url(https://foo.bar/?=var(bad char);

So any extra parens when the value is not quoted will result in an error. So we should probably let the parser in place go through the unquoted tokens, and then afterward combine them all into a string. Any invalid unquoted tokens should be picked up. The only gotcha here is that CSS doesn't several unescaped characters in unquoted url params, so we'd have to identify those and run additional checks.

@shellscape
Copy link
Owner

Closing as abdoned. If this gets picked back up, please ping me.

@shellscape shellscape closed this Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

URL parameters are being parsed into functional groups, when they should be ignored.

3 participants