text-align and direction in vertical writing-mode

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bugzilla, Assigned: smontagu)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
mozilla38
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: Updated links to tests are in comment 14)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Tests
-----

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-text-align-vertical-rl-004.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-text-align-vertical-lr-005.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-text-align-vertical-rl-016.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-text-align-vertical-lr-017.xht

Expected results
----------------

A green square and no red

Explanation
-----------
The initial, default text-align given by direction declaration must be overriden by text-align declaration. Eg. 'direction: rtl; text-align: left' implies that 'text-align: left' has precedence.

Notes
-----
- I have not yet submitted those tests to the test.csswg.org repository but I will, along with correspondent reference files
- IE11, Chrome 40.0.2214.93 pass these 4 tests. Prince 9.0.5 also passes the 2 vertical-rl ones (004 and 016 tests)
- I am using Firefox 38.0a1 buildID=20150125230903
- I use Linux 3.16.0-29-generic x86_64, Qt: 4.8.6, KDE 4.14.1; Kubuntu (utopic) 14.10
- I've searched for duplicates and did not find any
(Reporter)

Updated

4 years ago
Blocks: 145503
Keywords: testcase
(Reporter)

Updated

4 years ago
OS: Linux → All
Hardware: x86_64 → All
I guess we currently do not handle RTL on vertical writing mode at all. The inline-start should be bottom if writing-mode is vertical and direction is RTL, but it seems we always treat inline-start as top in vertical text.
(Reporter)

Comment 2

4 years ago
Xidorn Quan,

You are correct and your comment is excellent. I am quickly adding 2 additional tests based on your useful feedback:

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s62-direction-vrl-002.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s62-direction-vlr-003.xht

Gérard
(Assignee)

Comment 3

4 years ago
I have a fix for the testcases linked here, currently testing that it doesn't regress other cases
Assignee: nobody → smontagu
(Assignee)

Comment 5

4 years ago
Created attachment 8560974 [details] [diff] [review]
Reftests

I thought it wise to test (almost) all the combinations and permutations of direction, writing-mode and text-align while we're here. text-align: justify seems like a different ball game and not relevant to this bug. I hesitated about text-align: center and eventually decided to leave it out, but I'll add it if you think it's worth it.
Attachment #8560974 - Flags: review?(jfkthame)
(Assignee)

Comment 6

4 years ago
Created attachment 8560975 [details] [diff] [review]
Patch: map correctly between abstract and physical directions of text-align and writing-mode
Attachment #8560975 - Flags: review?(jfkthame)
(Reporter)

Comment 7

4 years ago
> I thought it wise to test (almost) all the combinations and permutations of
> direction, writing-mode and text-align while we're here. 

Yes, it is wise and better.

> text-align: justify
> seems like a different ball game and not relevant to this bug. I hesitated
> about text-align: center and eventually decided to leave it out, but I'll
> add it if you think it's worth it.

Huh... it is okay with me to leave out 'text-align: center'. I am not sure why you wanted my opinion (or approval? or consent?) on this here.
Anyway... 22 tests testing combinations of direction (rtl, ltr, default), text-align (left, right, center, default), vertical writing-mode (vertical-rl, vertical-lr) have been submitted to test.csswg.org :

http://lists.w3.org/Archives/Public/public-css-testsuite/2015Feb/0002.html

There ought to be a way for all sides involved here to reduce duplicate efforts when creating tests and reftests... Ideally, if the folders structure was the same at both (mozilla-central/source/layout/reftests/writing-mode/ and http://test.csswg.org/source/writing-mode/) places... Mozilla does not make use of a /support folder but uses a ../fonts/ folder.

----------

Just curious... I wonder what "fuzzy(255,402)" actually means or refer to.
Comment on attachment 8560975 [details] [diff] [review]
Patch: map correctly between abstract and physical directions of text-align and writing-mode

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

::: layout/generic/nsLineLayout.cpp
@@ +3018,5 @@
> +     *
> +     * In vertical modes, align-left and align-right map to line-left
> +     * and line-right, i.e. logical inline start and end, but
> +     * align-start and align-end map to inline start and end for ltr,
> +     * and inline end and start for rtl.

I'm struggling to understand this, although the patch as written seems to provide the expected results.

When writing-mode is vertical and dir is RTL, inline-start should be physical bottom, and align-start should still map to inline-start, just as it does in horizontal mode. And align-left would map to line-left, which is physical top but logical inline end.

My suspicion is that we're failing to handle RTL inline dir properly for vertical mode at some other level, and that's why this patch currently results in the correct alignment: a case of two wrongs making a right. Maybe that's OK as an interim fix, but I'd like to understand this better -- or see where I'm currently misunderstanding it all -- before we press ahead.

Note that we are not doing bidi reordering properly in vertical mode; I wonder if fixing that would result in things making better sense here too? I've just filed bug 1131013 on this.
(Assignee)

Comment 9

4 years ago
Created attachment 8561957 [details] [diff] [review]
Reftests v.2

As we discussed on IRC, this uses sized inline-blocks instead of Ahem glyphs, which avoids the need for fuzzy matching.
Attachment #8560974 - Attachment is obsolete: true
Attachment #8560974 - Flags: review?(jfkthame)
Attachment #8561957 - Flags: review?(jfkthame)
(Assignee)

Comment 10

4 years ago
Comment on attachment 8560975 [details] [diff] [review]
Patch: map correctly between abstract and physical directions of text-align and writing-mode

If we take the patch for bug 1131013, we don't need this.
Attachment #8560975 - Attachment is obsolete: true
Attachment #8560975 - Flags: review?(jfkthame)
Comment on attachment 8561957 [details] [diff] [review]
Reftests v.2

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

They look reasonable but I didn't go through them all individually! Anyway, rs=me. :)
Attachment #8561957 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 12

4 years ago
Fixed by bug 1131013

Checked in the reftests https://hg.mozilla.org/integration/mozilla-inbound/rev/c14208fc79c8.
Depends on: 1131013
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/c14208fc79c8
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Reporter)

Comment 14

3 years ago
(In reply to Gérard Talbot from comment #0)
> Tests
> -----
> 
> http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-text-align-
> vertical-rl-004.xht
> 
> http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-text-align-
> vertical-lr-005.xht
> 
> http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-text-align-
> vertical-rl-016.xht
> 
> http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-text-align-
> vertical-lr-017.xht


These tests have been filename-renamed as following:


http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-text-align-vrl-004.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-text-align-vlr-005.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-text-align-vrl-016.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-text-align-vlr-017.xht

and those 4 tests along with 14 other tests have been submitted to the test.csswg.org repository in section #s71 :

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/text-align-vrl-002.htm

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/text-align-vrl-004.htm

...

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/text-align-vrl-016.htm

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/text-align-vrl-018.htm


and


http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/text-align-vlr-003.htm

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/text-align-vlr-005.htm

...

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/text-align-vlr-017.htm

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/text-align-vlr-0019.htm
Whiteboard: Updated links to tests are in comment 14
You need to log in before you can comment on or make changes to this bug.