-
Notifications
You must be signed in to change notification settings - Fork 679
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] getComputedStyle and shorthands. #2529
Comments
cc @ericwilligers, which was filing bugs related to this. |
This is very related to #1033. |
I think "yes" looked like a not-great idea when css started. But now that shorthands can have values that you cannot expose in the longhands (like |
Variables cannot appear in result from getComputedStyle, so that's not an issue. For Firefox, we do have a special case that system font on |
"Shorthands aren't present" was always a design mistake - it meant that the shape of the gCS result changes when we break a longhand apart into further sub-properties, "upgrading" it into a shorthand, so userland code depending on that property would break. Shorthands should always have been present in gCS, and we should update the spec to say so; the fact that we already have an impl doing this is a nice plus. (The fact that the non-exposing browsers have an exception for one of the "upgraded" shorthands shows the problem with not exposing them from the get-go.) |
I have an impression that not having shorthands in gCS result was also intended for forward compatibility. There might be a discussion with @dbaron three years ago, but I don't recall the reasoning exactly. I would guess the reason is something like, when a new longhand property is added to a shorthand, and the new property contains some value which is not expressible in the shorthand, the shorthand may fail to serialize. If there is code relying on serialization of shorthand, they may fail unexpectedly in browsers with the new property, leading to webcompat issue and making it harder to ship some new properties. Obvious past examples may include This argument may also apply to specified value as well, but my understanding is that computed value is usually more useful to interpret for scripts (since that's a step closer to the value engine actually uses), while specified value is often just used for setting. That means keeping serialization forward and backward compatible is probably more important for gCS than for specified value. |
While that's a reasonable concern, it's much less important for forward-compat than the one I gave. "Don't serialize shorthands at all" means you'll automatically break code that depends on the serialization of that property as soon as you ship the change turning a longhand into a shorthand. "Serialize shorthands" breaks zero code when you ship the new feature; it only breaks stuff if the page is further updated to take advantage of the new, unserializable, feature. At that point the maintainer can just fix that code as well (or otherwise work around it), since they're already there altering the page. |
I agree that "don't serialize shorthands at all" is definitely not something we want. We at least need to serialize shorthands which were longhands. |
Right, but "dont' serialize this seemingly-random set of properties with no clear grouping" isn't nearly as convincing. ^_^ |
I'd like to discuss this at the F2F, given everyone would be around. |
The Working Group just discussed
The full IRC log of that discussion<florian> topic: getComputedStyles and shorthands<florian> github: https://github.com//issues/2529 <florian> emilio: FF and Edge only support getComputedStyle on shorthands that used to be longhands, Blink supports more <florian> emilio: shorthands in the computed style object are exposed as properties <florian> emilio: I want to know what people want. Fine with changing, but it's work <florian> ericwilligers: if it was a longhand, it would serialize <florian> TabAtkins: it needs to <florian> emilio: there are bugs on the grid shorthands <florian> emilio: People have argued various ways. <florian> TabAtkins: this is an obvious forward compat mistake <florian> TabAtkins: all properties from now on should support it <florian> TabAtkins: and if we can, all the legacy shorthands should as well, when a value can be constructed if it's not a a webcompat problem <florian> TabAtkins: which doesn't seem to be the case <florian> dbaron: if it's not possible to represent, then we have to go for empty string <florian> TabAtkins: that's fine <TabAtkins> It's still forward-compatible; adding a new property to the shorthand will never make it, by *default*, stop serializing <florian> emilio: there's also the question of computed values vs resolved values <florian> heycam: it would be very weird if the longhands resolved but the shorthands didn't, so we should match <florian> heycam: Edge is the only other browser not doing it. So Edge? <florian> emilio: Edge, are you OK with doing it? the only ones you have on top of used-to-be-longhands are grid properties. <florian> Rossen: I don't believe I've seen compat issues. I am reluctant to write a bunch of code for something that's not needed. <florian> TabAtkins: going forward, we need <florian> s/we need/we need to./ <florian> TabAtkins: for legacy props, it wouldn't be the end of the world if we didn't do it, but it would be inconsistant and unfortunate. <florian> emilio: It can be done quite easily for lots of properties <florian> Florian: can we resolve to do on all, even if priorities means it might take a while <florian> Rossen: we need consistency, so let's have it everywhere. <florian> RESOLVED: all shorthands must show up in getComputedStyles <florian> xidorn: I wonder if you should see the shorthands in the iterator <florian> xidorn: I think there's a usecase for copying computed values from one element to another, and they just iterate through <florian> myles: This should be a separate issue. <florian> [mumble mumble] |
This discrepency was discovered in a Fullscreen test: #13102 The spec issue for resolving this is: w3c/csswg-drafts#2529
I depended on this difference in behavior accidentally in a Fullscreen test, which @upsuper fixed in web-platform-tests/wpt#13102. I've created web-platform-tests/wpt#13238 as a test for that issue specifically. When this issue is resolved, please be sure to also merge that or a similar test. Filing bugs on the browsers that fail it would be great too. |
These tests are made to start after onload so that iframe content doesn't change anymore: * fullscreen/api/element-ready-check-containing-iframe-manual.html * fullscreen/api/element-ready-check-not-allowed-manual.html * fullscreen/api/element-request-fullscreen-and-exit-iframe-manual.html * fullscreen/model/move-to-fullscreen-iframe-manual.html fullscreen/rendering/ua-style-iframe-manual.html is updated to check each subproperties individually rather than shorthand properties. fullscreen/api/element-request-fullscreen-timing-manual.html is changed so that the second test starts after an animation frame to work around Gecko throttling rAF before the first paint. Related bugs: https://bugzilla.mozilla.org/show_bug.cgi?id=1493878 w3c/csswg-drafts#2529
…, a=testonly Automatic update from web-platform-testsSeveral fixes to manual fullscreen tests (#13102) These tests are made to start after onload so that iframe content doesn't change anymore: * fullscreen/api/element-ready-check-containing-iframe-manual.html * fullscreen/api/element-ready-check-not-allowed-manual.html * fullscreen/api/element-request-fullscreen-and-exit-iframe-manual.html * fullscreen/model/move-to-fullscreen-iframe-manual.html fullscreen/rendering/ua-style-iframe-manual.html is updated to check each subproperties individually rather than shorthand properties. fullscreen/api/element-request-fullscreen-timing-manual.html is changed so that the second test starts after an animation frame to work around Gecko throttling rAF before the first paint. Related bugs: https://bugzilla.mozilla.org/show_bug.cgi?id=1493878 w3c/csswg-drafts#2529 -- wpt-commits: b3c7bf4261939cd1aba9b3264152a718b634ffa7 wpt-pr: 13102
…, a=testonly Automatic update from web-platform-testsSeveral fixes to manual fullscreen tests (#13102) These tests are made to start after onload so that iframe content doesn't change anymore: * fullscreen/api/element-ready-check-containing-iframe-manual.html * fullscreen/api/element-ready-check-not-allowed-manual.html * fullscreen/api/element-request-fullscreen-and-exit-iframe-manual.html * fullscreen/model/move-to-fullscreen-iframe-manual.html fullscreen/rendering/ua-style-iframe-manual.html is updated to check each subproperties individually rather than shorthand properties. fullscreen/api/element-request-fullscreen-timing-manual.html is changed so that the second test starts after an animation frame to work around Gecko throttling rAF before the first paint. Related bugs: https://bugzilla.mozilla.org/show_bug.cgi?id=1493878 w3c/csswg-drafts#2529 -- wpt-commits: b3c7bf4261939cd1aba9b3264152a718b634ffa7 wpt-pr: 13102
…, a=testonly Automatic update from web-platform-testsSeveral fixes to manual fullscreen tests (#13102) These tests are made to start after onload so that iframe content doesn't change anymore: * fullscreen/api/element-ready-check-containing-iframe-manual.html * fullscreen/api/element-ready-check-not-allowed-manual.html * fullscreen/api/element-request-fullscreen-and-exit-iframe-manual.html * fullscreen/model/move-to-fullscreen-iframe-manual.html fullscreen/rendering/ua-style-iframe-manual.html is updated to check each subproperties individually rather than shorthand properties. fullscreen/api/element-request-fullscreen-timing-manual.html is changed so that the second test starts after an animation frame to work around Gecko throttling rAF before the first paint. Related bugs: https://bugzilla.mozilla.org/show_bug.cgi?id=1493878 w3c/csswg-drafts#2529 -- wpt-commits: b3c7bf4261939cd1aba9b3264152a718b634ffa7 wpt-pr: 13102 UltraBlame original commit: 4c97e39c26a7f0ea4a0714fc8a7c77e21734ed0e
…, a=testonly Automatic update from web-platform-testsSeveral fixes to manual fullscreen tests (#13102) These tests are made to start after onload so that iframe content doesn't change anymore: * fullscreen/api/element-ready-check-containing-iframe-manual.html * fullscreen/api/element-ready-check-not-allowed-manual.html * fullscreen/api/element-request-fullscreen-and-exit-iframe-manual.html * fullscreen/model/move-to-fullscreen-iframe-manual.html fullscreen/rendering/ua-style-iframe-manual.html is updated to check each subproperties individually rather than shorthand properties. fullscreen/api/element-request-fullscreen-timing-manual.html is changed so that the second test starts after an animation frame to work around Gecko throttling rAF before the first paint. Related bugs: https://bugzilla.mozilla.org/show_bug.cgi?id=1493878 w3c/csswg-drafts#2529 -- wpt-commits: b3c7bf4261939cd1aba9b3264152a718b634ffa7 wpt-pr: 13102 UltraBlame original commit: 4c97e39c26a7f0ea4a0714fc8a7c77e21734ed0e
…, a=testonly Automatic update from web-platform-testsSeveral fixes to manual fullscreen tests (#13102) These tests are made to start after onload so that iframe content doesn't change anymore: * fullscreen/api/element-ready-check-containing-iframe-manual.html * fullscreen/api/element-ready-check-not-allowed-manual.html * fullscreen/api/element-request-fullscreen-and-exit-iframe-manual.html * fullscreen/model/move-to-fullscreen-iframe-manual.html fullscreen/rendering/ua-style-iframe-manual.html is updated to check each subproperties individually rather than shorthand properties. fullscreen/api/element-request-fullscreen-timing-manual.html is changed so that the second test starts after an animation frame to work around Gecko throttling rAF before the first paint. Related bugs: https://bugzilla.mozilla.org/show_bug.cgi?id=1493878 w3c/csswg-drafts#2529 -- wpt-commits: b3c7bf4261939cd1aba9b3264152a718b634ffa7 wpt-pr: 13102 UltraBlame original commit: 4c97e39c26a7f0ea4a0714fc8a7c77e21734ed0e
This inconsistency also affects WebDriver tests where users try to get the shorthand CSS property of an element resulting in different responses, see webdriverio/webdriverio#6161. |
I'm closing web-platform-tests/wpt#13238 as it's blocked on this issue, but when this is resolved it could be a starting point for tests. |
@foolip there's a resolution above (edits still needed), or what am I missing? |
This doesn't actually follow from the current spec; partial interface CSSStyleDeclaration {
[CEReactions] attribute [LegacyNullToEmptyString] CSSOMString font;
}; It does, however, mean that
|
https://bugs.webkit.org/show_bug.cgi?id=234785 Reviewed by Sam Weinig. LayoutTests/imported/w3c: Mark WPT progressions. * web-platform-tests/css/css-animations/computed-style-animation-parsing-expected.txt: * web-platform-tests/css/css-animations/parsing/animation-computed-expected.txt: * web-platform-tests/css/css-pseudo/parsing/marker-supported-properties-expected.txt: Source/WebCore: There is an existing WPT for the "animation" shorthand in the computed style which we used to fail because we would simply not do any work to return the longhands compiled into a list. It seems that the CSS WG, per w3c/csswg-drafts#2529, is moving in the direction of specifying what happens with shorthands in computed style, so we're adding support for the "animation" shorthand. * css/CSSComputedStyleDeclaration.cpp: (WebCore::animationShorthandValue): (WebCore::ComputedStyleExtractor::valueForPropertyInStyle): Canonical link: https://commits.webkit.org/245670@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@287535 268f45cc-cd09-0410-ab3c-d52691b4dbfc
https://bugs.webkit.org/show_bug.cgi?id=234785 Reviewed by Sam Weinig. LayoutTests/imported/w3c: Mark WPT progressions. * web-platform-tests/css/css-animations/computed-style-animation-parsing-expected.txt: * web-platform-tests/css/css-animations/parsing/animation-computed-expected.txt: * web-platform-tests/css/css-pseudo/parsing/marker-supported-properties-expected.txt: Source/WebCore: There is an existing WPT for the "animation" shorthand in the computed style which we used to fail because we would simply not do any work to return the longhands compiled into a list. It seems that the CSS WG, per w3c/csswg-drafts#2529, is moving in the direction of specifying what happens with shorthands in computed style, so we're adding support for the "animation" shorthand. * css/CSSComputedStyleDeclaration.cpp: (WebCore::animationShorthandValue): (WebCore::ComputedStyleExtractor::valueForPropertyInStyle): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@287535 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@emilio per my prior comment, what edit do you believe is actually needed? |
I think given that the spec (and browsers it seems) are correct |
Currently https://drafts.csswg.org/cssom/#dom-window-getcomputedstyle says:
Which means that
getComputedStyle(document.documentElement).font
shouldn't be present, since it's a shorthand.This is inconsistent with https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-getpropertyvalue, though which handles shorthands.
Should
ComputedStyle.getPropertyValue
handle shorthands? If so, should they be exposed as a property in the declaration object?Right now answers from browsers to "does
getPropertyValue
handle shorthands?", and "are shorthands exposed as a property?" are:What should happen here?
Note that both FF and Edge have a special-case for
overflow
, which used to be a longhand.The text was updated successfully, but these errors were encountered: