1081742 - Reader mode doesn't support RTL/bidi text (e.g., Arabic)

Status: RESOLVED FIXED

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

Description

[kafha]

Attachment: tmp_14972-Screenshots_2014-10-13-00-13-311259547122.png

User Agent: Mozilla/5.0 (Android; Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
Build ID: 20140923174357

Steps to reproduce:

firefox support arabic font very good


Actual results:

but when i add some of arabic page to read later, font change to left and not support right text alignment in read later list and also when l want to convert this page to pdf


Expected results:

 for arabic when add to read later it should to align right side of page and also when convert to pdf

Comment 1

[Aaron Train [:aaronmt]]

Do you have an example page showcasing this? Maybe this is bug 828922.

Comment 2

[Richard Newman [:rnewman]]

Spotted a user mentioning this in a review on Google Play.

Comment 4

[Aaron Train [:aaronmt]]

Just noting in here based on IRC conversation that I ran into a related problem where RTL text-selection in reader mode will unselect a first word in a sentence, e.g, http://cl.ly/1c3v2V24042x/video.mp4 (mentioned that perhaps element .direction is not maintained)

Comment 5

[Mark Capella [:capella]]

Test case I'm using ... https://www.dropbox.com/s/ik1ks4mhhe0js6k/test_bug1081742.html?dl=0

Comment 6

[Mark Capella [:capella]]

Attachment: bug1081742.diff

This seems to affect desktop too.

I have a basic patch I threw together that seems to work for both platforms, but at this point it depends partially on backing out bug 785992.

So I'll need discussion on why we did that, a way to have nsIParserUtils honor the text direction, or any other more obvious fix I'm unaware of.

I've little experience in this area, so I'm asking bnicholson for feedback since I see his prior involvement with JSDOMParser, and I may have spotted a loosely (un)related issue in getStyle() / setStyle().

Comment 7

[Mark Capella [:capella]]

before [1] / after [2]

[1] https://www.dropbox.com/s/lfco29eosn946cc/bug1081742_noFix.png?dl=0
[2] https://www.dropbox.com/s/5pho43n3a4khjds/bug1081742_fix.png?dl=0

Comment 8

[Mark Capella [:capella]]

Attachment: bug1081742.diff

Tweaked during testing.

Comment 9

[Mark Capella [:capella]]

Comment on attachment 8557515 [details] [diff] [review]
bug1081742.diff

Additional suggested reviewers ...

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 8557515 [details] [diff] [review]
bug1081742.diff

Review of attachment 8557515 [details] [diff] [review]:
-----------------------------------------------------------------

Removing jaws since Margaret and I are now peers of the Reader submodule (https://wiki.mozilla.org/Toolkit/Submodules). Shouldn't need 3 reviewers here.

::: toolkit/components/reader/AboutReader.jsm
@@ -588,5 @@
> -    let parserUtils = Cc["@mozilla.org/parserutils;1"].getService(Ci.nsIParserUtils);
> -    let contentFragment = parserUtils.parseFragment(article.content, Ci.nsIParserUtils.SanitizerDropForms,
> -                                                    false, articleUri, this._contentElement);
> -    this._contentElement.innerHTML = "";
> -    this._contentElement.appendChild(contentFragment);

This wasn't working since the sanitizer strips styles by default. You should be able to fix this by adding the SanitizerAllowStyle flag [1].

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIParserUtils

To answer your question, the sanitizer probably isn't essential since the reader content is sandboxed, and we already strip unwanted elements in Readability.js. It doesn't hurt to have it if we can, though, since it will purge anything we may have missed.

::: toolkit/components/reader/content/JSDOMParser.js
@@ +578,5 @@
>        let attr = this.node.getAttribute("style");
>        if (!attr)
>          return undefined;
>  
> +      let styles = attr.split(";");

Ouch, nice catch.

::: toolkit/components/reader/content/Readability.js
@@ +431,5 @@
>          allElements[i]._index = i;
>        }
>  
>        /**
> +       * Readability returns static node lists, not live ones. When we remove

I think this comment was correct before. JSDOMParser isn't a real standards-compliant parser; things like "document.getElementsByTagName(...)" return a static array instead of a live NodeList. That means if any of these elements are added/removed from the document, the result will not change. This is a limitation of JSDOMParser, not Readability. The latter can still be used without the former, in which case this comment wouldn't apply.

@@ +913,4 @@
>        e.removeAttribute('style');
> +      if (textDirection !== undefined) {
> +        e.setAttribute("style", textDirection);
> +      }

So what's happening here is:
1) You're getting the style from the node
2) The style is being removed from the node
3) The style from #1 is being applied to node

In other words, this block isn't doing anything :) What you want is to keep the direction style while stripping everything else:

let style = e.getStyle("direction");
e.removeAttribute("style");
if (style) {
  e.setStyle("direction", style);
}

@@ +926,4 @@
>            cur.removeAttribute("style");
> +          if (textDirection !== undefined) {
> +            cur.setAttribute("style", textDirection);
> +          }

Same here. I'd recommend creating a _stripNonDirectionStyles() helper that can be called from both places.

Comment 11

[Mark Capella [:capella]]

Thanks for all that ! Not sure how that comment got changed, accidental find/replace I'm guessing.

I'm not sure what you mean by |In other words, this block isn't doing anything|.
Both out intent seems to be to save the |direction| style, remove any other style k:v pairs such as "display:foo", "margin;foo", etc. end reset the saved direction style.

I'll look at your example closer, but adding a helper to JSDOMParser is "yeah let's!" :)

I appreciate the info re: You should be able to fix this by adding the SanitizerAllowStyle flag. TIL moment, and no need to backout useful code.

Comment 12

[Brian Nicholson (:bnicholson)]

(In reply to Mark Capella [:capella] from comment #11)
> I'm not sure what you mean by |In other words, this block isn't doing
> anything|.
> Both out intent seems to be to save the |direction| style, remove any other
> style k:v pairs such as "display:foo", "margin;foo", etc. end reset the
> saved direction style.

Yeah, I could see that was your intent, but I don't think it was quite working that way. `let textDirection = e.getAttribute("style");` gets *all* styles for the element, not just the direction style. So when you reset the style after removing it, you were restoring all other k:v pairs with it.

Comment 13

[Mark Capella [:capella]]

ooops, there it is.

Comment 14

[Mark Capella [:capella]]

Attachment: bug1081742.diff

Cleaner :)

Comment 16

[Brian Nicholson (:bnicholson)]

Comment on attachment 8558275 [details] [diff] [review]
bug1081742.diff

Review of attachment 8558275 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK to me!

::: toolkit/components/reader/content/Readability.js
@@ +929,5 @@
> +   * Remove all style atrributes, preserving RTL/LTR text direction styles.
> +   *
> +   * @param Element
> +   * @return void.
> +   */ 

Nit: whitespace

Comment 17

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Try&rev=21a6344d76cd

Comment 18

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/efd053a1586c

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Boaz Dodin]

