1083134 - margin/border/padding specified with logical *-start or *-end property does not map to the correct side of the element (top or bottom) in vertical writing mode

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Testcase showing failure to handle logically-specified borders

In the attached example, the <i> element has borders specified with -moz-border-start and -end (5px blue and 10px red respectively). It then reports the computed values for the border-{left,right,top,bottom}-width property.

Toggling LTR/RTL directionality in horizontal writing mode shows the computed values of border-left and -right being exchanged, as expected.

However, when setting vertical-lr (or -rl) writing mode, the borders remain resolutely stuck to the left and right sides of the element, instead of the top and bottom as would be expected.

Comment 2

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

We need to do bug 649142 (and its dependency) before fixing this.

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #2)
> We need to do bug 649142 (and its dependency) before fixing this.

Is there a prospect of this happening in the near future? (Offhand, it sounds like a pretty substantial undertaking.)

I've experimented with a hack that simply remaps box sides within nsRuleNode::AdjustLogicalBoxProp in order to apply the *-start and *-end properties appropriately in vertical mode. Might you consider something like that as an interim fix, if nobody is working on bug 649142 for now?

Comment 4

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

Nobody is working on it, but I would not consider the interim fix; it requires far too many hidden properties.

Comment 5

[Gérard Talbot]

-moz-padding-start should reference (or map to) padding-top of list in vertical writing-mode

This is what Chrome 37.0.2171.71 does: it maps -webkit-padding-start of list in vertical writing-mode to padding-top 

Tests
-----

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/wm-block-flow-direction-015.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/wm-block-flow-direction-016.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/wm-line-box-direction-015.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/wm-line-box-direction-016.xht


If you prefer, I can create a separate new bug report specific and dedicated to -moz-padding-start and then make it depend on this bug 1083134.


Gérard

Comment 6

[Jonathan Kew [:jfkthame]]

I don't think a separate bug is needed at this point; all the padding-, margin-, and border- properties need similar handling.

Comment 7

[Cameron McCormack (:heycam)]

I can take this once bug 649142 has laid the foundation.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: WIP v0.1

Jonathan: in my WIP patch, I'm adding logic to convert between logical sides to physical sides based on {writing-mode,text-orientation,direction} values in nsCSSDataBlock.cpp (which is where the physical and logical properties are cascaded together).  I feel like I'm maybe duplicating some part of WritingModes.h here, but I'm a bit confused about the bits stored in a WritingMode value -- specifically I can't tell whether it stores the values of these three properties in a form I can use to index into my side mapping arrays.

Another issue is that some of the WritingMode stuff is #ifdef WRITING_MODE_VERTICAL_ENABLED and that's not going to interact well with making these new block-axis properties preffable.

Do you think I should move my array-based lookups into WritingMode?  If you're OK with the (possible) duplication (for now, at least), then it's fine by me to leave it where it is.

Comment 10

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add CSS_PROPERTY_LOGICAL_{BLOCK_AXIS,END_SIDE} flags to record which side a logical property is for.

Comment 11

[Cameron McCormack (:heycam)]

Attachment: Part 2: Add function that can return which physical box property shorthand a given logical longhand is related to.

Comment 12

[Cameron McCormack (:heycam)]

Attachment: Part 3: Cascade block-axis logical properties with their physical equivalents.

Comment 13

[Cameron McCormack (:heycam)]

Attachment: Part 4: Add padding-block-{start,end} logical properties.

Comment 14

[Cameron McCormack (:heycam)]

Attachment: Part 5: Add margin-block-{start,end} logical properties.

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Part 6: Add border-block-{start,end}-{color,style,width} logical properties.

Comment 16

[Cameron McCormack (:heycam)]

Attachment: Part 7: Tests.

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add CSS_PROPERTY_LOGICAL_{BLOCK_AXIS,END_SIDE} flags to record which side a logical property is for.

Add some more context in the diff.

