[css-values] Spec does not reject duplicate `<request-url-modifier>`s, but WPT does

[AtkinsSJ]

This test requires that a url() with more than one of the same <request-url-modifier> be treated as invalid, but I can't find anything in the spec that says this is invalid. The 3 major engines all pass these tests, so evidently that's the desired behaviour, but it'd be good to specify it. Correction: The 3 major engines pass that test because they don't support url-modifiers at all. So it's not established whether the test behaviour is what's wanted. 😅

The <url> and <url-modifier> definitions in CSS Values 4 do not place any requirements on repeated modifiers.

Likewise, the definitions in CSS Values 5 of <request-url-modifier>s do not say anything about more than one of a given type appearing in a url().

cc: @weinig as you wrote that particular test.

[AtkinsSJ]

The tests also expect the modifiers to be sorted by name alphabetically, which needs mentioning somewhere in "serialize a URL", see #12057.

[weinig]

This test requires that a url() with more than one of the same be treated as invalid, but I can't find anything in the spec that says this is invalid

You are right, the grammar currently allows duplicate modifiers due to the use of <url-modifier>* url( <string> <url-modifier>* )

I don't think duplicate modifiers make sense, so I would propose we change grammar

<url> = <url()> | <src()>

<url()> = url( <string> <url-modifiers> ) | <url-token>
<src()> = src( <string> <url-modifiers> )

<url-modifiers> = <crossorigin-modifier> || <integrity-modifier> || <referrerpolicy-modifier>
<crossorigin-modifier> = crossorigin(anonymous | use-credentials)
<integrity-modifier> = integrity(<string>)
<referrerpolicy-modifier> = referrerpolicy(no-referrer | no-referrer-when-downgrade | same-origin | origin | strict-origin | origin-when-cross-origin | strict-origin-when-cross-origin | unsafe-url)

With this construction, the serialization would fall out of the normal serialize in grammar order assumptions.

@noamr @tabatkins, any thoughts on that change?

[noamr]

This test requires that a url() with more than one of the same be treated as invalid, but I can't find anything in the spec that says this is invalid

You are right, the grammar currently allows duplicate modifiers due to the use of <url-modifier>* url( <string> <url-modifier>* )

I don't think duplicate modifiers make sense, so I would propose we change grammar

<url> = <url()> | <src()>

<url()> = url( <string> <url-modifiers> ) | <url-token>
<src()> = src( <string> <url-modifiers> )

<url-modifiers> = <crossorigin-modifier> || <integrity-modifier> || <referrerpolicy-modifier>
<crossorigin-modifier> = crossorigin(anonymous | use-credentials)
<integrity-modifier> = integrity(<string>)
<referrerpolicy-modifier> = referrerpolicy(no-referrer | no-referrer-when-downgrade | same-origin | origin | strict-origin | origin-when-cross-origin | strict-origin-when-cross-origin | unsafe-url)

With this construction, the serialization would fall out of the normal serialize in grammar order assumptions.

@noamr @tabatkins, any thoughts on that change?

Works for me!

[cdoublev]

Please take this resolution into consideration to change the value definition:

Unknown <url-modifier>s get silently ignored (rather than invalidating the function).

[noamr]

@weinig do you want to take a crack at a spec PR? Otherwise I need to get a few things off my backlog before I can attend to doing this myself.

[weinig]

@noamr, happy to.

I realize there is one little issue with what I proposed, it should actually have a ? in url() and src() and should be:

<url> = <url()> | <src()>

<url()> = url( <string> <url-modifiers>? ) | <url-token>
<src()> = src( <string> <url-modifiers>? )

<url-modifiers> = <crossorigin-modifier> || <integrity-modifier> || <referrerpolicy-modifier>
<crossorigin-modifier> = crossorigin(anonymous | use-credentials)
<integrity-modifier> = integrity(<string>)
<referrerpolicy-modifier> = referrerpolicy(no-referrer | no-referrer-when-downgrade | same-origin | origin | strict-origin | origin-when-cross-origin | strict-origin-when-cross-origin | unsafe-url)

The only question I have is which spec to make the changes in since this would change the CSS Value 4 definition of as well as the CSS Value 5 definitions of the modifiers.

I think probably the least invasive change would be to update both CSS Value 4 to:

<url> = <url()> | <src()>

<url()> = url( <string> <url-modifiers>? ) | <url-token>
<src()> = src( <string> <url-modifiers>? )

and leave without any definitions.

And then update CSS Value 5 with the new changes.

Does that sound good?

[noamr]

SGTM, also you can update both specs using the same PR.
Let's open this for an async resolution first?

[weinig]

Ok, what needs to be done to open for async resolution?

[cdoublev]

Please take this resolution into consideration to change the value definition:

Unknown <url-modifier>s get silently ignored (rather than invalidating the function).

<url()> = url( <string> <url-modifiers>? ) | <url-token>
<src()> = src( <string> <url-modifiers>? )

<url-modifiers> = <request-url-modifier> || [ <ident> | <function-token> <any-value>? ]+
<request-url-modifier> = <crossorigin-modifier> || <integrity-modifier> || <referrerpolicy-modifier>

[astearns]

@weinig could you summarize the intended resolution?

[astearns]

As I’m not sure what to propose here, please re-add a tag when things are ready to discuss on a call or there is a proposed async resolution.

[weinig]

Intended resolution:

Update grammar for <url> to disallow duplicate <request-url-modifier> productions by changing the grammar from:

<url> = <url()> | <src()>

<url()> = url( <string> <url-modifier>* ) | <url-token>
<src()> = src( <string> <url-modifier>* )

to

<url> = <url()> | <src()>

<url()> = url( <string> <url-modifiers>? ) | <url-token>
<src()> = src( <string> <url-modifiers>? )

and adding the definition of <url-modifiers> as:

<url-modifiers> = <request-url-modifier> || [ <ident> | <function-token> <any-value>? ]+
<request-url-modifier> = <crossorigin-modifier> || <integrity-modifier> || <referrerpolicy-modifier>
<crossorigin-modifier> = crossorigin(anonymous | use-credentials)
<integrity-modifier> = integrity(<string>)
<referrerpolicy-modifier> = referrerpolicy(no-referrer | no-referrer-when-downgrade | same-origin | origin | strict-origin | origin-when-cross-origin | strict-origin-when-cross-origin | unsafe-url)