text-orientation:upright should not affect bidi directionality in sideways-* writing modes

RESOLVED FIXED in Firefox 50

Status

()

Core
Layout: Text
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
Created attachment 8764196 [details]
testcase: sideways-* writing modes, text-orientation:upright, and bidi

In bug 1157752, we made text-orientation:upright force a left-to-right direction override (as per writing-modes spec) in vertical writing modes.

That was fine at the time, but since then we've added the new sideways-* modes, which are also vertical, but do not support text-orientation:upright.

In those modes, just as text-orientation:upright does NOT force upright glyph rendering, it should NOT result in forced LTR directionality. (See testcase.)
(Assignee)

Comment 1

2 years ago
Created attachment 8764199 [details] [diff] [review]
Don't let text-orientation:upright affect directionality in sideways-* writing modes

This is a trivial fix in itself, but it may overlap with the work you're doing in bug 1160847; if it's more convenient to finish that first, before we land this change, that's fine with me.
Attachment #8764199 - Flags: review?(bugzilla)
(Assignee)

Updated

2 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8764201 [details] [diff] [review]
Reftest to check that text-orientation:upright does not override bidi in sideways-* modes

The testcase in reftest form.
Attachment #8764201 - Flags: review?(bugzilla)
Comment on attachment 8764199 [details] [diff] [review]
Don't let text-orientation:upright affect directionality in sideways-* writing modes

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

Looks fine to me.

::: layout/base/nsBidiPresUtils.cpp
@@ +85,1 @@
>        vis->mTextOrientation == NS_STYLE_TEXT_ORIENTATION_UPRIGHT) {

I wonder whether it is easier to just
> WritingMode wm(aStyleContext);
> if (wm.IsVertical() && !wm.IsSideways()) {
Attachment #8764199 - Flags: review?(bugzilla) → review+
Attachment #8764201 - Flags: review?(bugzilla) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #1)
> Created attachment 8764199 [details] [diff] [review]
> Don't let text-orientation:upright affect directionality in sideways-*
> writing modes
> 
> This is a trivial fix in itself, but it may overlap with the work you're
> doing in bug 1160847; if it's more convenient to finish that first, before
> we land this change, that's fine with me.

I wouldn't expect that bug can finish quicker than this :)
I actually think the test should go to w3c-css/submitted/writing-modes-3.
(Assignee)

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e8426231b72c1f6d1b8fd56b4c4055147e3a99a
Bug 1281424 - Don't let text-orientation:upright affect directionality in sideways-* writing modes. r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f0ef528a365084479353b52accf835fea7dcdd2
Bug 1281424 - Reftest to check that text-orientation:upright does not override bidi in sideways-* modes. r=xidorn

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0e8426231b72
https://hg.mozilla.org/mozilla-central/rev/0f0ef528a365
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Sorry for missing this at the time, but I noticed:
https://test.csswg.org/shepherd/reference/text-orientation-upright-directionality-001-ref/ says:

Reference must not have specification links.
Status: Needs Work

Would you mind removing the spec link from the reference?
Flags: needinfo?(jfkthame)
(Assignee)

Comment 9

2 years ago
Created attachment 8826216 [details] [diff] [review]
followup, remove spec link from reference file

Better?
(Assignee)

Comment 10

2 years ago
(As :dbaron isn't accepting r? at the moment, and this seems too trivial to really need it anyway, I'll just push it to inbound with DONTBUILD.)
Flags: needinfo?(jfkthame)
You need to log in before you can comment on or make changes to this bug.