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.

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

[cssom] Throwing on invalid pseudo-elements in getComputedStyle is not web-compatible #6501

Closed
emilio opened this issue Aug 9, 2021 · 5 comments · Fixed by #6540
Closed
Labels
cssom-1 Current Work

Comments

@emilio
Copy link
Collaborator

emilio commented Aug 9, 2021

I tried to implement the resolution from #2149 in Firefox, and we instantly found broken pages because of that.

There are two patterns that show up:

  • getComputedStyle(something, "height"); (or any other property instead of height). Seems like a coding error on the given site, but all browsers agree on not throwing there.
  • getComputedStyle(something, false); (which per WebIDL rules gets turned into getComputedStyle(something, "false");). This is also an error (because the right thing to use if you want to be explicit about the argument is null, not false), but it seems throwing there is more harmful than good.

So I propose we need to at least not throw when the string doesn't start with a colon. All browsers agree on returning the non-pseudo-element style there, so I suggest to just spec reality there.

For the cases the string does start with a colon we could:

  • Try to throw (I can come back with data).
  • Not throw, returning the non-pseudo-element style instead (WebKit / Blink behavior): It is consistent with the "string doesn't begin with a colon" case, but it seems a bit unfortunate.
  • Not throw, returning an empty style (length = 0) (Firefox behavior).

I think I prefer the third but I'd love to hear opinions.

@emilio emilio added cssom-1 Current Work Agenda+ labels Aug 9, 2021
@Loirooriol
Copy link
Contributor

Loirooriol commented Aug 9, 2021

What do these sites do with the getComputedStyle(something, "height")? Would it be possible to return an empty style rather that the element style? Possibly with an exception for "false", or false and use an union type DOMString or boolean?

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 9, 2021
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 9, 2021
@emilio
Copy link
Collaborator Author

emilio commented Aug 11, 2021

Both of the cases of getComputedStyle(something, "height") I found come from here (which does nothing btw, that line of code is just useless, I'll send a PR to remove it). So at least returning an empty style there seems ok.

emilio added a commit to emilio/next.js that referenced this issue Aug 11, 2021
See w3c/csswg-drafts#6501 to see how I found this.

This line doesn't recompute layout in browsers, because `"height"` is given as a pseudo-element name rather than a property.

The right way to do what it wants is `getComputedStyle(document.body).height`, but given nobody noticed this (and this is generally never needed, manually triggering layout should never be needed to avoid FOUC) it seems better to keep current behavior and just remove the call.
@birtles
Copy link
Contributor

birtles commented Aug 12, 2021

I was seeing this on https://www.apple.com/jp/ipad-air/ too for what it's worth. (I really wish I made a note of what file/line it occurred on over in webcompat/web-bugs#82685 🤦‍♀️)

@emilio
Copy link
Collaborator Author

emilio commented Aug 12, 2021

Yep, that one is Window.getComputedStyle: 'false' is not a valid pseudo-element, so it's the getComputedStyle(foo, false) case.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Throwing on invlaid pseudo-elements in getComputedStyle, and agreed to the following:

  • RESOLVED: Return empty style if string begins with a colon, return element style otherwise
The full IRC log of that discussion <fantasai> Topic: Throwing on invlaid pseudo-elements in getComputedStyle
<fantasai> github: https://github.com//issues/6501
<fantasai> emilio: resolved to throw in these cases
<fantasai> emilio: I implemented it
<fantasai> emilio: but people do silly things
<fantasai> emilio: and it's not Web-compatible
<fantasai> emilio: One error is passing property names as pseudo-element names
<fantasai> emilio: and one is ???
<jfkthame> s/???/passing 'false'/
<fantasai> emilio: Question is, should we try to do something smarter? Should we return an empty style? Should we not throw and return the element's own style?
<fantasai> emilio: Blink and WebKit return the element's own style
<fantasai> emilio: Firefox does that, but not if pseudo-element starts with two colons
<fantasai> emilio: which is at least a bit more forwards-compatible
<fantasai> astearns: Not returning element style with two-colon strings means we're less likely to have problems when we introduce new pseudo-eements
<fantasai> emilio: Errors we're seeing seem to be mostly typos
<fantasai> emilio: ...
<fantasai> emilio: We could special-case in the IDL
<fantasai> dbaron[m]: Seems the Web-compat probems are strings without double-colon, so could throw on double-colon strings that are also errors
<fantasai> emilio: I'd be OK with that
<fantasai> TabAtkins: Don't have a strong opinion, whatever is both Web- and forwards-compatible
<fantasai> TabAtkins: document legacy weirdness
<fantasai> astearns: We'd resolved on throwing rather than empty style before, why did we decide that?
<fantasai> emilio: usually better, but optimistic that we could get away with it
<fantasai> emilio: anyone object to returning empty style if string begins with colon, otherwise return element style?
<dbaron[m]> the cases where we need to worry about fowards compat are (we think) the start-with-colon case
<dbaron[m]> s
<fantasai> iank_: Does this paint us into a corner for slot pseudos or anything like that?
<fantasai> emilio: I don't think so
<fantasai> emilio: If you do '::slotted' there's no style to return, multiple elements can match it
<fantasai> astearns: objections?
<fantasai> RESOLVED: Return empty style if string begins with a colon, return element style otherwise

jamienicol pushed a commit to jamienicol/gecko that referenced this issue Aug 20, 2021
emilio added a commit to emilio/csswg-drafts that referenced this issue Aug 24, 2021
…-elements, and the element's style if the string doesn't begin with a colon.

Fixes w3c#6501.

Tests in web-platform-tests/wpt#30140.
emilio added a commit that referenced this issue Sep 18, 2021
…-elements, and the element's style if the string doesn't begin with a colon.

Fixes #6501.

Tests in web-platform-tests/wpt#30140.
kodiakhq bot pushed a commit to vercel/next.js that referenced this issue Sep 20, 2021
See w3c/csswg-drafts#6501 to see how I found this.

This line doesn't recompute layout in browsers, because `"height"` is given as a pseudo-element name rather than a property.

The right way to do what it wants is `getComputedStyle(document.body).height`, but given nobody noticed this (and this is generally never needed, manually triggering layout should never be needed to avoid FOUC) it seems better to keep current behavior and just remove the call.
natew pushed a commit to natew/next.js that referenced this issue Feb 16, 2022
See w3c/csswg-drafts#6501 to see how I found this.

This line doesn't recompute layout in browsers, because `"height"` is given as a pseudo-element name rather than a property.

The right way to do what it wants is `getComputedStyle(document.body).height`, but given nobody noticed this (and this is generally never needed, manually triggering layout should never be needed to avoid FOUC) it seems better to keep current behavior and just remove the call.
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 31, 2023
…om highlight API tests.

As per [0], [1] and [2], `getComputedStyle()` should not return the
default style when the pseudo is invalid.
Instead, an empty style should be returned.

[0] https://drafts.csswg.org/cssom/#extensions-to-the-window-interface
[1] w3c/csswg-drafts#6501
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1726396

Differential Revision: https://phabricator.services.mozilla.com/D184810

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1838262
gecko-commit: 1c145766032f7bbdecfd68895b7e3038e923100b
gecko-reviewers: emilio
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 31, 2023
…le()` in custom highlight API tests. r=emilio

As per [0], [1] and [2], `getComputedStyle()` should not return the
default style when the pseudo is invalid.
Instead, an empty style should be returned.

[0] https://drafts.csswg.org/cssom/#extensions-to-the-window-interface
[1] w3c/csswg-drafts#6501
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1726396

Differential Revision: https://phabricator.services.mozilla.com/D184810
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 31, 2023
…om highlight API tests.

As per [0], [1] and [2], `getComputedStyle()` should not return the
default style when the pseudo is invalid.
Instead, an empty style should be returned.

[0] https://drafts.csswg.org/cssom/#extensions-to-the-window-interface
[1] w3c/csswg-drafts#6501
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1726396

Differential Revision: https://phabricator.services.mozilla.com/D184810

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1838262
gecko-commit: 1c145766032f7bbdecfd68895b7e3038e923100b
gecko-reviewers: emilio
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Aug 1, 2023
…le()` in custom highlight API tests. r=emilio

As per [0], [1] and [2], `getComputedStyle()` should not return the
default style when the pseudo is invalid.
Instead, an empty style should be returned.

[0] https://drafts.csswg.org/cssom/#extensions-to-the-window-interface
[1] w3c/csswg-drafts#6501
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1726396

Differential Revision: https://phabricator.services.mozilla.com/D184810

UltraBlame original commit: 1c145766032f7bbdecfd68895b7e3038e923100b
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Aug 1, 2023
…le()` in custom highlight API tests. r=emilio

As per [0], [1] and [2], `getComputedStyle()` should not return the
default style when the pseudo is invalid.
Instead, an empty style should be returned.

[0] https://drafts.csswg.org/cssom/#extensions-to-the-window-interface
[1] w3c/csswg-drafts#6501
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1726396

Differential Revision: https://phabricator.services.mozilla.com/D184810

UltraBlame original commit: 1c145766032f7bbdecfd68895b7e3038e923100b
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Aug 1, 2023
…le()` in custom highlight API tests. r=emilio

As per [0], [1] and [2], `getComputedStyle()` should not return the
default style when the pseudo is invalid.
Instead, an empty style should be returned.

[0] https://drafts.csswg.org/cssom/#extensions-to-the-window-interface
[1] w3c/csswg-drafts#6501
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1726396

Differential Revision: https://phabricator.services.mozilla.com/D184810

UltraBlame original commit: 1c145766032f7bbdecfd68895b7e3038e923100b
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Aug 4, 2023
…le()` in custom highlight API tests. r=emilio

As per [0], [1] and [2], `getComputedStyle()` should not return the
default style when the pseudo is invalid.
Instead, an empty style should be returned.

[0] https://drafts.csswg.org/cssom/#extensions-to-the-window-interface
[1] w3c/csswg-drafts#6501
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1726396

Differential Revision: https://phabricator.services.mozilla.com/D184810
Lightning00Blade pushed a commit to Lightning00Blade/wpt that referenced this issue Dec 11, 2023
…om highlight API tests.

As per [0], [1] and [2], `getComputedStyle()` should not return the
default style when the pseudo is invalid.
Instead, an empty style should be returned.

[0] https://drafts.csswg.org/cssom/#extensions-to-the-window-interface
[1] w3c/csswg-drafts#6501
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1726396

Differential Revision: https://phabricator.services.mozilla.com/D184810

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1838262
gecko-commit: 1c145766032f7bbdecfd68895b7e3038e923100b
gecko-reviewers: emilio
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cssom-1 Current Work
Projects
None yet
4 participants