Bug 239669 – [cssom] Iterating computed style should not include 'all' shorthand

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Oriol Brufau]

A computed style should only list declarations for longhand properties, e.g.

    var cs = getComputedStyle(document.body);
    var longhands = new Set(cs);
    longhands.has("margin"); // false
    longhands.has("margin-left"); // true

However, the 'all' shorthand is included:

    longhands.has("all"); // true, should be false

Same with item():

    cs.item(4); // "alignment-baseline"
    cs.item(5); // "all"
    cs.item(6); // "alt"

See 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.
> Additionally, append to decls all the custom properties whose computed
> value for obj is not the guaranteed-invalid value.

getComputedStyle().all should continue working of course, but it shouldn't be indexed.

[Oriol Brufau]

Created attachment 458588 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/92538720>

[Darin Adler]

Comment on attachment 458588 [details]
Patch

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

> Source/WebCore/css/makeprop.pl:211
> +  if ($name eq "all") {
> +    return 1;
> +  }

Two small thoughts:

1) Is there any way to do this with a property rather than literally hardwiring "all"?
2) Since this is perl, there is a syntax like:

    return 1 if $name eq "all";

[Oriol Brufau]

(In reply to Darin Adler from comment #3)
> Comment on attachment 458588 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458588&action=review
> 
> > Source/WebCore/css/makeprop.pl:211
> > +  if ($name eq "all") {
> > +    return 1;
> > +  }
> 
> Is there any way to do this with a property rather than literally
> hardwiring "all"?

Do you mean in CSSProperties.json?
I guess I could add some flag, Blink uses "computable".

But "all" is special anyways, since it has

            "codegen-properties": {
                "longhands": [
                    "all"
                ]
            },

which is already hardwired:

    if ($_ eq "all") {
        foreach my $propname (@names) {
            next if (exists $propertiesWithStyleBuilderOptions{$propname}{"longhands"});
            next if ($propname eq "direction" || $propname eq "unicode-bidi");
            die "Unknown CSS property used in all shorthand: $propname" if !exists($nameToId{$propname});
            push(@{$longhandToShorthands{$propname}}, $name);
            print SHORTHANDS_CPP "        CSSProperty" . $nameToId{$propname} . ",\n";
        }
    }

[Oriol Brufau]

Created attachment 458702 [details]
Patch

[Oriol Brufau]

(In reply to Darin Adler from comment #3)
> Comment on attachment 458588 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458588&action=review
>
> 1) Is there any way to do this with a property rather than literally
> hardwiring "all"?

Adding some way to do that can be discussed in bug 239670.

> 2) Since this is perl, there is a syntax like:
> 
>     return 1 if $name eq "all";

Done, and added comment.

[Oriol Brufau]

Created attachment 458704 [details]
Patch

[EWS]

Committed r293689 (250187@main): <https://commits.webkit.org/250187@main>

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