137688 - getPropertyValue on computed style (getComputedStyle) does not do shorthand properties

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

nsComputedDOMStyle should handle getPropertyValue asking for shorthands

Comment 1

[Boris Zbarsky [:bzbarsky]]

No plans to work on this any time in the foreseeable future, so to default owner.

Comment 5

[Mads Konradsen]

Attachment: test.html

Comment 6

[Mads Konradsen]

10 years later... Still no plan on fixing the bug?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Not until there's a spec for the behavior, at least, no, because until then there's no way to implement this: the behavior is not define.  The spec is being worked on.

Comment 8

[Mads Konradsen]

Where is the spec being worked on, and is it possible to see the progress?

Comment 9

[Boris Zbarsky [:bzbarsky]]

http://dev.w3.org/csswg/cssom/#serializing-css-values

Comment 10

[Boris Zbarsky [:bzbarsky]]

And note that there are computed values of some shorthands that cannot actually be represented as a value for the shorthand.  Not an issue for "padding", but definitely an issue for "border".

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that getPropertyCSSValue is supposed to return null for shorthands -- which is what we do for the shorthands that have always been shorthands -- but not the ones that were originally longhands.

There are plenty of non-representable values of shorthands -- that happens for specified style too -- and I believe the spec says to handle it the same way, by serializing to the empty string:
https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-getpropertyvalue refers to
https://drafts.csswg.org/cssom/#serialize-a-css-value which says:
"If shorthand cannot represent the values of list in its grammar, return the empty string and terminate these steps."
(and that wording covers both computed and specified style).


That said:  what do other browsers do here?  Will we hit compatibility problems if we start supporting shorthands?

Comment 15

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

It looks like Chrome and Safari supports shorthands currently. I'm able to get appropriate serialized value on them. I think we should do the same too.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

This is sort-of easily-implementable without duplicating a lot of code by using the "uncomputing" machinery we already have for animations, and constructing a temporary declaration block to try serializing the shorthand with all the relevant declarations, I guess.

Comment 21

[Brian Birtles (:birtles, Feb 26~ parental leave)]

As per [1] the CSSWG has resolved: "all shorthands must show up in getComputedStyles". The spec has yet to be updated to reflect the resolution, however.

[1] https://github.com/w3c/csswg-drafts/issues/2529#issuecomment-402386896

Comment 23

[Daniel Holbert [:dholbert]]

For the record, this bug is the reason that we (alone) fail these WPT tests:
https://wpt.fyi/results/css/css-align/gaps/gap-animation-001.html
https://wpt.fyi/results/css/css-align/gaps/gap-animation-002.html
https://wpt.fyi/results/css/css-align/gaps/gap-animation-003.html
https://wpt.fyi/results/css/css-align/gaps/gap-animation-004.html

Comment 25

[Boris Chiou [:boris]]

I may take a look at this once I finish Bug 1574222.

Comment 26

[Daniel Holbert [:dholbert]]

[tweaking title to mention "getComputedStyle", for bugzilla-search-discoverability purposes]

Comment 27

[Boris Chiou [:boris]]

I'm thinking about what should we do for this bug. Perhaps we should drop SHORTHAND_IN_GETCS from ServoCSSPropList.mako.py, which makes "shorthand in gCS" be a default behavior.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

There are various different cases. The current shorthand-in-getcs code doesn't work for all properties. In particular, this code won't do the right thing for:

• Properties that depend on layout (like padding and margin).
• Properties that are logical / non-animatable.

I haven't thought all this through, but for the first, I think ideally we'd fix ToResolvedValue for the relevant bits so that we can also remove all the C++-side implementations and CSSValue and so on. That may be a bit of work though, and depending on how many shorthands of the layout-affecting properties are there, we may want to punt on it and just implement them on C++. However it seems like there's a fair amount of them.

For the second we just need more generated code to go from resolved to specified value. Right now that code is reusing AnimatedValue::from_computed_value() and then AnimatedValue::uncompute. This makes assumptions that don't hold for all properties:

• That resolved and computed values are the same.
• That all properties are animatable.

All those assumptions don't hold for all shorthands. They hold for the shorthands we support right now though.

Comment 29

[Christian Bromann]

This bug is also problematic for WebDriver tests where "Get Element CSS Value" is being called with a shorthand css property resulting in different results for Chrome and Firefox (see e.g.: https://github.com/webdriverio/webdriverio/issues/6161).

Comment 31

[Daniel Holbert [:dholbert]]

FWIW, this bug seems to be the root cause of some of the test-failures in tests on the compat2021 list:
https://wpt.fyi/results/css/css-flexbox/parsing/flex-computed.html?label=master&label=experimental&aligned
https://wpt.fyi/results/css/css-flexbox/parsing/flex-flow-computed.html?label=master&label=experimental&aligned
https://wpt.fyi/results/css/css-grid/parsing/grid-area-computed.html?label=master&label=experimental&aligned

Comment 34

[Karl Dubost💡 :karlcow]

The Webcompat team is not sure about it. There's only one restaurant site breaking, but there seems to be duplicate from devs reporting the issue.

Comment 35

[Mats Palmgren (inactive)]

As Daniel noted above, we do fail a large number of WPTs (unnecessarily) because of this bug, which makes us look bad in "Web Standards Compliance" scores. So maybe it doesn't impact our users that much directly, it's still bad for our reputation to have low scores on "number of WPTs passed" IMO.

Is the spec clear nowadays how this should be implemented?
(I seem to recall it was discussed/resolved somewhat recently in a csswg issue?)

Comment 36

[Karl Dubost💡 :karlcow]

maybe
https://github.com/w3c/csswg-drafts/issues/2529
https://github.com/webdriverio/webdriverio/issues/6161

https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-getpropertyvalue

The spec seems clear that it should return the full list of values.

1. If property is a shorthand property, then follow these substeps:
   1. Let list be a new empty array.
   2. For each longhand property longhand that property maps to, in canonical order, follow these substeps:
      1. If longhand is a case-sensitive match for a property name of a CSS declaration in the declarations, let declaration be that CSS declaration, or null otherwise.
      2. If declaration is null, then return the empty string.
      3. Append the declaration to list.

Just tested.

data:text/html,<!doctype html> <p id="log" style="border: 1px solid red">border</p> <script> var log = document.getElementById('log'); log.textContent = window.getComputedStyle(log).getPropertyValue('border'); </script>

• FAIL Gecko Firefox Nightly 95.0a1 (2021-10-31)
• PASS WebKit Safari Tech Preview Release 133 (Safari 15.4, WebKit 17613.1.2.2)
• PASS Blink Edge Canary 97.0.1060.0

Comment 37

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 137688 - Serialize shorthands in getComputedStyle. r=#style,#layout-reviewers

There's still some potential work to do for layout-dependent shorthands, but I
believe given there's only progressions here this is worth doing.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8786e120af24
Serialize shorthands in getComputedStyle. r=xidorn

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57f8c783bb2b
Modify expectancies for table-attribute.html a=wpt-fix

Comment 40

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/8786e120af24
https://hg.mozilla.org/mozilla-central/rev/57f8c783bb2b