1120284 - add support for axis-relative and non-shorthand box-edge logical properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

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 1

[Cameron McCormack (:heycam)]

Attachment: Part 1: Define logical property groups more explicitly.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Part 2: Support non-shorthand-related logical box property groups.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Part 3: Add physical axis constants.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Part 4: Add functions to convert from logical to physical axes based on writing mode.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Part 5: Support logical axis properties.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8547348 [details] [diff] [review]
Part 1: Define logical property groups more explicitly.

Forgot to update a comment in here...

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Part 1: Define logical property groups more explicitly.

Comment 8

[Cameron McCormack (:heycam)]

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

Comment 9

[Jonathan Kew [:jfkthame]]

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.

Comment 10

[Cameron McCormack (:heycam)]

(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 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 14

[Cameron McCormack (:heycam)]

Attachment: Part 4: Add functions to convert from logical to physical axes based on writing mode. (v2)

Comment 15

[Jonathan Kew [:jfkthame]]

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 16

[Cameron McCormack (:heycam)]

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.

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Part 3: Add physical axis constants. (v2)

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.

Comment 18

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb39d2f17fc2

Comment 19

[Cameron McCormack (:heycam)]

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

Comment 20

[Phil Ringnalda (:philor)]

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

Comment 21

[Chris Mills [:cmills]]

This is pretty much done, thanks to Rachel's great work: 

https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Logical_Properties