1127488 - text-align and direction in vertical writing-mode

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Gérard Talbot]

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

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 2

[Gérard Talbot]

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

Comment 3

[Simon Montagu :smontagu]

I have a fix for the testcases linked here, currently testing that it doesn't regress other cases

Comment 4

[Gérard Talbot]

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

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

added for testing completeness purposes.

Comment 5

[Simon Montagu :smontagu]

Attachment: 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.

Comment 6

[Simon Montagu :smontagu]

Attachment: Patch: map correctly between abstract and physical directions of text-align and writing-mode

Comment 7

[Gérard Talbot]

> 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 8

[Jonathan Kew [:jfkthame]]

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.

Comment 9

[Simon Montagu :smontagu]

Attachment: Reftests v.2

As we discussed on IRC, this uses sized inline-blocks instead of Ahem glyphs, which avoids the need for fuzzy matching.

Comment 10

[Simon Montagu :smontagu]

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.

Comment 11

[Jonathan Kew [: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. :)

Comment 12

[Simon Montagu :smontagu]

Fixed by bug 1131013

Checked in the reftests https://hg.mozilla.org/integration/mozilla-inbound/rev/c14208fc79c8.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c14208fc79c8

Comment 14

[Gérard Talbot]

(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

Comment 15

[Gérard Talbot]

err... last link corrected:

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