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
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.)
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]
Created attachment 8334965 [details] [diff] [review] 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
(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).
Created attachment 8335098 [details] [diff] [review] 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".
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
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
Created attachment 8335865 [details] [diff] [review] 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 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
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