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

VERIFIED FIXED in Firefox 60

Status

()

defect
VERIFIED FIXED
3 years ago
10 months ago

People

(Reporter: sebo, Assigned: tromey)

Tracking

(Blocks 1 bug)

Trunk
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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
(Assignee)

Comment 1

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

Updated

a year ago
Assignee: nobody → ttromey
(Reporter)

Comment 2

a year ago
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
Status: NEW → ASSIGNED
Summary: inIDOMUtils.getCSSValuesForProperty() is missing keywords for 'clip' → inIDOMUtils.getCSSValuesForProperty() is missing keywords for 'clip-path'
(Assignee)

Comment 3

a year ago
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 hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
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

a year ago
mozreview-review
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.
Attachment #8945579 - Flags: review?(xidorn+moz) → review+
(Assignee)

Comment 7

a year ago
(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

a year ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d3d5c5af4582
fix getCSSValuesForProperty for clip and clip-path; r=xidorn
https://hg.mozilla.org/mozilla-central/rev/d3d5c5af4582
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(Reporter)

Comment 10

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

Sebastian
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.