Comment 18

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5bf8030846fd

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #9)
> Created attachment 8543697 [details] [diff] [review]
> WIP v0.1
> 
> Jonathan: in my WIP patch, I'm adding logic to convert between logical sides
> to physical sides based on {writing-mode,text-orientation,direction} values
> in nsCSSDataBlock.cpp (which is where the physical and logical properties
> are cascaded together).  I feel like I'm maybe duplicating some part of
> WritingModes.h here, but I'm a bit confused about the bits stored in a
> WritingMode value -- specifically I can't tell whether it stores the values
> of these three properties in a form I can use to index into my side mapping
> arrays.

AFAICS, it would be possible to use the low 4 bits of the WritingMode value:

    eOrientationMask = 0x01, // true means vertical text
    eInlineFlowMask  = 0x02, // true means absolute RTL/BTT (against physical coords)
    eBlockFlowMask   = 0x04, // true means vertical-LR (or horizontal-BT if added)
    eLineOrientMask  = 0x08, // true means over != block-start

This provides all the info you need, but you'd need a 16-element array, rather than the 8 elements you have in your current kLogicalInlineSides. Which makes sense: it doesn't fit into 3 bits here, because in the overallWritingMode that you're using with kLogicalInlineSides, you've conflated the block-dir element of vertical-* with the text-orientation setting into a single bit, which is OK for that purpose but would not be usable for WritingMode in general.

So yes, your OverallWritingMode() helper largely duplicates what the WritingMode constructor is doing, in that it gets the three relevant CSS properties and combines them into a single value; the difference is that WritingMode doesn't fit the precise subset of values you care about into 3 bits, so it can't be used with quite such a compact table. Still, perhaps it's worth accepting a bit of redundancy there, so as to avoid the need for the custom (but near-duplicate) logic in OverallWritingMode().

> Another issue is that some of the WritingMode stuff is #ifdef
> WRITING_MODE_VERTICAL_ENABLED and that's not going to interact well with
> making these new block-axis properties preffable.

IMO, we're at the point where we could safely remove that #ifdef if it makes things cleaner/easier.

> Do you think I should move my array-based lookups into WritingMode?  If
> you're OK with the (possible) duplication (for now, at least), then it's
> fine by me to leave it where it is.

I think I'd lean towards using the WritingMode constructor instead of a custom OverallWritingMode() unless you're able to get a significant perf win by having a simplified version here. We could have a method on WritingMode that provides the mapping you need, so that in EnsurePhysicalProperty() you'd just do something like

  side = WritingMode(aRuleData->styleContext).GetPhysicalInlineSide(isEnd);

to get the physical side corresponding to the logical start/end. That method would encapsulate your table (expanded to 16 entries, as noted above), and index it on (mWritingMode & 0x0f).

I'm not strongly attached to this approach, though; if there appear to be real efficiency benefits to doing it with custom code in nsCSSDataBlock, I can live with that.

