Closed Bug 1255378 Opened 8 years ago Closed 6 years ago

inIDOMUtils.getCSSValuesForProperty() is missing keywords for 'box-shadow'

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla60
Tracking Status
firefox60 --- verified

People

(Reporter: sebo, Assigned: tromey)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

This property is missing the 'inset' keyword as well as all <color> keywords.

Test case (to execute in Scratchpad):

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

DOMUtils.getCSSValuesForProperty("box-shadow");

Sebastian
I looked into this a bit.

As with some other cases (e.g., bug 1255401), the immediate issue is that box-shadow has
a custom parser and does not set the variant flags here:

https://dxr.mozilla.org/mozilla-central/rev/21ddfb9e6cc008e47da89db50e22697dc7b38635/layout/style/nsCSSPropList.h#1377

So it would be possible to fix this by tweaking this line.

However, this same problem affects text-shadow (which is part of bug 1255391).  And I found this
in InspectorUtils:

https://dxr.mozilla.org/mozilla-central/rev/21ddfb9e6cc008e47da89db50e22697dc7b38635/layout/inspector/InspectorUtils.cpp#634-637

... that is, code that already tries to compute the proper variants for a property that has a custom parser.

So my question is whether it's better to continue along the lines of bug 1255401, updating nsCSSPropList
(with an aim of removing PropertySupportsVariant); or whether it's better to fix up PropertySupportsVariant
(note that it doesn't mention VARIANT_NONE or VARIANT_INHERIT in those lines -- because
InspectorUtils::CssPropertySupportsType didn't need them) and then reuse this same code in
InspectorUtils::GetCSSValuesForProperty.

I think my inclination is to fix nsCSSPropList, however I don't know if that has any hidden gotchas.
Flags: needinfo?(cam)
As far as I know there's no problem with mentioning the VARIANT_* values in nsCSSPropList for properties with custom parsing.  If you'd like to uniformly handle them there instead of in InspectorUtils that's fine.  At some point soon those VARIANT_* values will no longer be used for parsing at all since we'll only have Servo's CSS parser, so in the longer term it probably makes sense to try to have these supported property value types be derived from the Rust property definitions somehow.
Flags: needinfo?(cam)
I may never remember to rebuild the database.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Comment on attachment 8943363 [details]
Bug 1255378 - factor color lists out of expected values in test_bug877690.html;

https://reviewboard.mozilla.org/r/213692/#review220120
Attachment #8943363 - Flags: review?(cam) → review+
Comment on attachment 8943364 [details]
Bug 1255378 - fix getCSSValuesForProperty for box-shadow and text-shadow;

https://reviewboard.mozilla.org/r/213694/#review220122
Attachment #8943364 - Flags: review?(cam) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e8172456fce57102953c3a423a5507c85330c97e -d de52bf2201a6: rebasing 443494:e8172456fce5 "Bug 1255378 - factor color lists out of expected values in test_bug877690.html; r=heycam"
rebasing 443495:1c1aa75bc51b "Bug 1255378 - fix getCSSValuesForProperty for box-shadow and text-shadow; r=heycam" (tip)
merging layout/inspector/InspectorUtils.cpp
merging layout/style/nsCSSPropList.h
warning: conflicts while merging layout/inspector/InspectorUtils.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Rebased, and nothing significant changed, but at least building it on try to be sure.
Comment on attachment 8943363 [details]
Bug 1255378 - factor color lists out of expected values in test_bug877690.html;

https://reviewboard.mozilla.org/r/213692/#review220354


Static analysis found 3 defects in this patch.
 - 3 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:66
(Diff revision 2)
>    ok(testValues(values, expected), "property color's values.");
>  
>    // test a shorthand property
>    var prop = "background";
>    var values = InspectorUtils.getCSSValuesForProperty(prop);
> -  var expected = [ "initial", "inherit", "unset", "aliceblue", "antiquewhite", "aqua", "aquamarine", "azure",
> +  var expected = [ "initial", "inherit", "unset", ...allColors, "no-repeat", "repeat",

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

::: layout/inspector/tests/test_bug877690.html:76
(Diff revision 2)
>        "-moz-radial-gradient", "-moz-repeating-linear-gradient", "-moz-repeating-radial-gradient" ];
>    ok(testValues(values, expected), "Shorthand property values.");
>  
>    var prop = "border";
>    var values = InspectorUtils.getCSSValuesForProperty(prop);
> -  var expected = [ "initial", "unset", "aliceblue",
> +  var expected = [ "initial", "unset", "calc", "dashed", "dotted", "double", "fill",

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

::: layout/inspector/tests/test_bug877690.html:81
(Diff revision 2)
> -      "salmon", "sandybrown", "seagreen", "seashell", "sienna", "silver", "skyblue", "slateblue", "slategray", "slategrey",
> -      "snow", "solid", "space", "springgreen", "steelblue", "stretch", "tan", "teal", "thick", "thin", "thistle", "tomato",
> -      "transparent", "turquoise", "-moz-element", "-moz-image-rect", "url", "violet", "wheat", "white", "whitesmoke",
> -      "yellow", "yellowgreen", "linear-gradient", "radial-gradient", "repeating-linear-gradient",
>        "repeating-radial-gradient", "-moz-linear-gradient", "-moz-radial-gradient", "-moz-repeating-linear-gradient",
> -      "-moz-repeating-radial-gradient" ]
> +      "-moz-repeating-radial-gradient", ...allColors ]

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8943364 [details]
Bug 1255378 - fix getCSSValuesForProperty for box-shadow and text-shadow;

https://reviewboard.mozilla.org/r/213694/#review220358


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:191
(Diff revision 3)
>      var values = InspectorUtils.getCSSValuesForProperty(prop);
>      ok(testValues(values, expected), "property " + prop + "'s values");
>    }
>  
> +  // Regression test for bug 1255378.
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];

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

::: layout/inspector/tests/test_bug877690.html:192
(Diff revision 3)
>      ok(testValues(values, expected), "property " + prop + "'s values");
>    }
>  
> +  // Regression test for bug 1255378.
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");

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

::: layout/inspector/tests/test_bug877690.html:193
(Diff revision 3)
>    }
>  
> +  // Regression test for bug 1255378.
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");
> +  ok(testValues(values, expected), "property text-shadow's values");

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

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

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

::: layout/inspector/tests/test_bug877690.html:196
(Diff revision 3)
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");
> +  ok(testValues(values, expected), "property text-shadow's values");
> +
> +  var expected = [ "inherit", "initial", "unset", "inset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("box-shadow");

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

::: layout/inspector/tests/test_bug877690.html:197
(Diff revision 3)
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");
> +  ok(testValues(values, expected), "property text-shadow's values");
> +
> +  var expected = [ "inherit", "initial", "unset", "inset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("box-shadow");
> +  ok(testValues(values, expected), "property box-shadow's values");

Error: 'ok' is not defined. [eslint: no-undef]
Attachment #8943363 - Attachment is obsolete: true
I ran the wrong git mozreview command and messed this up; will fix momentarily.
Well, I thought I could fix it, but :heycam isn't accepting requests and now
mozreview won't accept the patches again.
Comment on attachment 8943364 [details]
Bug 1255378 - fix getCSSValuesForProperty for box-shadow and text-shadow;

https://reviewboard.mozilla.org/r/213694/#review220364


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:191
(Diff revision 3)
>      var values = InspectorUtils.getCSSValuesForProperty(prop);
>      ok(testValues(values, expected), "property " + prop + "'s values");
>    }
>  
> +  // Regression test for bug 1255378.
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];

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

::: layout/inspector/tests/test_bug877690.html:192
(Diff revision 3)
>      ok(testValues(values, expected), "property " + prop + "'s values");
>    }
>  
> +  // Regression test for bug 1255378.
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");

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

