With bug 649142, we support logical property groups whose physical properties correspond to an existing physical shorthand. For bug 1117983, where we are implementing {,max-,min-}{block,inline}-size, we will need support for defining groups of logical properties whose names correspond to axes rather than sides. For bug 1120283, we we are implementing offset-{block,inline}-{start,end}, we will need support for defining groups of logical properties whose names correspond to four box sides but which also don't corresponding to an existing shorthand. This bug is for adding support for these two kinds of logical properties.

Comment on attachment 8547348 [details] [diff] [review] Part 1: Define logical property groups more explicitly. Forgot to update a comment in here...

Try run for my logical properties branch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9bf2ce4d8c8c

Comment on attachment 8547351 [details] [diff] [review] Part 4: Add functions to convert from logical to physical axes based on writing mode. Review of attachment 8547351 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/WritingModes.h @@ +287,5 @@ > + // NS_STYLE_WRITING_MODE_VERTICAL_RL, and not the other two (real > + // and hypothetical) values. But this is fine; we only need to > + // distinguish between vertical and horizontal in > + // PhysicalAxisForLogicalAxis. > + int wm = (mWritingMode & eBlockFlowMask) >> 1; Why not just use int wm = mWritingMode & eOrientationMask; here? That's exactly the one bit of information you need.

(In reply to Jonathan Kew (:jfkthame) from comment #9) > Why not just use > > int wm = mWritingMode & eOrientationMask; > > here? That's exactly the one bit of information you need. I must have made a mistake there; (mWritingMode & eOrientationMask) is indeed the value that will correspond to NS_STYLE_WRITING_MODE_HORIZONTAL_TB or NS_STYLE_WRITING_MODE_VERTICAL_RL that I mention in the comment.

Comment on attachment 8547364 [details] [diff] [review] Part 1: Define logical property groups more explicitly. Could you use logicalgroup_ instead of group_ as the CSS_PROP_* argument name, for clarity? (I'm waiting to see how you construct kLogicalGroupTable once you have groups that don't correspond to shorthands...) >+ return &kLogicalGroupTable[gLogicalGroupMappingTable[i + 1]][0]; Can you remove the & and the [0] ? r=dbaron with that

Comment on attachment 8547349 [details] [diff] [review] Part 2: Support non-shorthand-related logical box property groups. >+// CSS_PROP_LOGICAL_GROUP_BOX(name_) >+// Defines a logical property group whose corresponding physical >+// properties are a set of four box properties which are not >+// already represented by an existing shorthand property. For >+// example, the logical property group for >+// offset-{block,inline}-{start,end} contains the top, right, >+// bottom and left physical properties, but there is no shorthand >+// that sets those four properties. The name_ argument must be >+// capitalized LikeSo. A table must be defined in nsCSSProps.cpp >+// named g<name_>LogicalGroupTable containing the four physical >+// properties in top/right/bottom/left order. I think you should also specify that name_ must not a be the DOM property name of a CSS property.

Comment on attachment 8547352 [details] [diff] [review] Part 5: Support logical axis properties. Same comment about name_ as patch 2. >+#ifdef DEBUG >+ size_t len = isAxisProperty ? 2 : 4; >+ for (size_t i = 0; i < len; i++) { >+ MOZ_ASSERT(props[i] != eCSSProperty_UNKNOWN, >+ "unexpected logical group length"); >+ } >+ MOZ_ASSERT(props[len] == eCSSProperty_UNKNOWN, >+ "unexpected logical group length"); >+#endif Please brace and indent the whole contents of the #ifdef, like so: #ifdef DEBUG { size_t len ... ... MOZ_ASSERT(...); } #endif

Comment on attachment 8549526 [details] [diff] [review] Part 4: Add functions to convert from logical to physical axes based on writing mode. (v2) Review of attachment 8549526 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/WritingModes.h @@ +270,5 @@ > + static mozilla::Axis PhysicalAxisForLogicalAxis(uint8_t aWritingModeValue, > + LogicalAxis aAxis) > + { > + // This relies on bit 0 of a writing-value mode indicating vertical > + // orientation and bit 0 of a LogicalAxis value indicating the inline axis. // and on the actual values used by the mozilla::Axis enum. @@ +275,5 @@ > + static_assert(NS_STYLE_WRITING_MODE_HORIZONTAL_TB == 0 && > + NS_STYLE_WRITING_MODE_VERTICAL_RL == 1 && > + NS_STYLE_WRITING_MODE_VERTICAL_LR == 3 && > + eLogicalAxisBlock == 0 && > + eLogicalAxisInline == 1, ...so perhaps the static_assert should check eAxisVertical and eAxisHorizontal, too.

Comment on attachment 8547350 [details] [diff] [review] Part 3: Add physical axis constants. roc suggests not putting these under gfx/ since it's unlikely gfx will need to use them.

So I think I'll move this into WritingModes.h. roc also suggested that "x-axis" and "y-axis" were better names than "horizontal axis" and "vertical axis", and I was set to agree with him, but I think now that I like the symmetry with the LogicalAxis names. So I'm going to stick with that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c04677df85e https://hg.mozilla.org/integration/mozilla-inbound/rev/853c7f0ffd62 https://hg.mozilla.org/integration/mozilla-inbound/rev/002730607596 https://hg.mozilla.org/integration/mozilla-inbound/rev/810f46a5cd16 https://hg.mozilla.org/integration/mozilla-inbound/rev/5e1ea6b6bed8

https://hg.mozilla.org/mozilla-central/rev/2c04677df85e https://hg.mozilla.org/mozilla-central/rev/853c7f0ffd62 https://hg.mozilla.org/mozilla-central/rev/002730607596 https://hg.mozilla.org/mozilla-central/rev/810f46a5cd16 https://hg.mozilla.org/mozilla-central/rev/5e1ea6b6bed8

This is pretty much done, thanks to Rachel's great work: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Logical_Properties