Last Comment Bug 849404 - RTL support for text/plain documents
: RTL support for text/plain documents
Status: VERIFIED FIXED
: rtl
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
-- normal (vote)
: Firefox 53
Assigned To: Tomer Cohen :tomer
:
:
Mentors:
http://benyehuda.org/rachel/Rac051.html
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-08 14:20 PST by Tomer Cohen :tomer
Modified: 2017-01-11 03:13 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard: [bugday-2017-01-11]
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
Hebrew text file (1.29 KB, text/plain)
2013-03-08 14:20 PST, Tomer Cohen :tomer
no flags Details
screenshot of injected unicode-bidi:-moz-plaintext using Firefox Page Inspector (66.92 KB, image/png)
2013-03-08 14:55 PST, Tomer Cohen :tomer
no flags Details
Bug 849404 RTL support for text/plain documents (59 bytes, text/x-review-board-request)
2016-12-22 20:20 PST, Tomer Cohen :tomer
dbaron: review+
Details | Review

Description User image Tomer Cohen :tomer 2013-03-08 14:20:22 PST
Created attachment 722960 [details]
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 User image Jared Wein [:jaws] (please needinfo? me) 2013-03-08 14:30:30 PST
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 User image Tomer Cohen :tomer 2013-03-08 14:55:52 PST
Created attachment 722972 [details]
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 User image Jared Wein [:jaws] (please needinfo? me) 2013-03-08 15:05:47 PST
Cool, sorry, I forgot to use the -moz- prefix when I tested it out.
Comment 4 User image Tomer Cohen :tomer 2016-12-22 20:20:38 PST Comment hidden (mozreview-request)
Comment 5 User image Tomer Cohen :tomer 2016-12-22 20:29:35 PST Comment hidden (mozreview-request)
Comment 6 User image Jared Wein [:jaws] (please needinfo? me) 2016-12-23 14:46:41 PST
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 User image David Baron :dbaron: ⌚️UTC-7 2016-12-23 15:01:35 PST
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 User image David Baron :dbaron: ⌚️UTC-7 2016-12-23 15:02:27 PST
(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 User image Tomer Cohen :tomer 2016-12-23 15:57:42 PST Comment hidden (mozreview-request)
Comment 10 User image Tomer Cohen :tomer 2016-12-23 16:05:30 PST Comment hidden (mozreview-request)
Comment 11 User image Pulsebot 2017-01-05 20:46:02 PST
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0c5acd7cdffc
RTL support for text/plain documents r=dbaron
Comment 12 User image Iris Hsiao [:ihsiao] 2017-01-06 08:09:46 PST
https://hg.mozilla.org/mozilla-central/rev/0c5acd7cdffc
Comment 13 User image ItielMaN 2017-01-07 07:47:01 PST
Attached testcase seems to be working fine in latest Nightly.

Note You need to log in before you can comment on or make changes to this bug.