Closed
Bug 1083134
Opened 10 years ago
Closed 10 years ago
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
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: jfkthame, Assigned: heycam)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete)
Attachments
(11 files, 4 obsolete files)
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 |
No description provided.
Reporter | ||
Comment 1•10 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•10 years ago
|
Blocks: writing-mode
We need to do bug 649142 (and its dependency) before fixing this.
Depends on: 649142
Reporter | ||
Comment 3•10 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)
Updated•10 years ago
|
Comment 5•10 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•10 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•10 years ago
|
Blocks: enable-writing-mode-dev
Assignee | ||
Comment 7•10 years ago
|
||
I can take this once bug 649142 has laid the foundation.
Assignee: nobody → cam
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8543845 -
Flags: review?(dbaron)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8543846 -
Flags: review?(dbaron)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8543697 -
Attachment is obsolete: true
Attachment #8543697 -
Flags: feedback?(jfkthame)
Attachment #8543848 -
Flags: review?(dbaron)
Attachment #8543848 -
Flags: feedback?(jfkthame)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8543849 -
Flags: review?(dbaron)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8543850 -
Flags: review?(dbaron)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8543851 -
Flags: review?(dbaron)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8543853 -
Flags: review?(dbaron)
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Version: unspecified → Trunk
Assignee | ||
Comment 17•10 years ago
|
||
Add some more context in the diff.
Attachment #8543845 -
Attachment is obsolete: true
Attachment #8543845 -
Flags: review?(dbaron)
Attachment #8543858 -
Flags: review?(dbaron)
Assignee | ||
Comment 18•10 years ago
|
||
Reporter | ||
Comment 19•10 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.)
Assignee | ||
Comment 20•10 years ago
|
||
(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+
Assignee | ||
Comment 25•10 years ago
|
||
(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.
Assignee | ||
Comment 28•10 years ago
|
||
(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)
Assignee | ||
Comment 29•10 years ago
|
||
(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)
Assignee | ||
Comment 31•10 years ago
|
||
(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.
Assignee | ||
Comment 32•10 years ago
|
||
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)
Assignee | ||
Comment 33•10 years ago
|
||
Attachment #8544397 -
Flags: review?(dbaron)
Assignee | ||
Comment 34•10 years ago
|
||
Assignee | ||
Comment 35•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=93f1e6baa844 with the vertical text pref forced on
Assignee | ||
Comment 36•10 years ago
|
||
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)
Assignee | ||
Comment 37•10 years ago
|
||
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 38•10 years ago
|
||
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•10 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•10 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•10 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•10 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!
Comment 44•10 years ago
|
||
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
Assignee | ||
Comment 45•10 years ago
|
||
(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/
Assignee | ||
Comment 46•10 years ago
|
||
(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)".
Assignee | ||
Comment 47•10 years ago
|
||
Attachment #8544395 -
Attachment is obsolete: true
Attachment #8544395 -
Flags: review?(jfkthame)
Attachment #8547340 -
Flags: review?(jfkthame)
Reporter | ||
Comment 48•10 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•10 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+
Assignee | ||
Comment 50•10 years ago
|
||
Attachment #8549241 -
Flags: review?(dbaron)
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+
Assignee | ||
Comment 52•10 years ago
|
||
(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.)
ok, sounds good.
Assignee | ||
Comment 54•10 years ago
|
||
Assignee | ||
Comment 55•10 years ago
|
||
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•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 57•10 years ago
|
||
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
Comment 58•10 years ago
|
||
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
Updated•10 years ago
|
Flags: needinfo?(cam)
Reporter | ||
Comment 59•10 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)
Comment 60•10 years ago
|
||
(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•9 years ago
|
||
Everything related to the new properties should be documented by now.
Sebastian
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 62•9 years ago
|
||
Thanks Sebastian!
Blocks: css-logical-1
You need to log in
before you can comment on or make changes to this bug.
Description
•