Bug 210695 - CSS logical longhands are not enumerated in the computed style.
Summary: CSS logical longhands are not enumerated in the computed style.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Emilio Cobos Álvarez (:emilio)
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks: 210707
  Show dependency treegraph
 
Reported: 2020-04-18 12:31 PDT by Emilio Cobos Álvarez (:emilio)
Modified: 2020-04-19 06:46 PDT (History)
10 users (show)

See Also:


Attachments
Patch (16.06 KB, patch)
2020-04-18 17:38 PDT, Emilio Cobos Álvarez (:emilio)
no flags Details | Formatted Diff | Diff
Patch (16.16 KB, patch)
2020-04-18 17:40 PDT, Emilio Cobos Álvarez (:emilio)
no flags Details | Formatted Diff | Diff
Patch for landing (13.12 KB, patch)
2020-04-19 04:56 PDT, Emilio Cobos Álvarez (:emilio)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emilio Cobos Álvarez (:emilio) 2020-04-18 12:31:20 PDT
It contains only the shorthands, e.g.:

> [...getComputedStyle(document.body)].filter(prop => prop.includes("block"))

["border-block", "border-block-color", "border-block-end", "border-block-start", "border-block-style", "border-block-width", "inset-block", "margin-block", "padding-block"]

Per spec it should contain logical longhands as well.
Comment 1 Emilio Cobos Álvarez (:emilio) 2020-04-18 12:34:58 PDT
The relevant spec is https://drafts.csswg.org/cssom/#supported-css-property:

> The term supported CSS property refers to a CSS property that the user agent implements, including any vendor-prefixed properties, but excluding custom properties. A supported CSS property must be in its lowercase form for the purpose of comparisons in this specification.

And https://drafts.csswg.org/cssom/#dom-window-getcomputedstyle:

>  set decls to a list of all longhand properties that are supported CSS properties, in lexicographical order, with the value being the resolved value computed for obj using the style rules associated with doc.

So I guess enumerating shorthands altogether is a bit bogus.
Comment 2 Emilio Cobos Álvarez (:emilio) 2020-04-18 12:38:36 PDT
Ah, so I think I know where this goes wrong. This is indeed a bug, the intention is that shorthands are not emitted looking at makeprop.pl
Comment 3 Radar WebKit Bug Importer 2020-04-18 12:52:40 PDT
<rdar://problem/61980678>
Comment 4 Emilio Cobos Álvarez (:emilio) 2020-04-18 15:27:59 PDT
I think I can write the fix for this, just having some issues running tests locally :)
Comment 5 Oriol Brufau 2020-04-18 15:58:24 PDT
Dang, I think I checked `Reflect.ownKeys(computedStyle)` but forgot about `[...computedStyle]`.
Comment 6 Emilio Cobos Álvarez (:emilio) 2020-04-18 17:38:03 PDT
Created attachment 396878 [details]
Patch
Comment 7 Emilio Cobos Álvarez (:emilio) 2020-04-18 17:40:52 PDT
Created attachment 396879 [details]
Patch
Comment 8 Oriol Brufau 2020-04-18 17:47:17 PDT
Comment on attachment 396879 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396879&action=review

> LayoutTests/ChangeLog:8
> +        Add a test enumerating all entries in the style. This is a bit

I tried to do the same when I added the option to hide a CSS property behind a runtime flag.
But the problem is that the boots run the test in multiple platforms (windows, linux, and multiple mac versions).
Each platform has its own default compile flags which can show or hide CSS properties.
So you need lots of different expectations, which seemed too annoying.
Comment 9 Emilio Cobos Álvarez (:emilio) 2020-04-18 17:51:08 PDT
What about just omitting the platform-dependent properties? Seems it'd get most of the benefit and none of the pain.
Comment 10 Antti Koivisto 2020-04-18 22:55:17 PDT
Comment on attachment 396879 [details]
Patch

It would probably be good to keep these enumeration tests to minimum due to maintenance needs. There appears to be LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml.html already.
Comment 11 Emilio Cobos Álvarez (:emilio) 2020-04-19 04:56:09 PDT
Created attachment 396902 [details]
Patch for landing
Comment 12 Emilio Cobos Álvarez (:emilio) 2020-04-19 04:56:45 PDT
(In reply to Antti Koivisto from comment #10)
> It would probably be good to keep these enumeration tests to minimum due to
> maintenance needs. There appears to be
> LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-
> xml.html already.

Yes, I updated that test instead and removed the one I added. Thank you for the review!
Comment 13 EWS 2020-04-19 06:46:19 PDT
Committed r260335: <https://trac.webkit.org/changeset/260335>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396902 [details].