Closed
Bug 1212191
Opened 9 years ago
Closed 9 years ago
Test that all shorthands in property_database.js have a nonempty "subproperties" list
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
Right now, "-moz-transform-style" is listed as having
> type: CSS_TYPE_SHORTHAND_AND_LONGHAND,
in property_database.js (which we use as a hack for alias properties for some reason).
But unlike other CSS_TYPE_SHORTHAND_AND_LONGHAND properties (including aliases), it's missing a "subproperties" list. And this hasn't tripped any alarm bells.
We should have a test that verifies that "SHORTHAND"-type properties have a nonempty subproperties list. (Maybe as part of test_property_database.html)
Assignee | ||
Comment 1•9 years ago
|
||
Oh, maybe nevermind... looks like test_property_database.html already has a chunk like this, for TRUE_SHORTHAND, and has a comment saying:
/* optional for CSS_TYPE_SHORTHAND_AND_LONGHAND */
http://mxr.mozilla.org/mozilla-central/source/layout/style/test/test_property_database.html?rev=e801ecdbd5f9&force=1#86
Assignee | ||
Comment 2•9 years ago
|
||
...though we could make it mandatory, if it's useful. (Not sure if it's useful.)
There's a fairly short list of CSS_TYPE_SHORTHAND_AND_LONGHAND properties that fail this hypothetical requirement right now:
-moz-border-end-color
-moz-border-end-style
-moz-border-end-width
-moz-border-start-color
-moz-border-start-style
-moz-border-start-width
-moz-margin-end
-moz-margin-start
-moz-padding-end
-moz-padding-start
(and -moz-transform-style, which I just fixed in a followup on bug 1210905.)
This list looks kinda arbitrary... there's probably no harm in just making these consistent with the rest of our SHORTHAND_AND_LONGHAND properties and enforcing that subproperties must be listed.
Assignee | ||
Comment 3•9 years ago
|
||
FWIW, the "optional" comment dates back to when this test code was first added, here: http://hg.mozilla.org/mozilla-central/rev/f4c7169b23bd#l3.25
...and yet we've been mostly treating it as mandatory, except for the few properties listed in comment 2.
If we're specifying it for most properties in this category, we might as well be consistent and specify it for all of them. And then if it's not useful (I'm not sure it is), we can remove it later across the board.
Updated•9 years ago
|
Attachment #8670628 -
Flags: review?(cam) → review+
Assignee | ||
Comment 4•9 years ago
|
||
Comment 5•9 years ago
|
||
I think most of these have an alias_for -- which is effectively also the "subproperties". Maybe we could have something that automatically creates a subproperties list for properties with "alias_for", by setting it to the alias, unless that alias also has subproperties? (We could just skip the filling in that case, or could copy the subproperties list from the alias.)
Assignee | ||
Comment 6•9 years ago
|
||
I was thinking about doing that too, but separately. For now, since we already populate this 'subproperties' list for every other alias property, we might as well be consistent for these few additional ones as well. So: I'll land this, and then if we want to, we can drop "subproperties" from aliases all at once, separately.
(It might be nice to drop the SHORTHAND_AND_LONGHAND charade for aliases, before [or when] we drop their explicit "subproperties" lists, so that we can remain consistent about always explicitly specifying subproperties for things that we say are shorthands.)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee | ||
Comment 9•9 years ago
|
||
I filed bug 1216351 on dbaron's suggestion in comment 5, BTW.
You need to log in
before you can comment on or make changes to this bug.
Description
•