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

RESOLVED FIXED in mozilla29

Status

()

Core
CSS Parsing and Computation
P4
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.)

Updated

4 years ago
Priority: -- → P5
(Assignee)

Comment 2

4 years ago
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]
Priority: P5 → P4
(Assignee)

Updated

4 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
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
Attachment #8334965 - Flags: review+
(Assignee)

Comment 4

4 years ago
(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).
Depends on: 940778
(Assignee)

Comment 5

4 years ago
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".
Attachment #8335098 - Flags: review?(dbaron)
(Assignee)

Comment 6

4 years ago
Try run: https://tbpl.mozilla.org/?tree=Try&rev=6a6e2da36ee2
(Assignee)

Comment 7

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

Comment 8

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

Comment 9

4 years ago
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.
Attachment #8335098 - Attachment is obsolete: true
Attachment #8335098 - Flags: review?(dbaron)
Attachment #8335865 - Flags: review?(dbaron)
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
Attachment #8335865 - Flags: review?(dbaron) → review+
(Assignee)

Comment 11

4 years ago
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
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/6ac178db3717
https://hg.mozilla.org/mozilla-central/rev/53aedd08699f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 957797
Depends on: 958075
(Assignee)

Updated

4 years ago
Blocks: 958802
(Assignee)

Updated

4 years ago
No longer depends on: 957797
(Assignee)

Updated

2 years ago
Blocks: 1187110
You need to log in before you can comment on or make changes to this bug.