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

RESOLVED FIXED in mozilla38

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: jfkthame, Assigned: heycam)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

Trunk
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 4 obsolete attachments)

2.20 KB, text/html
Details
8.03 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.36 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.32 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.70 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
21.65 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.18 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.94 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.44 KB, patch
pbro
: review+
Details | Diff | Splinter Review
7.42 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
7.02 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: writing-mode
We need to do bug 649142 (and its dependency) before fixing this.
Depends on: 649142
(Reporter)

Comment 3

5 years ago
(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?
Flags: needinfo?(dbaron)
Nobody is working on it, but I would not consider the interim fix; it requires far too many hidden properties.
Flags: needinfo?(dbaron)
Blocks: 1079125
No longer blocks: writing-mode
(Reporter)

Updated

5 years ago
Blocks: 1102177

Comment 5

4 years ago
-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
(Reporter)

Comment 6

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

Updated

4 years ago
I can take this once bug 649142 has laid the foundation.
Assignee: nobody → cam
Duplicate of this bug: 1114875
Posted patch WIP v0.1 (obsolete) — Splinter Review
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.
Attachment #8543697 - Flags: feedback?(jfkthame)
Attachment #8543697 - Attachment is obsolete: true
Attachment #8543697 - Flags: feedback?(jfkthame)
Attachment #8543848 - Flags: review?(dbaron)
Attachment #8543848 - Flags: feedback?(jfkthame)
Attachment #8543853 - Flags: review?(dbaron)
Status: NEW → ASSIGNED
Version: unspecified → Trunk
Add some more context in the diff.
Attachment #8543845 - Attachment is obsolete: true
Attachment #8543845 - Flags: review?(dbaron)
Attachment #8543858 - Flags: review?(dbaron)
(Reporter)

Comment 19

4 years ago
(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.)
(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.
Flags: needinfo?(jfkthame)
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!)
Attachment #8543858 - Flags: review?(dbaron) → review+
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
Attachment #8543846 - Flags: review?(dbaron) → review+
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.
Attachment #8543848 - Flags: review?(dbaron)
Attachment #8543849 - Flags: review?(dbaron) → review+
Attachment #8543850 - Flags: review?(dbaron) → review+
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)
Attachment #8543851 - Flags: review?(dbaron) → review+
(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 on attachment 8543853 [details] [diff] [review]
Part 7: Tests.

I didn't look too closely, but the stuff you're testing looks good.
Attachment #8543853 - Flags: review?(dbaron) → review+
(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.
(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?
Flags: needinfo?(dbaron)
(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}.)
(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.
Flags: needinfo?(dbaron)
(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.
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.
Attachment #8543848 - Attachment is obsolete: true
Attachment #8543848 - Flags: feedback?(jfkthame)
Attachment #8544395 - Flags: review?(jfkthame)
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.
Attachment #8544499 - Flags: review?(pbrosset)
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.
Attachment #8544499 - Flags: review?(pbrosset) → review+
(Reporter)

Comment 39

4 years ago
(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.
(Reporter)

Comment 40

4 years ago
(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.
Flags: needinfo?(jfkthame)
(Reporter)

Comment 41

4 years ago
(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 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
Attachment #8544397 - Flags: review?(dbaron) → review+
(Reporter)

Comment 43

4 years ago
(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!
(Reporter)

Updated

4 years ago
Blocks: 1119475
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?
Keywords: dev-doc-needed
(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/
(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)".
(Reporter)

Comment 48

4 years ago
(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.
(Reporter)

Comment 49

4 years ago
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 :)
Attachment #8547340 - Flags: review?(jfkthame) → review+
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
Attachment #8549241 - Flags: review?(dbaron) → review+
(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.)
(Reporter)

Updated

4 years ago
Depends on: 1131994
See Also: → 1132859
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
Depends on: 1132889
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
Flags: needinfo?(cam)
(Reporter)

Comment 59

4 years ago
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.)
Flags: needinfo?(cam)
(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
Everything related to the new properties should be documented by now.

Sebastian
Thanks Sebastian!
You need to log in before you can comment on or make changes to this bug.