1255379 - inIDOMUtils.getCSSValuesForProperty() is missing keywords for 'clip-path'

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Sebastian Zartner [:sebo]]

This property is missing the 'auto' keyword as well as the rect() function.

Test case (to execute in Scratchpad):

let DOMUtils = Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);

DOMUtils.getCSSValuesForProperty("clip");

Sebastian

Comment 1

[Tom Tromey :tromey]

According to MDN, clip is deprecated and going to be removed; but clip-path (the recommended
replacement) also has completion issues.

Comment 2

[Sebastian Zartner [:sebo]]

You're right. 'clip' is deprecated, so it actually helps to drop its usage by not providing autocompletion for it.
I've also filed an issue for the spec. now asking to remove the requirement for usage agents to support the property.[1]

Anyway, I've turned this report into an issue for 'clip-path' now. So, the autocompletion should include these values and functions:

none
url()
fill-box
stroke-box
view-box
margin-box
border-box
padding-box
content-box
inset()
circle()
polygon()

(The function parentheses are actually part of bug 1430558.)

Sebastian

[1] https://github.com/w3c/fxtf-drafts/issues/248

Comment 3

[Tom Tromey :tromey]

I actually implemented clip as well, since it was easy.
I'll upload it once I figure out who to give the review to.

Comment 4

[Tom Tromey :tromey]

Attachment: Bug 1255379 - fix getCSSValuesForProperty for clip and clip-path;

This fixes getCSSValuesForProperty for clip and clip-path.

Although clip is deprecated, it seemed harmless to fix it as well;
when it is removed this code will also be readily found and removed.

It wasn't clear to me if setting the keyword table for clip-path in
nsCSSPropList.h would be ok, so I added a special case in
InspectorUtils.

Review commit: https://reviewboard.mozilla.org/r/215716/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215716/

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8945579 [details]
Bug 1255379 - fix getCSSValuesForProperty for clip and clip-path;

https://reviewboard.mozilla.org/r/215716/#review221442


Static analysis found 6 defects in this patch.
 - 6 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/inspector/tests/test_bug877690.html:200
(Diff revision 1)
>    var expected = [ "inherit", "initial", "unset", "inset", "none", "calc", ...allColors ];
>    var values = InspectorUtils.getCSSValuesForProperty("box-shadow");
>    ok(testValues(values, expected), "property box-shadow's values");
>  
> +  // Regression test for bug 1255379.
> +  var expected = [ "inherit", "initial", "unset", "none", "url",

Error: 'expected' is already defined. [eslint: no-redeclare]

::: layout/inspector/tests/test_bug877690.html:204
(Diff revision 1)
> +  // Regression test for bug 1255379.
> +  var expected = [ "inherit", "initial", "unset", "none", "url",
> +                   "polygon", "circle", "ellipse", "inset",
> +                   "fill-box", "stroke-box", "view-box", "margin-box",
> +                   "border-box", "padding-box", "content-box" ];
> +  var values = InspectorUtils.getCSSValuesForProperty("clip-path");

Error: 'values' is already defined. [eslint: no-redeclare]

::: layout/inspector/tests/test_bug877690.html:205
(Diff revision 1)
> +  var expected = [ "inherit", "initial", "unset", "none", "url",
> +                   "polygon", "circle", "ellipse", "inset",
> +                   "fill-box", "stroke-box", "view-box", "margin-box",
> +                   "border-box", "padding-box", "content-box" ];
> +  var values = InspectorUtils.getCSSValuesForProperty("clip-path");
> +  ok(testValues(values, expected), "property clip-path's values");

Error: 'ok' is not defined. [eslint: no-undef]

::: layout/inspector/tests/test_bug877690.html:207
(Diff revision 1)
> +                   "fill-box", "stroke-box", "view-box", "margin-box",
> +                   "border-box", "padding-box", "content-box" ];
> +  var values = InspectorUtils.getCSSValuesForProperty("clip-path");
> +  ok(testValues(values, expected), "property clip-path's values");
> +
> +  var expected = [ "inherit", "initial", "unset", "auto", "rect" ];

Error: 'expected' is already defined. [eslint: no-redeclare]

::: layout/inspector/tests/test_bug877690.html:208
(Diff revision 1)
> +                   "border-box", "padding-box", "content-box" ];
> +  var values = InspectorUtils.getCSSValuesForProperty("clip-path");
> +  ok(testValues(values, expected), "property clip-path's values");
> +
> +  var expected = [ "inherit", "initial", "unset", "auto", "rect" ];
> +  var values = InspectorUtils.getCSSValuesForProperty("clip");

Error: 'values' is already defined. [eslint: no-redeclare]

::: layout/inspector/tests/test_bug877690.html:209
(Diff revision 1)
> +  var values = InspectorUtils.getCSSValuesForProperty("clip-path");
> +  ok(testValues(values, expected), "property clip-path's values");
> +
> +  var expected = [ "inherit", "initial", "unset", "auto", "rect" ];
> +  var values = InspectorUtils.getCSSValuesForProperty("clip");
> +  ok(testValues(values, expected), "property clip's values");

Error: 'ok' is not defined. [eslint: no-undef]

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8945579 [details]
Bug 1255379 - fix getCSSValuesForProperty for clip and clip-path;

https://reviewboard.mozilla.org/r/215716/#review222190

It looks fine to me, but I'm not super familiar with inspector stuff. There is definitely better way to do this change, but I doubt whether it's worth at the moment.

We should probably port the whole thing to stylo at some point in bug 1434130.

::: layout/inspector/InspectorUtils.cpp:485
(Diff revision 1)
> +    InsertNoDuplicates(aArray, NS_LITERAL_STRING("circle"));
> +    InsertNoDuplicates(aArray, NS_LITERAL_STRING("ellipse"));
> +    InsertNoDuplicates(aArray, NS_LITERAL_STRING("inset"));
> +    InsertNoDuplicates(aArray, NS_LITERAL_STRING("polygon"));

A better way would probably be adding a flag `VARIANT_BASIC_SHAPE` and add this list for that, so that this can be shared between `clip-path` and `shape-outside`. Not sure how much it's worth...

Probably not worth at this moment as we don't want to add new things to the old style system.

Comment 7

[Tom Tromey :tromey]

(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #6)

Thanks for the review.

> We should probably port the whole thing to stylo at some point in bug
> 1434130.

I totally agree, but I'm only fixing these bugs as a kind of hobby, and don't
really have the time to take on a big task like that.
At least, hopefully, the tests will survive to help with the transition.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3d5c5af4582
fix getCSSValuesForProperty for clip and clip-path; r=xidorn

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d3d5c5af4582

Comment 10

[Sebastian Zartner [:sebo]]

From a quick check it works fine for me using Nightly 60.0a1 (2018-02-01). Thanks for that!

Sebastian