(For kLogicalBlockSides, just continuing to use the styleVisibility->mWritingMode value directly seems fine; there's no point in going through the extra work of constructing a WritingMode in this case. Though maybe it'd be nice to move this to WritingModes as well, if we move the inline mapping, so that they're kept together in the code.)

Comment 20

[Cameron McCormack (:heycam)]

(In reply to Jonathan Kew (:jfkthame) from comment #19)
> AFAICS, it would be possible to use the low 4 bits of the WritingMode value:
> 
>     eOrientationMask = 0x01, // true means vertical text
>     eInlineFlowMask  = 0x02, // true means absolute RTL/BTT (against physical coords)
>     eBlockFlowMask   = 0x04, // true means vertical-LR (or horizontal-BT if added)
>     eLineOrientMask  = 0x08, // true means over != block-start

OK, I don't mind to make it a 16-element array.  So from what I understand:

* eOrientationMask and eBlockFlowMask determine the value of writing-mode (including
  a hypothetical horizontal-bt value) in a straightforward manner

* eLineOrientMask is the value of text-orientation relative to whether the
  block start edge is left or right; if we're vertical-rl, eLineOrientMask
  being set means text-orientation was sideways-left; if we're vertical-lr,
  eLineOrientMask being set means text-orientation we had a value other
  than sideways-left

Now what I can't tell is how eInlineFlowMask corresponds to the value of direction.  It seems that eBidiMask is exactly what I want, but if I can use eInlineFlowMask instead that would save some bit manipulation.  Currently eInlineFlowMask and eBidiMask are always set together, but from the comment I guess eInlineFlowMask won't be set in some direction:rtl cases.  Can you help me understand how the other bits (is it eBlockFlowMask and eLineOrientMask?) combine with eInlineFlowMask to get me the same value that would be stored in eBidiMask?

> > Another issue is that some of the WritingMode stuff is #ifdef
> > WRITING_MODE_VERTICAL_ENABLED and that's not going to interact well with
> > making these new block-axis properties preffable.
> 
> IMO, we're at the point where we could safely remove that #ifdef if it makes
> things cleaner/easier.

Great.

By the way, we're missing code in here to map text-orientation:sideways to sideways-{left,right} depending on whether we're vertical-rl or vertical-lr.

> (For kLogicalBlockSides, just continuing to use the
> styleVisibility->mWritingMode value directly seems fine; there's no point in
> going through the extra work of constructing a WritingMode in this case.
> Though maybe it'd be nice to move this to WritingModes as well, if we move
> the inline mapping, so that they're kept together in the code.)

Yeah.  I'll see if I can keep all the conversion logic in WritingModes.h but avoid most of the work for the block cases when we're calling it from nsCSSDataBlock.cpp.

Comment 21

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

Re comment 19 and comment 20:  aren't the first 3 bits sufficient for logical properties?  They seem like they shouldn't depend on eLineOrientMask, no?  (If not, then I'm confused by the comments for those bits!)

Comment 22

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

Comment on attachment 8543846 [details] [diff] [review]
Part 2: Add function that can return which physical box property shorthand a given logical longhand is related to.

>+// Mapping of logical longhand properties to the shorthand that sets the four
>+// corresponding physical properties.

Maybe expand this comment to comment about the two-entries-per-item format and mention (maybe in the header, or maybe here) the lookup time being proportional to the number of logical proerties?

r=dbaron

Comment 23

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

Comment on attachment 8543848 [details] [diff] [review]
Part 3: Cascade block-axis logical properties with their physical equivalents.

Sounds like you're looking to revise this one, so I'll wait.  re-request if you want me to review it anyway.

Comment 24

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

Comment on attachment 8543851 [details] [diff] [review]
Part 6: Add border-block-{start,end}-{color,style,width} logical properties.

The color ones should all also have 
CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED.

The width ones should have
CSS_PROPERTY_GETCS_NEEDS_LAYOUT_FLUSH.

r=dbaron with that (though perhaps it's worth updating tests so they'd catch that -- though I'd think the second would already how up as assertion counts, though maybe not)

Comment 25

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #21)
> Re comment 19 and comment 20:  aren't the first 3 bits sufficient for
> logical properties?  They seem like they shouldn't depend on
> eLineOrientMask, no?  (If not, then I'm confused by the comments for those
> bits!)

The value of text-orientation influences the logical -> physical mapping, per:

  http://dev.w3.org/csswg/css-writing-modes/#logical-to-physical

Comment 26

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

Comment on attachment 8543853 [details] [diff] [review]
Part 7: Tests.

I didn't look too closely, but the stuff you're testing looks good.

Comment 27

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

(In reply to Cameron McCormack (:heycam) from comment #25)
> The value of text-orientation influences the logical -> physical mapping,
> per:
> 
>   http://dev.w3.org/csswg/css-writing-modes/#logical-to-physical

Ah, ok.

Comment 28

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #24)
> Comment on attachment 8543851 [details] [diff] [review]
> Part 6: Add border-block-{start,end}-{color,style,width} logical properties.
> 
> The color ones should all also have 
> CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED.
> 
> The width ones should have
> CSS_PROPERTY_GETCS_NEEDS_LAYOUT_FLUSH.
> 
> r=dbaron with that (though perhaps it's worth updating tests so they'd catch
> that -- though I'd think the second would already how up as assertion
> counts, though maybe not)

Can you point me to the tests where the presence of these on the existing border properties would be tested?

Comment 29

[Cameron McCormack (:heycam)]

(I left those flags off -moz-border-{start,end}-{color,width} in bug 649142, which I coped to make border-block-{start,end}-{color,width}.)

Comment 30

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

(In reply to Cameron McCormack (:heycam) from comment #28)
> (In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from
> comment #24)
> > Comment on attachment 8543851 [details] [diff] [review]
> > Part 6: Add border-block-{start,end}-{color,style,width} logical properties.
> > 
> > The color ones should all also have 
> > CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED.
> > 
> > The width ones should have
> > CSS_PROPERTY_GETCS_NEEDS_LAYOUT_FLUSH.
> > 
> > r=dbaron with that (though perhaps it's worth updating tests so they'd catch
> > that -- though I'd think the second would already how up as assertion
> > counts, though maybe not)
> 
> Can you point me to the tests where the presence of these on the existing
> border properties would be tested?

The first flag is tested in layout/style/test/test_dont_use_document_colors.html .

I would think the lack of the second should trigger assertions in test_value_computation.html, assuming that it tests computed values of the logical properties.

(In reply to Cameron McCormack (:heycam) from comment #29)
> (I left those flags off -moz-border-{start,end}-{color,width} in bug 649142,
> which I coped to make border-block-{start,end}-{color,width}.)

Yeah, they should be there too.

Comment 31

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #30)
> The first flag is tested in
> layout/style/test/test_dont_use_document_colors.html .

Thanks.  Thinking about this more: the flags are unnecessary on the logical properties, since we clear out these values by looking in the nsRuleData, at which point we only have physical properties.

I think I'll static_assert that we don't use this flag on logical properties.

> I would think the lack of the second should trigger assertions in
> test_value_computation.html, assuming that it tests computed values of the
> logical properties.

Ah, so that assumption is false, since we're going through property_database.js' logical_box_prop_get_computed, which looks up the right physical property.  And that's because logical properties are not exposed on computed style objects.  I'll make a note in bug 1116638 to get rid of logical_box_prop_get_computed, which should end up triggering the assertions if we didn't have the flags.

Comment 32

[Cameron McCormack (:heycam)]

Attachment: Part 3: Add functions to WritingModes to convert between logical and physical sides. (v2)

Here's a patch that uses eInlineFlowMask and works currently.  Please let me know if I should be testing some other bits to get a future-proof value equivalent to eBidiMask.

Comment 33

[Cameron McCormack (:heycam)]

Attachment: Part 3.1: Cascade block-axis logical properties with their physical equivalents.

Comment 34

[Cameron McCormack (:heycam)]

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

Comment 35

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=93f1e6baa844 with the vertical text pref forced on

Comment 36

[Cameron McCormack (:heycam)]

And again without the warnings-as-errors induced failures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=252b9993e3ea
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b28b2183994b (with pref set)

Comment 37

[Cameron McCormack (:heycam)]

Attachment: Part 6.1: Change browser_ruleview_completion-existing-property_01.js from using border-* to font-*.

Patrick: I'm changing this test from using "border-" to "font-" to avoid having to conditionally change whether "border-bottom" or "border-block-end" is the auto-completed property name based on whether the vertical text pref is set.

Comment 38

[Patrick Brosset <:pbro>]

Comment on attachment 8544499 [details] [diff] [review]
Part 6.1: Change browser_ruleview_completion-existing-property_01.js from using border-* to font-*.

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

This looks fine. The test is still valid like this.

Comment 39

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #36)
> And again without the warnings-as-errors induced failures:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=252b9993e3ea
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b28b2183994b (with
> pref set)

Note that there are currently a few known test failures when the vertical-text pref is set, so orange M5 (from font sizes/line heights in test_inherit_computation.html) and R (a couple of the tests in layout/reftests/writing-mode) will be expected here.

Comment 40

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #20)

> Now what I can't tell is how eInlineFlowMask corresponds to the value of
> direction.  It seems that eBidiMask is exactly what I want, but if I can use
> eInlineFlowMask instead that would save some bit manipulation.  Currently
> eInlineFlowMask and eBidiMask are always set together, but from the comment
> I guess eInlineFlowMask won't be set in some direction:rtl cases.  Can you
> help me understand how the other bits (is it eBlockFlowMask and
> eLineOrientMask?) combine with eInlineFlowMask to get me the same value that
> would be stored in eBidiMask?

If I'm understanding/remembering correctly, the case where eInlineFlowMask and eBidiMask diverge will arise when we support text-orientation:sideways-left (or text-orientation:sideways in writing-mode:vertical-lr, where the effective value of text-orientation is the same as explicit sideways-left).

When we have vertical/sideways-left, dir=ltr text will run from bottom to top, and therefore eInlineFlowMask will be set, while eBidiMask will be clear. And conversely, with vertical/sideways-left and dir=rtl, eBidiMask will be set, but eInlineFlowMask will be clear.

Comment 41

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #39)

> and R (a couple of the tests in layout/reftests/writing-mode) 

Ah, I see your try push didn't enable the writing-modes test directory, so we won't be distracted by the known failures there after all. :)

I'll try to think through the various writing-mode bits and see if I can confirm your mapping table. I'm suspicious about the interplay of sideways-left and direction, but we're not in a position to test that yet....

Comment 42

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

Comment on attachment 8544397 [details] [diff] [review]
Part 3.1: Cascade block-axis logical properties with their physical equivalents.

>+  const nsCSSProperty* subprops = nsCSSProps::SubpropertyEntryFor(shorthand);

Please MOZ_ASSERT that subprops[0], subprops[1], subprops[2], and subprops[3] are not eCSSProperty_UNKNOWN and that subprops[4] *is* eCSSProperty_UNKNOWN.

>+  // test each regular expression in inlineMappings against the
>+  // {writing-mode,text-orientation,direction} triple
>+  var key = `${writingMode} ${textOrientation} ${direction}`;
>+  for (var k in inlineMappings) {
>+    if (new RegExp(k).test(key)) {
>+      inlineMapping = inlineMappings[k];
>     }

Maybe break on match?

(And maybe put in a test failure or throw an exception if there is none.)

r=dbaron with tha

Comment 43

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #41)
> I'll try to think through the various writing-mode bits and see if I can
> confirm your mapping table. I'm suspicious about the interplay of
> sideways-left and direction, but we're not in a position to test that yet....

I tried to go through the bit combinations and figure out the inline-{start,end}-to-physical-side mappings from scratch, to compare with your table, and here's what I came up with:

          Orientation
        InlineFlow
      BlockFlow
    LineOrient                         start  end
00  0 0 0 0  H-tb ltr                  L      R
01  0 0 0 1  V-rl ltr                  T      B
02  0 0 1 0  H-tb rtl                  R      L
03  0 0 1 1  V-rl rtl                  B      T
04  0 1 0 0  H-bt ltr (inverted)       R      L
05  0 1 0 1  V-lr rtl sideways-left    T      B
06  0 1 1 0  H-bt rtl (inverted)       L      R
07  0 1 1 1  V-lr ltr sideways-left    B      T
08  1 0 0 0  H-tb rtl (inverted)       L      R
09  1 0 0 1  V-rl rtl sideways-left    T      B
0a  1 0 1 0  H-tb ltr (inverted)       R      L
0b  1 0 1 1  V-rl ltr sideways-left    B      T
0c  1 1 0 0  H-bt ltr                  L      R
0d  1 1 0 1  V-lr ltr                  T      B
0e  1 1 1 0  H-bt rtl                  R      L
0f  1 1 1 1  V-lr rtl                  B      T

where H-tb and (inverted) are hypothetical modes that we don't have any current intention of supporting; and sideways-left is also not yet implemented. So this matches your patch for all the entries that are currently testable, but it looks like I reached the opposite conclusions for the vertical/sideways-left cases.

See if you think this makes sense, in the light of comment 40 ... I don't guarantee that I've been thinking straight here!

Comment 44

[Jean-Yves Perrier [:teoli]]

Added ddn as, when it lands, it is a good opportunity to revisit our documentation of these properties (it is very old, both for look and content).

Quick question: though not on a standard track, these properties are available to websites. Is this correct?

Comment 45

[Cameron McCormack (:heycam)]

(In reply to Jean-Yves Perrier [:teoli] from comment #44)
> Added ddn as, when it lands, it is a good opportunity to revisit our
> documentation of these properties (it is very old, both for look and
> content).

Thanks.

> Quick question: though not on a standard track, these properties are
> available to websites. Is this correct?

They will be available to websites, and they're specified here: http://dev.w3.org/csswg/css-logical-props/

Comment 46

[Cameron McCormack (:heycam)]

(In reply to Jonathan Kew (:jfkthame) from comment #40)
> If I'm understanding/remembering correctly, the case where eInlineFlowMask
> and eBidiMask diverge will arise when we support
> text-orientation:sideways-left (or text-orientation:sideways in
> writing-mode:vertical-lr, where the effective value of text-orientation is
> the same as explicit sideways-left).
> 
> When we have vertical/sideways-left, dir=ltr text will run from bottom to
> top, and therefore eInlineFlowMask will be set, while eBidiMask will be
> clear. And conversely, with vertical/sideways-left and dir=rtl, eBidiMask
> will be set, but eInlineFlowMask will be clear.

OK, I think I understand that now.  I just need to invert eInlineFlowMask to recover the direction value if we had vertical/sideways-left.

(In reply to Jonathan Kew (:jfkthame) from comment #43)
>           Orientation
>         InlineFlow
>       BlockFlow
>     LineOrient                         start  end
> 00  0 0 0 0  H-tb ltr                  L      R
> 01  0 0 0 1  V-rl ltr                  T      B
> 02  0 0 1 0  H-tb rtl                  R      L
> 03  0 0 1 1  V-rl rtl                  B      T
> 04  0 1 0 0  H-bt ltr (inverted)       R      L
> 05  0 1 0 1  V-lr rtl sideways-left    T      B
> 06  0 1 1 0  H-bt rtl (inverted)       L      R
> 07  0 1 1 1  V-lr ltr sideways-left    B      T
> 08  1 0 0 0  H-tb rtl (inverted)       L      R
> 09  1 0 0 1  V-rl rtl sideways-left    T      B
> 0a  1 0 1 0  H-tb ltr (inverted)       R      L
> 0b  1 0 1 1  V-rl ltr sideways-left    B      T
> 0c  1 1 0 0  H-bt ltr                  L      R
> 0d  1 1 0 1  V-lr ltr                  T      B
> 0e  1 1 1 0  H-bt rtl                  R      L
> 0f  1 1 1 1  V-lr rtl                  B      T

Thanks Jonathan.  I think this is starting to clear things up for me.

The lines I am not sure about are the ones with "(inverted)".  The rest I think make sense.

Let's take line 04.  Would "(inverted)" mean something like "when rendering normally vertically oriented characters in the horizontal line of text, do so with their top edges facing the right edge of the page, and with the first character closest to the right edge of the page"?  For example the sentence "The word 中國 means China." would be rendered like this: http://mcc.id.au/temp/text.jpg

Why is line 04 listed as ltr?  LineOrient=0, and we have a text-orientation value that places the inline orthogonal direction text such that its first character is furthest away from the previous horizontal character.  Shouldn't that mean LineOrient=0 actually means rtl?  Or would inversion of the sense of LineOrient (wrt direction) only apply in vertical writing-modes?

Anyway, maybe I'm spending too much time trying to get the right values here since we'll never use them.  I'm just going to use your values for those four lines with "(inverted)".

Comment 47

[Cameron McCormack (:heycam)]

Attachment: Part 3: Add functions to WritingMode to convert between logical and physical sides. (v3)

Comment 48

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #46)

> The lines I am not sure about are the ones with "(inverted)".  The rest I
> think make sense.

Sorry, I didn't clarify what I meant by that (as it's not a case that will actually be implemented). For "inverted", read "horizontal text rotated 180° from its usual appearance". Like the contrast between sideways-left and sideways-right in vertical modes, but we don't actually implement such a thing for horizontal text.

> 
> Let's take line 04.  Would "(inverted)" mean something like "when rendering
> normally vertically oriented characters in the horizontal line of text, do
> so with their top edges facing the right edge of the page, and with the
> first character closest to the right edge of the page"?

No, my understanding is that if this case actually existed, it would mean lines of text rendered something like this (yes, it would affect *all* text, even Latin; compare the 'sideways' orientations in vertical mode):

    ···xoɟ uʍoɹq
       ʞɔınb əɥʇ

Note that:
- line orientation is horizontal (the Orientation bit is clear)
- block progression is from (physical) bottom to top (the BlockFlow bit is set)
- line-relative "top" corresponds to the block-start edge (LineOrient is clear)
  - this is physical *bottom*, hence the upside-down text
- inline LTR (InlineFlow is clear; natural direction of Latin script etc) therefore becomes physically right-to-left

To get horizontal bottom-to-top lines with uninverted text:

    brown fox...
    the quick

you'd need Orientation:0; BlockFlow:1; LineOrient:1, because the line-top here corresponds to the block-end edge. So that's case 0c in the table (or 0e for such lines with RTL direction).

(By "ltr" in the table, btw, I meant line-relative ltr, which in an 180°-rotated line is of course physically rtl. That was also not clear, I guess. That's why line 04 is noted as ltr, but the inline-start edge resolves to R.)

> Anyway, maybe I'm spending too much time trying to get the right values here
> since we'll never use them.  I'm just going to use your values for those
> four lines with "(inverted)".

Sounds OK to me. It's entirely possible that I'm still getting something mixed up, but in practice these are unused anyway.

Comment 49

[Jonathan Kew [:jfkthame]]

Comment on attachment 8547340 [details] [diff] [review]
Part 3: Add functions to WritingMode to convert between logical and physical sides. (v3)

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

::: layout/generic/WritingModes.h
@@ +295,5 @@
> +      { NS_SIDE_LEFT,   NS_SIDE_RIGHT  },  // horizontal-tb                  ltr
> +      { NS_SIDE_TOP,    NS_SIDE_BOTTOM },  // vertical-rl                    ltr
> +      { NS_SIDE_RIGHT,  NS_SIDE_LEFT   },  // horizontal-tb                  rtl
> +      { NS_SIDE_BOTTOM, NS_SIDE_TOP    },  // vertical-rl                    rtl
> +      { NS_SIDE_RIGHT,  NS_SIDE_LEFTT  },  // (horizontal-bt)  (inverted)    ltr

s/LEFTT/LEFT/, as I'm sure the compiler will point out :)

Comment 50

[Cameron McCormack (:heycam)]

Attachment: Part 8: Add border-block-{start,end} logical shorthands.

Comment 51

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

Comment on attachment 8549241 [details] [diff] [review]
Part 8: Add border-block-{start,end} logical shorthands.

r=dbaron, but please don't land without also doing border-inline-{start,end} shorthands

Comment 52

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #51)
> r=dbaron, but please don't land without also doing border-inline-{start,end}
> shorthands

Those are already added as aliases to -moz-border-start etc.  (I forgot that I did do those two when I made bug 1119475 comment 8.)

Comment 53

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

ok, sounds good.

Comment 54

[Cameron McCormack (:heycam)]

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

Comment 55

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8aec6e55befa
https://hg.mozilla.org/integration/mozilla-inbound/rev/48c2205b50a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d58ed9fb838
https://hg.mozilla.org/integration/mozilla-inbound/rev/407730a423a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/37be3bf678f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/9251b40662af
https://hg.mozilla.org/integration/mozilla-inbound/rev/49d54ff37484
https://hg.mozilla.org/integration/mozilla-inbound/rev/83bff13e5f70
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf25b2eac0e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/440d1fea6662

Comment 56

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8aec6e55befa
https://hg.mozilla.org/mozilla-central/rev/48c2205b50a7
https://hg.mozilla.org/mozilla-central/rev/9d58ed9fb838
https://hg.mozilla.org/mozilla-central/rev/407730a423a0
https://hg.mozilla.org/mozilla-central/rev/37be3bf678f7
https://hg.mozilla.org/mozilla-central/rev/9251b40662af
https://hg.mozilla.org/mozilla-central/rev/49d54ff37484
https://hg.mozilla.org/mozilla-central/rev/cf25b2eac0e9
https://hg.mozilla.org/mozilla-central/rev/440d1fea6662

Comment 57

[Sebastian Zartner [:sebo]]

Described the new properties:
https://developer.mozilla.org/en-US/docs/Web/CSS/margin-block-start
https://developer.mozilla.org/en-US/docs/Web/CSS/margin-block-end
https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-start
https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-end
https://developer.mozilla.org/en-US/docs/Web/CSS/padding-block-start
https://developer.mozilla.org/en-US/docs/Web/CSS/padding-block-end
https://developer.mozilla.org/en-US/docs/Web/CSS/padding-inline-start
https://developer.mozilla.org/en-US/docs/Web/CSS/padding-inline-end
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-start-width
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-end-width
https://developer.mozilla.org/en-US/docs/Web/CSS/border-inline-start-width
https://developer.mozilla.org/en-US/docs/Web/CSS/border-inline-end-width
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-start-style
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-end-style
https://developer.mozilla.org/en-US/docs/Web/CSS/border-inline-start-style
https://developer.mozilla.org/en-US/docs/Web/CSS/border-inline-end-style
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-start-color
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-end-color
https://developer.mozilla.org/en-US/docs/Web/CSS/border-inline-start-color
https://developer.mozilla.org/en-US/docs/Web/CSS/border-inline-end-color
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-start
https://developer.mozilla.org/en-US/docs/Web/CSS/border-block-end
https://developer.mozilla.org/en-US/docs/Web/CSS/border-inline-start
https://developer.mozilla.org/en-US/docs/Web/CSS/border-inline-end
https://developer.mozilla.org/en-US/Firefox/Releases/38#CSS

Sebastian

Comment 58

[Sebastian Zartner [:sebo]]

Also, I realized right now that the inline properties still don't seem to consider the writing mode.

Here's a test case:
data:text/html,<!DOCTYPE html><style>p{background:yellow;margin-inline-start:20px;padding-inline-start:20px;border-inline-start:10px solid blue;float:left;}</style><p style="writing-mode:vertical-lr;">Test</p><p style="writing-mode:vertical-rl;">Test</p>

In both cases the margin, padding and border are at the top, while in the second case they should be at the bottom.

Sebastian

Comment 59

[Jonathan Kew [:jfkthame]]

No, it's correct that inline-start is top for both vertical-lr and vertical-rl writing modes. The "-lr" and "-rl" suffixes here refer to the block direction: whether successive lines progress from left to right or right to left. But in both cases, text is written top to bottom, so inline-start is top.

(Unless dir="rtl" is in effect, though we don't fully support that for vertical writing modes yet.)

Comment 60

[Sebastian Zartner [:sebo]]

(In reply to Jonathan Kew (:jfkthame) from comment #59)
> No, it's correct that inline-start is top for both vertical-lr and
> vertical-rl writing modes.

Ah, right. My fault.

> (Unless dir="rtl" is in effect, though we don't fully support that for vertical writing modes yet.)

That's obviously bug 1131451. Thanks for the info!

Sebastian

Comment 61

[Sebastian Zartner [:sebo]]

Everything related to the new properties should be documented by now.

Sebastian

Comment 62

[Cameron McCormack (:heycam)]

Thanks Sebastian!