::: layout/inspector/tests/test_bug877690.html:193
(Diff revision 3)
>    }
>  
> +  // Regression test for bug 1255378.
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");
> +  ok(testValues(values, expected), "property text-shadow's values");

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

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

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

::: layout/inspector/tests/test_bug877690.html:196
(Diff revision 3)
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");
> +  ok(testValues(values, expected), "property text-shadow's values");
> +
> +  var expected = [ "inherit", "initial", "unset", "inset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("box-shadow");

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

::: layout/inspector/tests/test_bug877690.html:197
(Diff revision 3)
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");
> +  ok(testValues(values, expected), "property text-shadow's values");
> +
> +  var expected = [ "inherit", "initial", "unset", "inset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("box-shadow");
> +  ok(testValues(values, expected), "property box-shadow's values");

Error: 'ok' is not defined. [eslint: no-undef]
Comment on attachment 8943364 [details]
Bug 1255378 - fix getCSSValuesForProperty for box-shadow and text-shadow;

https://reviewboard.mozilla.org/r/213694/#review220372


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:191
(Diff revision 3)
>      var values = InspectorUtils.getCSSValuesForProperty(prop);
>      ok(testValues(values, expected), "property " + prop + "'s values");
>    }
>  
> +  // Regression test for bug 1255378.
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];

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

::: layout/inspector/tests/test_bug877690.html:192
(Diff revision 3)
>      ok(testValues(values, expected), "property " + prop + "'s values");
>    }
>  
> +  // Regression test for bug 1255378.
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");

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

::: layout/inspector/tests/test_bug877690.html:193
(Diff revision 3)
>    }
>  
> +  // Regression test for bug 1255378.
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");
> +  ok(testValues(values, expected), "property text-shadow's values");

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

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

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

::: layout/inspector/tests/test_bug877690.html:196
(Diff revision 3)
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");
> +  ok(testValues(values, expected), "property text-shadow's values");
> +
> +  var expected = [ "inherit", "initial", "unset", "inset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("box-shadow");

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

::: layout/inspector/tests/test_bug877690.html:197
(Diff revision 3)
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");
> +  ok(testValues(values, expected), "property text-shadow's values");
> +
> +  var expected = [ "inherit", "initial", "unset", "inset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("box-shadow");
> +  ok(testValues(values, expected), "property box-shadow's values");

Error: 'ok' is not defined. [eslint: no-undef]
I discussed this on irc and I'm going to switch the reviews to :jryans so I can upload the patches again.
Comment on attachment 8945508 [details]
Bug 1255378 - factor color lists out of expected values in test_bug877690.html;

https://reviewboard.mozilla.org/r/214706/#review221300

Thanks, looks good! :)
Attachment #8945508 - Flags: review?(jryans) → review+
Comment on attachment 8943364 [details]
Bug 1255378 - fix getCSSValuesForProperty for box-shadow and text-shadow;

https://reviewboard.mozilla.org/r/213694/#review221302

Thanks for working on this, looks reasonable to me! :)
Attachment #8943364 - Flags: review?(jryans) → review+
Comment on attachment 8945508 [details]
Bug 1255378 - factor color lists out of expected values in test_bug877690.html;

https://reviewboard.mozilla.org/r/214706/#review221304


Static analysis found 3 defects in this patch.
 - 3 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:66
