940229 - Add mochitest to check that shorthand properties can't take "inherit" (or "initial", or "unset") in combination with subproperty values

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P4)

Description

[Daniel Holbert [:dholbert]]

While writing a patch for bug 939905 (the "flex-flow" shorthand), I initially wrote a shorthand parsing function that would accept e.g. "row inherit" [setting one subproperty to the value "row", and the second to "inherit"].

No existing mochitests caught this bug. Right now, the only way to catch this is to explicitly think up values like this and add them to the property_database.js "invalid_values" list for each shorthand property.

This is pretty easy to automate, though. We should have a mochitest that checks a list of of generated values that should all be rejected -- something like the following:
a) "inherit" repeated N times (where N = number of sub-properties)
b) "firstSubpropertyInitialVal inherit inherit ... inherit" (repeated N-1 times)
c) "inherit firstSubpropertyInitialVal"
d) "firstComponentInitialVal inherit"
e) All of the above, but now with s/inherit/initial/
f) All of the above, but now with s/inherit/unset/

Note that (b), (c), or (d) may be trivially invalid, depending on the shorthand syntax, but that's fine, and for most shorthands it'd be testing something useful. The point is, regardless of the shorthand syntax, we know they're invalid, and we should enforce that.

Spec references:
> If a shorthand is specified as one of the CSS-wide keywords [CSS3VAL]
> it sets all of its sub-properties to that keyword. (Note that these
> keywords cannot be combined with other values in a single declaration,
> not even in a shorthand.)
http://www.w3.org/TR/css3-cascade/#shorthand

And CSS-wide is defined as:
> CSS-wide keywords: ‘initial’ and ‘inherit’
[...]
> [CSS3CASCADE] adds an ‘unset’ keyword to this set.
http://www.w3.org/TR/css3-values/#common-keywords

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I actually have a test for this in my patch queue; I don't remember why I never landed it, but I think something was a bit of a pain.

https://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/3b33740a6e70/unapplied.test-extra-inherit-initial

(I think it only tests the beginning and end of values.)

Comment 2

[Daniel Holbert [:dholbert]]

Thanks, dbaron! On my machine, that patch's mochitest *nearly* passes, except for two problematic properties: counter-reset and counter-increment (both of which accept "inherit" at the end of their values -- e.g. this URL puts no parse errors in the browser console:
 data:text/html,<div style="counter-reset: foo 1 inherit">

I'll post dbaron's patch here, followed by my own patch which tweaks it to unprefix "initial", test "unset", and avoid those problematic properties (which I'll file a followup bug about).

[upping priority to P4; I think this is worth resolving, to catch bugs in parser code easier/sooner]

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 1: dbaron's patch from patch queue

Here's dbaron's patch from comment 1, with commit message updated to mention this bug #, "part 1", and r=me

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Thanks, dbaron! On my machine, that patch's mochitest *nearly* passes,
> except for two problematic properties: counter-reset and counter-increment
> (both of which accept "inherit" at the end of their values

I filed bug 940778 for this issue, and John Daggett already has a patch posted! So, it looks like I won't need to disable those properties in this bug's mochitest after all (as I'd initially been planning, per comment 2).

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 2: tweak boilerplate, unprefix "initial", test "unset"

This patch:
 - Adds the standard mochitest-boilerplate bug number references (using this bug).
 - Drops an obsolete bit of boilerplate (/MochiKit/packed.js), which is no longer needed and slows down mochitest load times, per http://nakubu.com/post/28 
 - Unprefixes "initial".
 - Adds tests for "unset".

Comment 6

[Daniel Holbert [:dholbert]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=6a6e2da36ee2

Comment 7

[Daniel Holbert [:dholbert]]

Sorry, ignore that last Try run -- it was missing the patch for bug 940778, so it had counter-increment/counter-reset failures.

Updated try run: https://tbpl.mozilla.org/?tree=Try&rev=52c9dd4a6da0

Comment 8

[Daniel Holbert [:dholbert]]

Here's another try run, now with requestLongerTimeout to hopefully fix the timeouts during this test on B2G emulator in previous run:
 https://tbpl.mozilla.org/?tree=Try&rev=eab2e5fd4b77

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: part 2 v2: tweak boilerplate, unprefix "initial", test "unset", *and* request longer timeout

Looks like that did it.

To recap: the Try run in comment 7 had the previous version of my tweaks here, and (with a bunch of retriggers for extra testing) that Try run revealed that this test was exceeding its allotted time on B2G M-9.

But with the requestLongerTimeout (included in this patch version and in the Try run in comment 8), this is green on B2G M-9 as well. So I think this is all set.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8335865 [details] [diff] [review]
part 2 v2: tweak boilerplate, unprefix "initial", test "unset", *and* request longer timeout

r=dbaron; sorry for the delay getting to this

Comment 11

[Daniel Holbert [:dholbert]]

No worries. Final Try run: https://tbpl.mozilla.org/?tree=Try&rev=b24b80e40a19

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac178db3717
https://hg.mozilla.org/integration/mozilla-inbound/rev/53aedd08699f

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6ac178db3717
https://hg.mozilla.org/mozilla-central/rev/53aedd08699f