Closed Bug 137688 Opened 23 years ago Closed 4 years ago

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

Categories

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

Product:
Core
Component:
DOM: CSS Object Model
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
96 Branch
Project Flags:
Webcompat Priority P2
Tracking Flags:
Tracking Status
firefox96 --- fixed

People

(Reporter: bzbarsky, Assigned: emilio)

References

(Blocks 4 open bugs,
URL
)

Details

(Whiteboard: [wpt-triaged])

Attachments

(2 files)

test.html
437 bytes, text/html
Details
Bug 137688 - Serialize shorthands in getComputedStyle. r=#style,#layout-reviewers
48 bytes, text/x-phabricator-request
Details | Review
nsComputedDOMStyle should handle getPropertyValue asking for shorthands
Priority: -- → P5
Target Milestone: --- → Future
Blocks: 200499
Blocks: 201830
No plans to work on this any time in the foreseeable future, so to default owner.
Assignee: bz-vacation → general
Priority: P5 → --
Target Milestone: Future → ---
Assignee: general → nobody
QA Contact: ian → general
QA Contact: general → style-system
Attached file test.html —
10 years later... Still no plan on fixing the bug?
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.
Where is the spec being worked on, and is it possible to see the progress?
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".
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?
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.
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.
Priority: -- → P3
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
Depends on: 1542178
See Also: → 1574222

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

Whiteboard: [wpt-triaged]

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

Summary: getPropertyValue on computed style does not do shorthand properties → getPropertyValue on computed style (getComputedStyle) does not do shorthand properties
QA Contact: boris.chiou
Assignee: nobody → boris.chiou
QA Contact: boris.chiou

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.

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.

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).

Assignee: boris.chiou → nobody
Webcompat Priority: --- → ?

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.

Webcompat Priority: ? → P2

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?)

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
Depends on: 1738658
Depends on: 1738663
Assignee: nobody → emilio
Depends on: 1743150

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.

Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8786e120af24 Serialize shorthands in getComputedStyle. r=xidorn
Pushed by malexandru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/57f8c783bb2b Modify expectancies for table-attribute.html a=wpt-fix

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

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox96: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: