Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CSS2] empty url() behaviour needs errata to match Level 3 #2211

Open
gsnedders opened this issue Jan 21, 2018 · 14 comments
Open

[CSS2] empty url() behaviour needs errata to match Level 3 #2211

gsnedders opened this issue Jan 21, 2018 · 14 comments
Labels
Closed Accepted by CSSWG Resolution CSS2 Needs Edits

Comments

@gsnedders
Copy link
Contributor

https://drafts.csswg.org/css-values-3/#url-empty states:

If the value of the url() is the empty string (like url("") or url()), the url must resolve to an invalid resource (similar to what the url about:invalid does).

This contradicts CSS2, which states:

In order to create modular style sheets that are not dependent on the absolute location of a resource, authors may use relative URIs. Relative URIs (as defined in [RFC3986]) are resolved to full URIs using a base URI. RFC 3986, section 5, defines the normative algorithm for this process. For CSS style sheets, the base URI is that of the style sheet, not that of the source document.

As an empty URL is a relative URI as defined by RFC3986, per CSS2 if the value of the url() is the empty string, then it must resolve to the URL of the style sheet.

This is observable both through the CSSOM and through use of content negotiation and polyglot documents.
@gsnedders
Copy link
Contributor Author

(FWIW, credit to web-platform-tests/wpt#9113 (comment) for making me notice this discrepancy.)
@fantasai
Copy link
Collaborator

fantasai commented Feb 3, 2018

@gsnedders Can you clarify what behavior is currently implemented? Because we should align both specs to that. (Assuming it's one or the other, and not something else completely insane.)
@gsnedders
Copy link
Contributor Author

FWIW:

This was added in e051892, per a WG resolution, so we'd presumably have to revert the resolution and then maybe eventually issue errata for 2.1 and 3 if we still want to change this behaviour in 4.
@fantasai
Copy link
Collaborator

fantasai commented Feb 6, 2018

@gsnedders I can't seem to find the minutes on this issue. :( But also, can you answer the above question? :)
@FremyCompany
Copy link
Contributor

Edge does not parse url() as a valid url (by choice, visibly).
Chrome and Firefox seems to.
@fantasai
Copy link
Collaborator

@FremyCompany That seems odd. What about url('')?
@FremyCompany
Copy link
Contributor

@fantasai I apparently need to correct my previous statement. Both url are valid at parse time in Edge, we just incorrectly serialize them as 'none' in background-image. If I had used content I would have seen it work.

The default value of the flag isURL = false I saw in the code is irrelevant because its value is always set to true when you encounter the closing parenthesis and nothing triggered fallback to bad-url-token before that.
@css-meeting-bot
Copy link
Member

The Working Group just discussed [CSS2] empty url() behaviour needs errata to match Level 3, and agreed to the following resolutions:

  • RESOLVED: revert the change and open an issue noting that we had this change and we're not sure about it at this point.
The full IRC log of that discussion <dael> Topic: [CSS2] empty url() behaviour needs errata to match Level 3
<dael> github: https://github.com//issues/2211
<dael> fantasai: Basically gsnedders found css2 and css3 disagree what to do with an empty URL.
<dael> fantasai: Looks like we decided for L3 to make it invalid instead of refer to location of stylesheet. I couldn't find the minutes of that resolution so if anyone remembers that discussion it would be useful. We should make specs agree and impl to agree with specs.
<dael> astearns: Do we have data on current impl?
<dael> fantasai: gsnedders do you know? I tested using computed values wehre i got URL of the page, it was invalid
<dael> gsnedders: I think Chrome & FF resolve to page itself. I'm not sure.
<dael> astearns: Proposal is change L3 to match impl and revert the invalid resource bit?
<dael> fantasai: Would need to look what that looks like in the changeset. I have no real opinion on which way to go.
<dael> astearns: I spent time trying to find the resolution that lead to this and I failed as well.
<dael> astearns: What should we do fantasai ? Resolve to revert or do more research and see what's impl, have tests, and then decide on how to change L3?
<dael> fantasai: I thinkw ehave a fairly good idea. Test we ran so far it's not treated as invalid. It's supposed to be parsed as incorrect. Since this is a spec in CR and we have 2.1 and previous version say valid points at page we should revert. If someone thinks revist we can re-open it. That's my opinion because we want to update V&U.
<dael> fantasai: If we're not sure what to do don't make the change, file an issue, leave it open for later.
<dael> astearns: Given that we're still trying to figure out why we made the change it makes sense to open an issue.
<dael> fantasai: Then let's revert the change tot he draft and publish
<dael> astearns: What about revert change to draft, leave change and note as a comment in the draft so there's something in the source?
<dael> fantasai: I can mark it as an issue in the draft, this is CR spec. That text I can leave it commented in and that text is linked from the issue.
<dael> astearns: Sounds fair.
<dael> astearns: Proposal is revert hte change and open an issue noting that we had this change and we're not sure about it at this point.
<dael> astearns: Obj?
<dael> RESOLVED: revert the change and open an issue noting that we had this change and we're not sure about it at this point.
@gsnedders gsnedders mentioned this issue Feb 15, 2018
Merged
@tabatkins
Copy link
Member

Marking as Agenda+ to protest the resolution (I guess I wasn't able to make this call, seeing as I'm not in the minutes...). The reasoning for the change is stated clearly in the Note immediately following the quoted paragraph, which has been part of the draft since June 2016.

So I'd like to keep this change. The only observable difference is in how it's reflected in the OM; no place where url() is currently used can validly take a stylesheet, so treating url("") as a normal relative url (and thus pointing to the stylesheet itself) always results in a broken image/etc.
@gsnedders
Copy link
Contributor Author

so treating url("") as a normal relative url (and thus pointing to the stylesheet itself) always results in a broken image/etc.

That's surely not true with content negotiation? Or, heck, just crazy polyglot documents like http://incept10n.com/
@tabatkins
Copy link
Member

"always" should be read as "practically always"; insane polyglots or content-negotiation of this sort don't happen in practice and thus aren't relevant for us deciding what to do here.
@css-meeting-bot
Copy link
Member

The Working Group just discussed empty url behavior, and agreed to the following:

  • RESOLVED: Empty URLs are treated as "about:invalid"
The full IRC log of that discussion <fantasai> Topic: empty url behavior
<fantasai> github: https://github.com//issues/2211
<fantasai> ScribeNick: fantasai
<fantasai> TabAtkins: url() function with empty string as its argument...
<fantasai> TabAtkins: I changed spec ot match behavior of empty resource requests in HTML
<fantasai> TabAtkins: and y'all reverted the change because you didn't understand why I did it
<fantasai> TabAtkins: Answer is to be consistent with HTML!
<fantasai> TabAtkins: Empty URL is technically a relative URL to the resource itself
<fantasai> TabAtkins: But e.g. in HTML <img src=""> doesn't load the resource
<fantasai> TabAtkins: Empty URLs will still resolve in some cases, like in links
<fantasai> TabAtkins: But not for resource loading like this. Makes no sense to load the CSS stylesheet as an image of itself.
<fantasai> ...
<fantasai> chris: Do we expose imports in the OM, and would ...??
<fantasai> TabAtkins: I don't know
<fantasai> TabAtkins: But recursively attaching the same URL again wouldn't be useful
<fantasai> TabAtkins: I would like it to automatically fail, invalid URL.
<fantasai> tantek: But not invalid syntax
<fantasai> TabAtkins: No, not invalid syntax
<fantasai> TabAtkins: There's no place in CSS where resolving the URL as the URL of the stylesheet would be useful
<fantasai> TabAtkins: Proposal is that the empty URL, rather than being treated as a relative URL, is treated as an invalid URL
<fantasai> TabAtkins: Computed value is still an empty string, just treated like 'about:invalid'
<fantasai> TabAtkins: There should be no noticeable behavior difference, since the stylesheet is not a valid image/etc.
<tantek> +1
<fantasai> Rossen: Other comments?
<fantasai> RESOLVED: Empty URLs are treated as "about:invalid"
<TabAtkins> ScribeNick: TabAtkins
<TabAtkins> Rossen: So this is the last Values 3 issue.
@fantasai
Copy link
Collaborator

fantasai commented Jan 2, 2019

AFAICT this was already published in the CR in August (shortly after the resolution above), so doesn't need edits. https://www.w3.org/TR/css-values-3/#url-empty
@fantasai
Copy link
Collaborator

fantasai commented Jan 2, 2019

NM, Needs Edits was left for CSS2. Untagging css-values-3 then.
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Jul 14, 2021
@shallawa
RenderElement::updateFillImages must fix the clients of the CSS image…
cd1d311 
… whose URL is invalid

https://bugs.webkit.org/show_bug.cgi?id=227926
<rdar://80457956>

Reviewed by Simon Fraser.

Per CSS2 specs, if the value of the url() is the empty string, then it
must be resolved to the URL of the style sheet which is an invalid URI
for an image. The issue w3c/csswg-drafts#2211
was filed to change this behavior.

In RenderElement::updateFillImages() we fix the old and new fill images
only if they are different. But we consider them equal if their URIs are
equal and they are not data URIs. If the two URIs are empty strings, the
fill images will be considered equal although their CachedImages might
be different.

We need to fix the clients of the fill images always if their CachedImages
have errorOccured() true or have hasImage() false.

* rendering/RenderElement.cpp:
(WebCore::RenderElement::updateFillImages):
* rendering/style/StyleCachedImage.cpp:
(WebCore::StyleCachedImage::hasImage const):
* rendering/style/StyleCachedImage.h:
* rendering/style/StyleImage.h:
(WebCore::StyleImage::hasImage const):


Canonical link: https://commits.webkit.org/239655@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@279906 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@smfr smfr mentioned this issue Sep 9, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Accepted by CSSWG Resolution CSS2 Needs Edits
5 participants
@gsnedders @FremyCompany @tabatkins @fantasai @css-meeting-bot