It looks that desktop reader mode still doesn't support RTL on this page:
http://hwzone.co.il/%D7%94%D7%A6%D7%A6%D7%94-%D7%9C%D7%9E%D7%95%D7%A6%D7%A8%D7%99-%D7%94%D7%A2%D7%99%D7%93%D7%9F-%D7%94%D7%97%D7%93%D7%A9-%D7%A9%D7%9C-amd-%D7%A9%D7%99%D7%92%D7%99%D7%A2%D7%95-%D7%91%D7%A9%D7%A0%D7%AA-201/

Kubuntu 15.04 64bit, Firefox 39.

Should I open a new bug?

Comment 21

[Kevin Brosnan [Ex-Mozilla]]

Works for me on a Nightly build. http://people.mozilla.org/~kbrosnan/tmp/1081742/rtl-reader.webm

Comment 22

[Boaz Dodin]

(In reply to Kevin Brosnan [:kbrosnan] from comment #21)
> Works for me on a Nightly build.
> http://people.mozilla.org/~kbrosnan/tmp/1081742/rtl-reader.webm

Kevin, it's that I get and it's wrong.
The text should be aligned to right and RTL should be enabled (so the rows with both English and Hebrew text will appear in proper order).

Comment 23

[Mark Capella [:capella]]

mmm ... looks like maybe we had a conflict with the implementation of the new ReaderMode, when it moved to support desktop [0] bug 1134443  (?)

My original patch changed AboutReader.jsm, JSDOMParse.js, and Readability.js.

Bug 1134443 seems to have stepped on Readability.js changes ... looking further ...

[0] http://hg.mozilla.org/mozilla-central/diff/2bbb4ac2a49e/toolkit/components/reader/content/Readability.js

Comment 24

[Mark Capella [:capella]]

The "lost code" in comment #23 is interesting, but seems un-related.

This particular page seems to be setting the body direction via .css [1]

I now recall an earlier irc conversation [2] for this issue, and a tracking bug [3] - "Reader View picks wrong direction for some RTL pages."


[1] view-source:http://hwzone.co.il/wp-content/themes/HWzone%20Theme/style.css?v=0.0753
[2] http://logs.glob.uno/?c=mozilla%23introduction&s=10+Jun+2015&e=10+Jun+2015&h=bidi#c221144
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1173548