(Diff revision 1)
>    ok(testValues(values, expected), "property color's values.");
>  
>    // test a shorthand property
>    var prop = "background";
>    var values = InspectorUtils.getCSSValuesForProperty(prop);
> -  var expected = [ "initial", "inherit", "unset", "aliceblue", "antiquewhite", "aqua", "aquamarine", "azure",
> +  var expected = [ "initial", "inherit", "unset", ...allColors, "no-repeat", "repeat",

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

::: layout/inspector/tests/test_bug877690.html:76
(Diff revision 1)
>        "-moz-radial-gradient", "-moz-repeating-linear-gradient", "-moz-repeating-radial-gradient" ];
>    ok(testValues(values, expected), "Shorthand property values.");
>  
>    var prop = "border";
>    var values = InspectorUtils.getCSSValuesForProperty(prop);
> -  var expected = [ "initial", "unset", "aliceblue",
> +  var expected = [ "initial", "unset", "calc", "dashed", "dotted", "double", "fill",

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

::: layout/inspector/tests/test_bug877690.html:81
(Diff revision 1)
> -      "salmon", "sandybrown", "seagreen", "seashell", "sienna", "silver", "skyblue", "slateblue", "slategray", "slategrey",
> -      "snow", "solid", "space", "springgreen", "steelblue", "stretch", "tan", "teal", "thick", "thin", "thistle", "tomato",
> -      "transparent", "turquoise", "-moz-element", "-moz-image-rect", "url", "violet", "wheat", "white", "whitesmoke",
> -      "yellow", "yellowgreen", "linear-gradient", "radial-gradient", "repeating-linear-gradient",
>        "repeating-radial-gradient", "-moz-linear-gradient", "-moz-radial-gradient", "-moz-repeating-linear-gradient",
> -      "-moz-repeating-radial-gradient" ]
> +      "-moz-repeating-radial-gradient", ...allColors ]

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8943364 [details]
Bug 1255378 - fix getCSSValuesForProperty for box-shadow and text-shadow;

https://reviewboard.mozilla.org/r/213694/#review221306


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:191
(Diff revision 4)
>      var values = InspectorUtils.getCSSValuesForProperty(prop);
>      ok(testValues(values, expected), "property " + prop + "'s values");
>    }
>  
> +  // Regression test for bug 1255378.
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];

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

::: layout/inspector/tests/test_bug877690.html:192
(Diff revision 4)
>      ok(testValues(values, expected), "property " + prop + "'s values");
>    }
>  
> +  // Regression test for bug 1255378.
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");

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

::: layout/inspector/tests/test_bug877690.html:193
(Diff revision 4)
>    }
>  
> +  // Regression test for bug 1255378.
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");
> +  ok(testValues(values, expected), "property text-shadow's values");

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

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

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

::: layout/inspector/tests/test_bug877690.html:196
(Diff revision 4)
> +  var expected = [ "inherit", "initial", "unset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");
> +  ok(testValues(values, expected), "property text-shadow's values");
> +
> +  var expected = [ "inherit", "initial", "unset", "inset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("box-shadow");

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

::: layout/inspector/tests/test_bug877690.html:197
(Diff revision 4)
> +  var values = InspectorUtils.getCSSValuesForProperty("text-shadow");
> +  ok(testValues(values, expected), "property text-shadow's values");
> +
> +  var expected = [ "inherit", "initial", "unset", "inset", "none", "calc", ...allColors ];
> +  var values = InspectorUtils.getCSSValuesForProperty("box-shadow");
> +  ok(testValues(values, expected), "property box-shadow's values");

Error: 'ok' is not defined. [eslint: no-undef]
Retesting out of an abundance of caution.
Also I think I am allowed to ignore those bot-generated eslint complaints,
because IIUC the bot is running eslint on files that are marked as ignored.
(In reply to Tom Tromey :tromey from comment #26)
> Retesting out of an abundance of caution.
> Also I think I am allowed to ignore those bot-generated eslint complaints,
> because IIUC the bot is running eslint on files that are marked as ignored.

Yes, feel free to drop these issues, there is a bug filed about it:

https://github.com/mozilla-releng/services/issues/788
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e0c7edf0677d
factor color lists out of expected values in test_bug877690.html; r=jryans
https://hg.mozilla.org/integration/autoland/rev/6315cd6fe542
fix getCSSValuesForProperty for box-shadow and text-shadow; r=heycam,jryans
https://hg.mozilla.org/mozilla-central/rev/e0c7edf0677d
https://hg.mozilla.org/mozilla-central/rev/6315cd6fe542
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Works for me on Nightly 60.0a1 (2018-01-27).

Thanks for fixing!

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

Attachment

General

Created:
Updated:
Size: