Closed Bug 800188 Opened 13 years ago Closed 10 years ago

Reader Mode: lack of RTL support

Categories

(Firefox for Android Graveyard :: Reader View, defect)

Product:
Firefox for Android Graveyard
Component:
Reader View
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX
Milestone:
Firefox 53

People

(Reporter: tomer, Assigned: alexbio2008)

References

(Blocks 2 open bugs)

Details

(Keywords: rtl, Whiteboard: [mentor=lucasr][lang=css][lang=js])

Attachments

(4 files)

changes made so far for "dir" attribute for html tag
6.57 KB, patch
lucasr
: review-
Details | Diff | Splinter Review
the new patch for this bug
3.76 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
Screenshot (globes.co.il)
307.28 KB, image/png
Details
First screen
57.58 KB, image/jpeg
Details
When watching a page in an RTL language and switching to Reader Mode, Firefox for Android should respect the directionality of every page element when reformatting the page. Currently we don't do so, which make the Reader Mode non-functional when content is in one of the RTL languages…
To fix this, we should change these CSS stylesheets to work with RTL, by using things like "text-align: start" instead of "text-align: left", and "-moz-padding-start" instead of "padding-left": http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReader.css http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReaderContent.css If that doesn't fix all the problems, we might also need to make sure that any "dir" attributes from the article's original parents (for example, the <body> element of the original page) are copied into the HTML generated for the about:reader page. The code for that is in this file, and if any fixes are necessary I can try to provide more detail: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Readability.js#1390
Whiteboard: [mentor=mbrubeck][lang=css][lang=js]
if this bug is assigned to nobody I would like to get involved.
(In reply to alex from comment #2) > if this bug is assigned to nobody I would like to get involved. alex - It looks like no one is working on this right now. Feel free to work on it. Here are some resources for getting started: https://wiki.mozilla.org/Mobile/Fennec/Android
I am a newbie and it would be nice to have someone guide me on the steps I need to do. Should I make the changes suggested by Matt Brubeck? I have already build Firefox Nightly.
Yes, you can start by making the changes suggested in comment 1. Then rebuild Firefox, install the new build, and test to see if the problems are solved. Let us know if you have any questions!
Assignee: nobody → alexbio2008
Status: NEW → ASSIGNED
I can make the changes but where to install the new build, and how to test it if I don't have any android device? Is there any way to test it on desktop? I am on Ubuntu 12.04.
Yes, you should be able to use the android emulator to test the changes. See https://wiki.mozilla.org/Mobile/Fennec/Android/Emulator for instructions.
Before making the changes suggested in comment 1, I wanted to see if I can reproduce the bug in order to know what to expect after making those changes. I installed Fennec on Android emulator but I can not enter the Reader Mode, the book icon does not appear. What should I do to enable it?
The reader mode icon will only appear on pages where Fennec finds a "main text" that it can extract from the page. This should usually work on a page that has a single news article or blog post, for example (but not always because the detection is not perfect).
alex, Try this link: http://www.ynet.co.il/articles/0,7340,L-4313634,00.html It should identify "main text", and it's in Hebrew, so you can see that it's displayed correctly.
Reader mode is also disabled on devices with under 512MB of RAM (bug 792603) so you may need to increase the memory available to the emulator.
after I changed "text-align: left" to "text-align: start", and "padding-left" to "-moz-padding-start" in : http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReader.css http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReaderContent.css I see no change, the problem still persists. I rebuilt Fennec with make -f client.mk and make -C objdir-droid package without deleting the objdir-droid folder(which contained the previous build of Fennec). Assuming I rebuilt Fennec correctly, what should I do next?
apparently on some pages it is working, but as far as I can understand the title is not positioned correctly.Here I put 2 screen shots, in first the Reader Mode is off, in the second is on. https://www.dropbox.com/sh/r50k6akbfylgutx/2YaTtO_GFt#/
Please read comment 12 and 13. So what should i do next?
Can you provide the URL of the page for which it is not working? One thing that comes to mind is that perhaps after the reader mode processing some styles or attributes are getting stripped out that might cause this. We should try and figure out what the page DOM looks like before and after the transformation to see why it is positioned correctly before but not after, and then try to find the code responsible for that change.
This is the URL of the page that Reader Mode is working( I putted 2 screen shots in comment 13): http://www.mechon-mamre.org/i/aboutpsq.htm And this is the URL of the page that Reader Mode is not working: http://www.mechon-mamre.org/i/t/t0.htm ( here are 2 screen shots----> https://www.dropbox.com/sh/mcnfua70a2827g6/5s2xkg5qpn in picture1.png the Reader Mode is OFF and in picture2.png the Reader Mode is ON)
Ok, so the next step is to follow the instructions at http://starkravingfinkle.org/blog/2012/10/firefox-for-android-remote-web-console-is-here/ to try and debug the page. You will need to grab a desktop nightly build from http://nightly.mozilla.org/ if you don't have one already, install it, turn on the remote web console feature, turn on the remote debugger in Fennec, and then debug the reader mode page. I suspect that the "dir" attribute on the <HTML> element in the page you linked to is getting lost when we show the reader mode version of the page. You can verify by running | document.documentElement.getAttribute("dir") | in the remote web console to see what that gives. You can also do things like dump document.documentElement.innerHTML to see the serialized DOM that makes up the page. Try to look for changes in the HTML or style that would explain what you see, and try to figure out where in the readability code the problems are coming from. Please ask questions if you get stuck or need any kind of help - this is going to be challenging :)
If it helps, try getting the original computed value of the CSS rule "direction" of the element that is identified as the content to be displayed and applying it to the content that is actually displayed. [I am not a Mozilla developer - just a web developer with strong opinions about RTL bugs. Forgive me if I wrote something silly.]
Well this it what I get: when I run ---document.documentElement.getAttribute("dir")--- on the page with Reader Mode OFF, I get ---"RTL"--- but when Reader Mode is ON the answer is ---"null"---. Then I run ---document.documentElement.setAttribute("dir","rtl")--- on that page with Reader Mode ON, the content is displayed as it should ( from right to left ). So the "dir" attribute is lost when Reader Mode is ON. I think that we must try the last solution mentioned by Matt Brubeck in comment 1, but I need help on that one :).
(In reply to alex from comment #19) > Well this it what I get: > when I run ---document.documentElement.getAttribute("dir")--- on the page > with Reader Mode OFF, I get ---"RTL"--- but when Reader Mode is ON the > answer is ---"null"---. > Then I run ---document.documentElement.setAttribute("dir","rtl")--- on that > page with Reader Mode ON, the content is displayed as it should ( from right > to left ). > So the "dir" attribute is lost when Reader Mode is ON. > I think that we must try the last solution mentioned by Matt Brubeck in > comment 1, but I need help on that one :). The DOM document used in reader mode is different than the original page. You'll probably need to save this information when doing the readability parsing so that you can restore it when reader mode is activated. I expect a patch for this to take a similar approach than this patch I wrote to provider hyphening support in Reader Mode. Have a look at the patch in bug 809841. Let me know if you have any questions.
Whiteboard: [mentor=mbrubeck][lang=css][lang=js] → [mentor=lucasr][lang=css][lang=js]
1. I followed the patch from bug 809841 and I made some modifications, and as far as I can tell Reader Mode works well, except for the title/header? which is still shown on the left side ( because I copied the "dir" attribute only on the content ---this._contentElement.setAttribute("dir",article.dir)---). How do I set the "dir" attribute for the entire document? 2. As Amir said in comment 18, I found a site ( http://www.haaretz.co.il/news/world/middle-east/1.1881463 ) where the direction is set in an external CSS for a div element (the entire site is contained in this div). How do we make Reader Mode to check if a page has the direction style set in one of its elements?
I don't refer to the CSS files that the sites use. There are several ways to apply directionality to an element. My feeling is that the thing that is really supposed to interest the people who will fix this bug is "computed style" - the eventual decision that the rendering engine makes. To see in the desktop Firefox browser, do: * "inspect element" * open the style * press the "computed" button * un-check the "user styles only" box * find the "direction" property. Get the computed direction value of the element as it is displayed in the whole site, and apply that to the element as it is displayed in reader mode. Again, that's the way that I think that it should be fixed, but I'm not a real Firefox developer, and I may be oversimplifying things.
(In reply to alex from comment #21) > 1. I followed the patch from bug 809841 and I made some modifications, and > as far as I can tell Reader Mode works well, except for the title/header? > which is still shown on the left side ( because I copied the "dir" attribute > only on the content > ---this._contentElement.setAttribute("dir",article.dir)---). > How do I set the "dir" attribute for the entire document? > > 2. As Amir said in comment 18, I found a site ( > http://www.haaretz.co.il/news/world/middle-east/1.1881463 ) where the > direction is set in an external CSS for a div element (the entire site is > contained in this div). > How do we make Reader Mode to check if a page has the direction style set in > one of its elements? here I was referring to the fact that the modifications which I made work just with sites that declare the "dir" attribute in the html tag, but do not work with the sites that declare direction of the text in style( inline/global/external ) of an element and not in the "dir" attribute of the html tag. So I think we need a way to make Reader Mode check for "computed style" of the content and see if it has set any "direction". And here I need help with the actual code :).
(In reply to alex from comment #23) > (In reply to alex from comment #21) > > 1. I followed the patch from bug 809841 and I made some modifications, and > > as far as I can tell Reader Mode works well, except for the title/header? > > which is still shown on the left side ( because I copied the "dir" attribute > > only on the content > > ---this._contentElement.setAttribute("dir",article.dir)---). > > How do I set the "dir" attribute for the entire document? > > > > 2. As Amir said in comment 18, I found a site ( > > http://www.haaretz.co.il/news/world/middle-east/1.1881463 ) where the > > direction is set in an external CSS for a div element (the entire site is > > contained in this div). > > How do we make Reader Mode to check if a page has the direction style set in > > one of its elements? > > here I was referring to the fact that the modifications which I made work > just with sites that declare the "dir" attribute in the html tag, but do not > work with the sites that declare direction of the text in style( > inline/global/external ) of an element and not in the "dir" attribute of the > html tag. > So I think we need a way to make Reader Mode check for "computed style" of > the content and see if it has set any "direction". And here I need help with > the actual code :). The readability parsing happens in a separate thread using a chrome worker. See: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7465 We serialize the document and send it as a string to the worker by posting a message to it: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7478 So, the readability parsing code doesn't run on top of a live DOM document, which means that you won't have access to computed style properties while parsing. What you could do is to send the computed direction for the document as an argument to this chrome worker. The problem is that the computed style doesn't necessarily apply to the top level DOM document. It might only apply to some inner div (e.g. in http://www.haaretz.co.il/news/world/middle-east/1.1881463 the direction CSS rule is only applied to the wrapper div inside the page) and you don't want to traverse the whole document to check that as this code would run on the main thread blocking interaction with the page until it's done. In summary: we need to find an efficient way to know the direction of the page so that we can pass this information to the readability chrome worker.
(In reply to alex from comment #21) > 1. I followed the patch from bug 809841 and I made some modifications, and > as far as I can tell Reader Mode works well, except for the title/header? > which is still shown on the left side ( because I copied the "dir" attribute > only on the content > ---this._contentElement.setAttribute("dir",article.dir)---). > How do I set the "dir" attribute for the entire document? You can set the dir attribute on _headerElement to apply direction to the title/credits bits. If you want to apply it to the whole document at once, you can get a reference to the top level document via the _docRef attribute. Just make sure this doesn't break the reader UI in any way.
(In reply to Lucas Rocha (:lucasr) from comment #25) > (In reply to alex from comment #21) > > 1. I followed the patch from bug 809841 and I made some modifications, and > > as far as I can tell Reader Mode works well, except for the title/header? > > which is still shown on the left side ( because I copied the "dir" attribute > > only on the content > > ---this._contentElement.setAttribute("dir",article.dir)---). > > How do I set the "dir" attribute for the entire document? > > You can set the dir attribute on _headerElement to apply direction to the > title/credits bits. If you want to apply it to the whole document at once, > you can get a reference to the top level document via the _docRef attribute. > Just make sure this doesn't break the reader UI in any way. I set the "dir" attribute on _headerElement, and so far at least for sites which declare the "dir" attribute in the html tag everything is looking good (Reader Mode is working properly). As for the other cases I don't know what must be done.
(In reply to alex from comment #26) > (In reply to Lucas Rocha (:lucasr) from comment #25) > > (In reply to alex from comment #21) > > > 1. I followed the patch from bug 809841 and I made some modifications, and > > > as far as I can tell Reader Mode works well, except for the title/header? > > > which is still shown on the left side ( because I copied the "dir" attribute > > > only on the content > > > ---this._contentElement.setAttribute("dir",article.dir)---). > > > How do I set the "dir" attribute for the entire document? > > > > You can set the dir attribute on _headerElement to apply direction to the > > title/credits bits. If you want to apply it to the whole document at once, > > you can get a reference to the top level document via the _docRef attribute. > > Just make sure this doesn't break the reader UI in any way. > > > I set the "dir" attribute on _headerElement, and so far at least for sites > which declare the "dir" attribute in the html tag everything is looking good > (Reader Mode is working properly). > As for the other cases I don't know what must be done. Hey Alex, could you please submit your patch? I think we can probably land it as an initial step and work on the computed style bit on a follow-up bug.
I made the patch with hg diff > patchdir.diff from mozilla-central/ top-level directiory
(In reply to alex from comment #28) > Created attachment 692304 [details] [diff] [review] > changes made so far for "dir" attribute for html tag > > I made the patch with hg diff > patchdir.diff from mozilla-central/ > top-level directiory Thanks for the patch alex! FYI: You have to request a patch review (r?) as you upload the patch. Have a look at this page for instructions: https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed You can request the patch review from me (just type ":lucasr" and select my user on the autocompletion). I strongly recommend you to join our IRC channel (irc.mozilla.org, #mobile channel) to get "real-time" support from us :-)
Attachment #692304 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 692304 [details] [diff] [review] changes made so far for "dir" attribute for html tag Review of attachment 692304 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! The only problem is that the patch contains a bunch of changes that just removes trailing spaces. Could you please remove these from the patch?
Attachment #692304 - Flags: review?(lucasr.at.mozilla) → review-
Attachment #700296 - Flags: review?
Attachment #700296 - Flags: review? → review?(lucasr.at.mozilla)
Attachment #700296 - Flags: review?(lucasr.at.mozilla) → review+
Filed bug 828922 to track the case of direction being defined in CSS btw.
https://hg.mozilla.org/mozilla-central/rev/f932b3812c63
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Depends on: 830308
This entirely broke Reader Mode; see bug 830308. Please back-out.
Ignore above comment; I don't think this bug was at fault.
Blocks: Persian
(In reply to rafi wiener from comment #37) > this bug still exists! I can confirm this. I'm not sure if it ever got fixed, as I don't really remember having this working well.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please file a new bug. Because of the way Mozilla's bugfixing process works reopening a bug that has been closed for years gets very complicated.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 10 years ago
Resolution: --- → FIXED
Screenshot of the first link from comment 37. Expected result is having the text flow from the right edge of the screen and not left aligned.
Blocks: 1232445
Attached image First screen —
Reader Mode had changed a lot - this is the first screen - Can it be confirmed by a native speaker?
QA Contact: ioana.chiorean
Target Milestone: Firefox 21 → Firefox 53
Version: unspecified → Trunk
Follow up bug: Bug 1331989 - [RTL] Reader Mode Settings not mirrored Setting the bug to wontfix as this version of Reader Mode is outdated
Resolution: FIXED → WONTFIX
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: