Closed Bug 1212191 Opened 4 years ago Closed 4 years ago

Test that all shorthands in property_database.js have a nonempty "subproperties" list

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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)
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
...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.
Attached patch fix v1Splinter Review
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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8670628 - Flags: review?(cam)
Attachment #8670628 - Flags: review?(cam) → review+
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.)
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.)
https://hg.mozilla.org/mozilla-central/rev/268cd349c514
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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.