RTL support for text/plain documents

VERIFIED FIXED in Firefox 53

Status

()

Firefox
General
VERIFIED FIXED
5 years ago
7 months ago

People

(Reporter: tomer, Assigned: tomer)

Tracking

({rtl})

unspecified
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
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).
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.
(Assignee)

Comment 2

5 years ago
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.
Cool, sorry, I forgot to use the -moz- prefix when I tested it out.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8821427 - Flags: review?(jaws) → review?(dbaron)

Comment 6

8 months ago
mozreview-review
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.
Assignee: nobody → tomer.moz.bugs
Status: NEW → ASSIGNED

Comment 7

8 months ago
mozreview-review
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.
Attachment #8821427 - Flags: review?(dbaron) → review+
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Keywords: checkin-needed

Comment 11

8 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0c5acd7cdffc
RTL support for text/plain documents r=dbaron
Keywords: checkin-needed

Comment 12

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0c5acd7cdffc
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Comment 13

8 months ago
Attached testcase seems to be working fine in latest Nightly.
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-2017-01-11]
You need to log in before you can comment on or make changes to this bug.