Bug 198512 – Presence of CSS variable causes a background url() to get resolved with a different base

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Simon Fraser (smfr)]

Created attachment 371239 [details]
Testcase

If the background shorthand has a variable reference in it:
    background: url('images/ducky.png') var(--darkGreen);

then URLs in that shorthand are resolved relative to the document URL, not the style sheet URL, which is wrong.

[Radar WebKit Bug Importer]

<rdar://problem/51379244>

[Simon Fraser (smfr)]

The CSSParserContext's baseURL is different in the two cases.

[Simon Fraser (smfr)]

...and that's because StyleResolver::resolvedVariableValue() makes a CSSParser parser(document()), so with the document's baseURL, rather than the stylesheet's.

[JC Simpson]

*** Bug 217124 has been marked as a duplicate of this bug. ***

[Tyler Wilcock]

Created attachment 410448 [details]
Patch

[Darin Adler]

Comment on attachment 410448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410448&action=review

> Source/WebCore/css/CSSPendingSubstitutionValue.h:40
> +    static Ref<CSSPendingSubstitutionValue> create(CSSPropertyID shorthandPropertyId, Ref<CSSVariableReferenceValue>&& shorthandValue, const URL baseURL)

"const URL" is not the type we want for this argument. Could use "const URL&" or "URL".

> Source/WebCore/css/CSSPendingSubstitutionValue.h:47
> +    const URL baseURL() const { return m_baseURL; }

Same here with return value. It can be URL or const URL&, but const URL is not right.

> Source/WebCore/css/CSSPendingSubstitutionValue.h:64
> +    const URL m_baseURL;

Here, the type "const URL" is fine, good, appropriate.

[Tyler Wilcock]

Created attachment 410486 [details]
Patch

[Tyler Wilcock]

Thanks Darin.  I've fixed that and marked the new patch with cq?

[EWS]

Committed r267951: <https://trac.webkit.org/changeset/267951>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410486 [details].

[Sam Sneddon [:gsnedders]]

*** Bug 190807 has been marked as a duplicate of this bug. ***

[Vadim Makeev]

I have just discovered the same problem in my code, here’s the demo:

https://bald-ring-chauffeur.glitch.me/ (should be gold)
https://bald-ring-chauffeur.glitch.me/subfolder/ (should be green)

And here’s the code https://glitch.com/edit/#!/bald-ring-chauffeur

It’s a major incompatibility and I’m shocked to see that fix still haven’t shipped. I wonder how soon it’ll make it into Safari, it’s been two years already.

[Darin Adler]

That "same problem" is not fixed by this patch.

So either this bug wasn’t successfully fixed 10 months ago, or there is more than one problem with the same effect.

[Vadim Makeev]

(In reply to Darin Adler from comment #12)
> That "same problem" is not fixed by this patch.
> 
> So either this bug wasn’t successfully fixed 10 months ago, or there is more
> than one problem with the same effect.

I’m sorry, but for an outsider it’s hard to tell whether it’s not shipped or it’s not sufficient. Description of this bug and two duplicates looks rather identical to my case.

Do you think I should file another bug or someone would reopen the current one? I’m happy to help in any way to get this bug fixed.

[Darin Adler]

I just noticed that we have bug 200092, which might be the problem you are seeing and is not yet fixed.

But you can also file a new bug report. That could do some good and should do no harm.

[Darin Adler]

I’m surprised this is not covered in Web Platform Tests; maybe it is and there is a failing test?

[Darin Adler]

(The fix would have been in a Safari Technology Preview release soon after the fix landed on 2020-10-04.)

[Darin Adler]

The patch definitely fixed a bug, it included a test case and that test case was failing and is now passing. We need to figure out what’s different about the new one that is still failing.

[Darin Adler]

OK, studied it a bit more, and yes, the bug Vadim is reporting is bug 200092, not the same as this one. Continuing the discussion there.

[Darin Adler]

(In reply to Vadim Makeev from comment #13)
> Do you think I should file another bug or someone would reopen the current
> one? I’m happy to help in any way to get this bug fixed.

I finally figured out what's wrong and bug 200092 covers fixing it.

[Darin Adler]

(In reply to Vadim Makeev from comment #13)
> Do you think I should file another bug or someone would reopen the current
> one? I’m happy to help in any way to get this bug fixed.

I finally figured out what's wrong and bug 230243 covers fixing it.