849404 - RTL support for text/plain documents

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Tomer Cohen :tomer]

Attachment: Hebrew text file

bug 253564 added support for word-wrap for Plain Texts documents, so these documents aren't plain text after all, and after reading JAWS blog post, I've suggested adding RTL support for plain text documents[1], which shouldn't be too difficult as most of the code already landed as part of dir=auto, so adding this feature will probably be a one line addition to the plain text stylesheet, unicode-bidi:plaintext[2].

[1] https://msujaws.wordpress.com/2013/03/08/improved-plain-text-handling-in-firefox/comment-page-1/#comment-8683
[2] https://developer.mozilla.org/en-US/docs/CSS/unicode-bidi

Expected results:
In the attached plain text file, The two lines at the top starts with Latin characters, and thous should be left-aligned. The poem text contains Hebrew characters and should be right-aligned. 

Currently, when browser.bidi.ui is set to true, it is possible to change text alignment using the "Switch Page Direction" item from the context menu, and we should make sure not to break it with a better implementation (i.e., unicode-bidi:plaintext should not be applied after changing page direction manually).

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

I don't know how simple this will be. All of the text of the document is a single block placed in a <pre> tag.

Comment 2

[Tomer Cohen :tomer]

Attachment: screenshot of injected unicode-bidi:-moz-plaintext using Firefox Page Inspector

Injecting unicode-bidi:-moz-plaintext to the <pre> element in the stylesheet seems to do the magic, although it breaks the Switch Page Direction functionality.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Cool, sorry, I forgot to use the -moz- prefix when I tested it out.

Comment 4

[Tomer Cohen :tomer]

Attachment: Bug 849404 RTL support for text/plain documents

The addition of unicode-plaintext to the pre element will make RTL text to appear from right to left. The html[dir] pre rule will make sure that this addition won't break the 'switch page direction' functionality.

Review commit: https://reviewboard.mozilla.org/r/100716/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100716/

Comment 5

[Tomer Cohen :tomer]

Comment on attachment 8821427 [details]
Bug 849404 RTL support for text/plain documents

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100716/diff/1-2/

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8821427 [details]
Bug 849404 RTL support for text/plain documents

https://reviewboard.mozilla.org/r/100716/#review101418

On the surface this looks OK to me, but I am not a layout peer. I've redirected the review to dbaron.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8821427 [details]
Bug 849404 RTL support for text/plain documents

https://reviewboard.mozilla.org/r/100716/#review101420

r=dbaron with the comments below

::: layout/style/res/plaintext.css:12
(Diff revision 2)
>    word-wrap: break-word;
>    -moz-control-character-visibility: visible;
> +  unicode-bidi: plaintext;
> +}
> +
> +html[dir] pre {

You should have a comment above this like the one in your commit message, explaining that the dir attribute on HTML is related to "switch page direction" (what's that?).  You can then remove that comment from your commit message.



I have mixed feeling about whether it would be clearer to just have a single declaration:
html:not([dir]) pre { unicode-bidi: plaintext}
so that you don't have to override the style back to what it was before.  (Since the second declaration you're adding is essentially just restoring the style from before.)  If you think that's clearer, it's fine with me if you also switch to doing that.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And it looks like plaintext.css is only used for text/plain documents, and not for HTML.  I'm certainly assuming that's the case.)

Comment 9

[Tomer Cohen :tomer]

Comment on attachment 8821427 [details]
Bug 849404 RTL support for text/plain documents

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100716/diff/2-3/

Comment 10

[Tomer Cohen :tomer]

Comment on attachment 8821427 [details]
Bug 849404 RTL support for text/plain documents

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100716/diff/3-4/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c5acd7cdffc
RTL support for text/plain documents r=dbaron

Comment 12

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/0c5acd7cdffc

Comment 13

[Itiel]

Attached testcase seems to be working fine in latest Nightly.