Closed Bug 1208344 Opened 9 years ago Closed 8 years ago

Add support for "-webkit-box-orient", as a writing-mode-dependent alias for "flex-direction" with value-mapping

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox44 --- affected
firefox46 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Whiteboard: [backed out / reimplemented in bug 1262049])

Attachments

(7 files, 4 obsolete files)

3.78 KB, patch
heycam
: review+
Details | Diff | Splinter Review
4.21 KB, patch
heycam
: review+
Details | Diff | Splinter Review
3.98 KB, patch
heycam
: review+
Details | Diff | Splinter Review
3.50 KB, patch
heycam
: review+
Details | Diff | Splinter Review
2.99 KB, patch
heycam
: review+
Details | Diff | Splinter Review
3.52 KB, patch
heycam
: review+
Details | Diff | Splinter Review
19.89 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
As part of (mobile) web compatibility, we're wanting to support "display:-webkit-box" and its associated properties by mapping things into modern flexbox syntax.

For this to work, we need to support several -webkit prefixed CSS property aliases with a custom keyword table, whose values we'll map to their modern equivalents.

(Right now, we do this for whitelisted sites via CSSUnprefixingService.js, which has a list of these properties and their old-to-modern value mappings:
http://mxr.mozilla.org/mozilla-central/source/layout/style/CSSUnprefixingService.js?rev=39c35b0c2e04&mark=56-88#56
But now that we're shifting away from the whitelist strategy, we can & should support these property-aliases natively in our C++ parser code, for more robust/performant parsing behavior.)

Note that this is distinct from bug 837211. There, the properties are pure aliases -- no difference in parsing behavior between the prefixed & standard version. Here, we want custom parsing behavior (probably just mapping old keywords to new enums), while still setting an aliased standard property.
I briefly looked into supporting this via a new flag in nsCSSPropAliasList.h.  For that to work, I think we'd need to adjust nsCSSProps::LookupProperty to recognize these properties and *not* map them to their alias-targets (which is what it normally does).  And we'd need to represent the mapping to the new keyword table somewhere, too. So: this might be a bit messy.

So: perhaps instead of using nsCSSPropAliasList.h, it might be better to do something similar to what we did for logical properties (bug 1117983)? IIUC, logical properties end up effectively being aliases for other properties, with a bit of custom behavior when we're parsing -- and that sounds a lot like what we want here.

(In short: I want to be able to make the CSS parser support the keyword-valued "-webkit-box-align" property as an alias for "align-items", and I'd like to be able to provide a custom keyword-table which we'll use instead of align-items' keyword table when parsing.)

heycam, I'd love your thoughts on this, from your experience implementing the logical properties.
Flags: needinfo?(cam)
Summary: Add support for aliased CSS properties whose values are *also* aliases → Add support for aliased CSS properties whose keyword values are *also* aliases
If we followed the same approach as logical properties, we would get the following behaviour:

We can support both -webkit-box-align and align-items on a single declaration, for them both to be queryable if you inspect the declaration, and the last one would win.  So for example:

  p {
    -webkit-box-align: start;
    align-items: flex-end;
  }

would mean:

* calling getPropertyValue("-webkit-box-align") on the declaration would give you "start"
* getPropertyValue("align-items") would give you "flex-end"
* the computed value stored in nsStylePosition::mAlignItems would be
  NS_STYLE_ALIGN_ITEMS_FLEX_END

Logical properties aren't exposed on nsComputedDOMStyle objects.  Would you want getComputedStyle(...).getPropertyValue("-webkit-box-align") to give you "end"?

Is that what we're looking for?
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #2)
> Logical properties aren't exposed on nsComputedDOMStyle objects.

Is that hard in general, though? (like, is it something we could do if we used a new macro that's that's similar but not quite the same as CSS_PROP_LOGICAL?)  I imagine we'd need custom DoGetXYZ implementations in nsComputedDOMStyle.cpp which e.g. look at mAlignItems and perform the translation back to the old-fashioned keyword space to produce the returned value.

> Would you
> want getComputedStyle(...).getPropertyValue("-webkit-box-align") to give you
> "end"?

In a perfect world, yes. (which I imagine means we might need a different macro, per above)

BUT: if that ends up being difficult, it's likely OK for a first-pass implementation & possibly forever.  There probably aren't too many sites that depend on that working, since we'll only be using this functionality for a few properties, and these properties computed values' aren't particularly interesting for sites to query. (unlike, say, the "width" property)

Anyway: this is sounding like a promising route [though alternate maybe-better strategies are very welcome].
No it won't be hard to expose them on nsComputedStyleObject.  We'll need to use a different, but similar mechanism to logical properties in any case, since they're quite tied to having pairs or (sets of four) physical properties that they map to.  But the general approach would work.  You can see how logical properties are handled in nsCSSDataBlock::MapRuleInfoInto.

Our requirements for the compatibility aliases are a bit different though and so we wouldn't need to support them as two separate properties, since we don't need to wait until after we've resolved direction/writing-mode/text-orientation before knowing which physical property a given logical property correspond to.  So we still could do it with an alias, as long as it's fine for -webkit-box-align and align-items to overwrite each other just like our existing aliases do.  But then we wouldn't be to see both on the declaration.  But we still could expose the compatibility alias on computed style.

So I think both approaches would work here, as long as the overriding behaviour is OK.

The logical-like approach is probably easier to implement.
(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #4)
> So we still could do it
> with an alias, as long as it's fine for -webkit-box-align and align-items to
> overwrite each other just like our existing aliases do.

(That's fine/expected, yeah. Also fine to not see it in the declaration.)

I'm curious how we'd represent the alias... We'd probably need to rework some of our parsing code to let out-of-bounds (alias-range) nsCSSProperty enum-values make it further in the parsing codepath - because right now, if I allow nsCSSProps::LookupProperty to return an alias's own property-ID, I fail some assertions pretty quick (since aliases are above eCSSProperty_COUNT).

I'm going to proceed with the logical-like approach for now, I think. Thanks for the help!
Actually, I've just realized that two of the properties that we need this for (-webkit-box-[align|pack]) *can* actually just be treated as true aliases, once we've broadened our CSS-align support.  The prefixed properties' values are almost the same as the standard properties values, except that we map "start" to "flex-start" and "end" to "flex-end" (as is shown in CSSUnprefixingService.js). BUT, the css-align spec actually adds "start" & "end" values to the standard properties here, with the correct meanings, so we don't actually have to do any keyword mapping once we support those new keywords.

So: -webkit-box-[align|pack] should be representable as true aliases.

So we may only need this aliasing-with-custom-parsing-behavior functionality for one or two properties. The only one I'm aware of right now is "-webkit-box-orient", whose mapping is defined here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/CSSUnprefixingService.js?rev=39c35b0c2e04&mark=70-78#70
Blocks: 1208635
Summary: Add support for aliased CSS properties whose keyword values are *also* aliases → Add support for aliased CSS properties whose keyword values are *also* aliases ("-webkit-box-orient" in particular)
Blocks: 1170789
Attached patch strawman fix v1 (obsolete) — Splinter Review
Here's a strawman patch which basically fixes this, in simple testing (though it doesn't fully pass style system mochitests. Not too concerned about that since it needs cleanup anyway.)

This just adds "-webkit-box-orient" as a fake axis-flavored property, whose numeric enum values happen to map exactly to the corresponding numeric enum values in "flex-direction" (so keyword-conversion automagically works).
In an ideal world, I'd like to support -webkit-box-orient: [horizontal|vertical] as well as -webkit-box-direction: [normal|reverse], and look at the *together* (with knowledge of the writing-mode) to come up with the appropriate 'flex-direction: row|column|row-reverse|colum-reverse' value.

So e.g. if we have "-webkit-box-orient: horizontal; -webkit-box-direction: reverse", then we should produce "flex-direction: row-reverse", if our writing-mode is horizontal.  Or if the writing mode is vertical, we should produce "flex-direction: column-reverse".

(This level of support hasn't been necessary for any of our CSSUnprefixingService-whitelisted sites, though, FWIW. In the CSSUnprefixingService, we just treat -webkit-box-orient as a standalone alias with horizontal-->flex-direction:row & vertical-->flex-direction:column. So, we could probably do that simple mapping to start, as the attached strawman approximately does.)
Blocks: 1213126
I was moving down the "ideal world" path laid out in comment 8, but I actually realized that integrating with -webkit-box-direction is more complex (and I spun off bug 1230708 as a result).

So for the purposes of this bug, I'll go back to just focusing on -webkit-box-orient, and I'll ignore the existence of -webkit-box-direction.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Blocks: 1234941
Patches coming up; sorry for the wait here.

* Parts 1 through 4 are very targeted refactorings in nsCSSDataBlock, with no behavior-change.
* Part 5 will be the actual fix (which plugs nicely into nsCSSDataBlock given the refactorings).
* Part 6 will be a mochitest.
(This one's the main patch, which everything else here supports.)
Attachment #8702728 - Flags: review?(cam)
Sorry, I forgot about this part when writing comment 10. This is one more "supporting" patch, to let the main patch be a little more compact.

The next part (part 7) will be the mochitest.
Attachment #8702730 - Flags: review?(cam)
(Reposting main patch -- part 5 -- with some minor improvements.)
Attachment #8689773 - Attachment is obsolete: true
Attachment #8702728 - Attachment is obsolete: true
Attachment #8702728 - Flags: review?(cam)
Attachment #8702737 - Flags: review?(cam)
NOTES ON GENERAL STRATEGY & MOTIVATION FOR HELPER-PATCHES:
==========================================================
To perform the "-webkit-box-orient" to "flex-direction" value-conversion in nsCSSDataBlock.cpp, we need to hold onto the logical property ID (not the physical property ID) longer than the code does now, so that we can check if we're dealing with -webkit-box-orient and do appropriate conversion on our nsCSSValue before we map it into the target struct.

So, in the interests of keeping the original property ID around longer, we have here:
 * part 1, which:
    - makes EnsurePhysicalProperty *not modify* its passed-in property ID.
    - adjusts callers accordingly to distinguish between the original property & the returned property.
 * part 2, which:
    - simplifies the scoping around calls to EnsurePhysicalProperty by pushing the "is this a logical property" flag-check inside of EnsurePhysicalProperty().
    - adjusts the callers to detect logicalness by checking whether EnsurePhysicalProperty's returned property is different from the passed-in property.

At this point (after part 2), we *could* add special-case code to each MapRuleInfoInto implementation, to do the value-mapping before we call MapSinglePropertyInto. (And I initially did this, in my strawman patch posted earlier.) However, to avoid repeating this code (with subtle differences in each copy), I think it's cleaner to just put the special-case code in the already-shared function, MapSinglePropertyInto.

BUT, for that to work, MapSinglePropertyInto needs to know what the source & destination properties are. So, we next have these patches:
 * part 3, which just renames the existing args in-place, to make it clearer what they currently represent.
 * part 4, which adds a new arg "aSrcProp", and moves the existing arg "aTargetProp" later within the arg-list.

At this point (after part 4), we can (in the main patch, part 5) simply add a special-case to MapSinglePropertyInto(), to perform the conversion. Hooray! So:
 * part 5 does this conversion and adds some other boilerplate.
 * part 6 reorders a new flag that was added in part 5 (originally at the end of a list, for simplicity)
 * part 7 adds a mochitest.
Comment on attachment 8702737 [details] [diff] [review]
part 5, v2: Add (preffed-off) support for "-webkit-box-orient" CSS property, as a writing-mode-dependent alias for "flex-direction"

>+++ b/layout/style/nsCSSProps.h
> #define CSS_PROPERTY_ABSPOS_CB                    (1<<30)
> 
>+// XXXdholbert Move this up to be with other _LOGICAL flags in a later patch.
>+// This property is a logical property which we map to another property/value
>+// using some custom code instead of a mapping-table.
>+#define CSS_PROPERTY_LOGICAL_CUSTOM               (1u << 31)

Note: if you're wondering about the "1u" here (why isn't it just "1" like all the other values?), the reason is: we can't store "1<<31" in an unsigned value, because 1 is signed, and 1<<31 produces a negative number.  So that trigggers a compiler warning (treated as error) if it's stored in an unsigned value.  "1u", on the other hand, is unsigned, and 1u << 31 is just fine being stored in an unsigned value. (their underlying bitfield representation is the same, of course)
[updating bug summary, to reflect what this bug's really about]
Summary: Add support for aliased CSS properties whose keyword values are *also* aliases ("-webkit-box-orient" in particular) → Add support for "-webkit-box-orient", as a writing-mode-dependent alias for "flex-direction" with value-mapping
In general this is looking good.  One comment before proceeding: I'm wondering whether we should handle the property more generically by having say CSS_PROP_LOGICAL_GROUP_SINGLE (instead of CSS_PROP_LOGICAL_GROUP_CUSTOM), adding a table for it in nsCSSProps.cpp that has a single entry in it, and look up the table in EnsurePhysicalProperty.  We could then use the flag CSS_PROPERTY_LOGICAL_CUSTOM to mean just "the value gets additional processing after mapping from the logical to the physical property".  That feels like it would separate the two concerns here a bit better (property mapping and value mapping).  WDYT?
Flags: needinfo?(dholbert)
That sounds reasonable -- I'll give that a shot.

I think that only impacts part 5 (where the logical group & the flag are added), and the other parts here are still needed & are still ready for review.
Flags: needinfo?(dholbert)
Attachment #8702720 - Flags: review?(cam) → review+
Comment on attachment 8702722 [details] [diff] [review]
part 2: Make EnsurePhysicalProperty() (not its callers) check whether property is logical

Review of attachment 8702722 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSDataBlock.cpp
@@ +261,3 @@
>                  // We can't cache anything on the rule tree if we use any data from
>                  // the style context, since data cached in the rule tree could be
>                  // used with a style context with a different value.

I just realized this comment is out of date now that we can cache on the rule tree parameterized by the writing mode...
Attachment #8702722 - Flags: review?(cam) → review+
Attachment #8702723 - Flags: review?(cam) → review+
Attachment #8702724 - Flags: review?(cam) → review+
Here's part 5 again, with comment 23 addressed -- namely:
 - Logical group is now called CSS_PROP_LOGICAL_GROUP_SINGLE
 - There's now a table, gWebkitBoxOrientLogicalGroupTable
 - EnsurePhysicalProperty now uses that table.

As noted in a comment in EnsurePhysicalProperty, there's an implied assumption that CSS_PROPERTY_LOGICAL_CUSTOM implies CSS_PROP_LOGICAL_GROUP_SINGLE. (This is how we know, based on our property-ID, that our table is "single"-flavored.)  That's fine for now, and potentially fine indefinitely.

Maybe it'd be worth renaming the flag from "..._CUSTOM" to something like
 CSS_PROPERTY_LOGICAL_SINGLE_WITH_CUSTOMVALUEMAPPING
to make that "single" assumption clearer; let me know what you think.
Attachment #8702737 - Attachment is obsolete: true
Attachment #8702737 - Flags: review?(cam)
Attachment #8703038 - Flags: review?(cam)
Comment on attachment 8703038 [details] [diff] [review]
part 5, v3: Add (preffed-off) support for "-webkit-box-orient" CSS property, as a writing-mode-dependent alias for "flex-direction"

Review of attachment 8703038 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below addressed.

::: layout/style/nsCSSDataBlock.cpp
@@ +244,5 @@
> +  // Note: we assume here that all CSS_PROPERTY_LOGICAL_CUSTOM properties are
> +  // in a CSS_PROP_LOGICAL_GROUP_SINGLE group. If we add a new logical property
> +  // that violates this assumption, we'll need another flag to disambiguate.
> +  bool isSingleProperty =
> +    nsCSSProps::PropHasFlags(aProperty, CSS_PROPERTY_LOGICAL_CUSTOM);

After renaming CSS_PROPERTY_LOGICAL_CUSTOM to CSS_PROPERTY_LOGICAL_SINGLE_WITH_CUSTOM_VALUE_MAPPING (which I agree with doing in a later comment), this comment might need tweaking.

::: layout/style/nsCSSPropList.h
@@ +1651,5 @@
> +    webkit_box_orient,
> +    WebkitBoxOrient,
> +    CSS_PROPERTY_PARSE_VALUE |
> +      CSS_PROPERTY_LOGICAL_CUSTOM |
> +      CSS_PROPERTY_LOGICAL,

Nit: putting CSS_PROPERTY_LOGICAL before CSS_PROPERTY_LOGICAL_CUSTOM would match other logical property entries in this file.

::: layout/style/nsCSSPropLogicalGroupList.h
@@ +51,5 @@
> +//     "logicalness" is in the value-mapping, not in the property-mapping.  For
> +//     example, the logical property "-webkit-box-orient" is always mapped to
> +//     "flex-direction", but its values ("horizontal", "vertical") map to
> +//     different flex-direction values ("row", "column") depending on the
> +//     writing-mode.

Please mention that a nsCSSProps.cpp table must be defined, like the preceding comments do.

::: layout/style/nsCSSProps.h
@@ +273,5 @@
>  
> +// XXXdholbert Move this up to be with other _LOGICAL flags in a later patch.
> +// This property is a logical property which we map to another property/value
> +// using some custom code instead of a mapping-table.
> +#define CSS_PROPERTY_LOGICAL_CUSTOM               (1u << 31)

I agree that we should rename this; CSS_PROPERTY_LOGICAL_SINGLE_WITH_CUSTOMVALUEMAPPING sounds fine (although "..._WITH_CUSTOM_VALUE_MAPPING" would be better IMO).

Please:
1. mention in the comment that it both (a) means that this is a logical property that always maps to the same physical property, and (b) additionally has some custom processing of its values
2. mention in the comment that it must not be used in conjunction with CSS_PROPERTY_LOGICAL_{AXIS,BLOCK_AXIS,END_EDGE}
3. add to the static_asserts at the bottom of nsCSSProps.cpp to ensure that it is not used in conjunction with those other CSS_PROPERTY_LOGICAL_* flags
4. update the comment above nsCSSProps::LogicalGroup in nsCSSProps.h to mention what happens when one of these properties is passed in
Attachment #8703038 - Flags: review?(cam) → review+
Comment on attachment 8702730 [details] [diff] [review]
part 6: Move new CSS_PROPERTY_LOGICAL_CUSTOM flag up with other LOGICAL flags, and adjust bits accordingly.

Review of attachment 8702730 [details] [diff] [review]:
-----------------------------------------------------------------

r=me although I don't think there's any particular need to move these around (apart from ease of reading).
Attachment #8702730 - Flags: review?(cam) → review+
Comment on attachment 8702732 [details] [diff] [review]
part 7: Add mochitest to test how "-webkit-box-orient" maps to "flex-direction"

Review of attachment 8702732 [details] [diff] [review]:
-----------------------------------------------------------------

If we ever get more CSS_PROPERTY_LOGICAL_SINGLE properties, we might want to do this in a more general manner with a new section in test_logical_properties.html.
Attachment #8702732 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) (away Jan 1-3) from comment #23)
> I'm wondering whether we should handle the property more generically by having
> say CSS_PROP_LOGICAL_GROUP_SINGLE (instead of
> CSS_PROP_LOGICAL_GROUP_CUSTOM),

I meant to say "in addition to" rather than "instead of", but I think handling both with the one flag is fine for now.
> I agree that we should rename this;
> CSS_PROPERTY_LOGICAL_SINGLE_WITH_CUSTOMVALUEMAPPING sounds fine (although
> "..._WITH_CUSTOM_VALUE_MAPPING" would be better IMO).

Sounds good. CSS_PROPERTY_LOGICAL_SINGLE_WITH_CUSTOM_VALUE_MAPPING is pretty long, though -- instead, I'm tentatively using CSS_PROPERTY_LOGICAL_SINGLE_CUSTOM_VALMAPPING. (Trying to find a compromise between readability & not being too long.)

Let me know if you have any objections or suggested alternatives that aren't too long.
No that's fine.
Here's updated part 5, with review comments addressed.

Thanks for the review!
Attachment #8703038 - Attachment is obsolete: true
Attachment #8703063 - Flags: review+
(Taking this off of the dependency list for bug 1170774, since it's already in the dependency tree via bug 1170789.)
No longer blocks: 1170774
Blocks: 1262049
Blocks: 1266248
FWIW: I've backed out this bug in its entirety (to reimplement -webkit-box-orient in a different way) in bug 1262049.

I plan on re-landing this bug's "part 3" and an assert from "part 4" over in bug 1266248, though, since they're still useful regardless of the rest of this bug's changes. See that bug for more details.
Whiteboard: [backed out / reimplemented in bug 1262049]
You need to log in before you can comment on or make changes to this bug.