Last Comment Bug 634386 - [RTL] Arabic pages don't render correctly
: [RTL] Arabic pages don't render correctly
Status: VERIFIED FIXED
[fennec-softblocker]
: rtl
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: ARM Android
: P3 major (vote)
: Firefox 7
Assigned To: Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
:
Mentors:
: 636978 (view as bug list)
Depends on: 681621
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-15 13:24 PST by Thomas Arend [:tarend]
Modified: 2011-09-02 14:26 PDT (History)
11 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Page cut off on the left (724.06 KB, video/x-ms-wmv)
2011-02-15 13:24 PST, Thomas Arend [:tarend]
no flags Details
wip v0.1 (8.84 KB, patch)
2011-05-17 16:01 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
ben: feedback+
Details | Diff | Splinter Review
Patch (6.39 KB, patch)
2011-05-18 07:40 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
ben: review-
Details | Diff | Splinter Review
Patch v0.2 (8.67 KB, patch)
2011-05-19 14:57 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
ben: review+
Details | Diff | Splinter Review

Description Thomas Arend [:tarend] 2011-02-15 13:24:42 PST
Created attachment 512584 [details]
Page cut off on the left

(1) Go to kooora.com or to http://alwatan.kuwait.tt/ in Fennec

Result: Pages get cut off on the left. Trying to pan opens the left panel
Expected: Pages show in full width or allow panning

The Android stock browser has no issues with these pages!
Comment 1 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-02-16 11:05:30 PST
Bug 526477 seems like it was closed off when it should not have been.
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-02-23 14:06:48 PST
Viven - is this a dupe?
Comment 3 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-02-24 07:48:47 PST
(In reply to comment #2)
> Viven - is this a dupe?

Ok, I've finally find this is not a dupe and this look like a front-end bug. I'm working on it.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-02-26 05:53:52 PST
*** Bug 636978 has been marked as a duplicate of this bug. ***
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2011-02-26 05:54:42 PST
See bug 636978 for a different testcase
Comment 6 :Ehsan Akhgari (away Aug 1-5) 2011-02-28 18:26:42 PST
From bug 636978 comment 0:

You can see the bad result in this page (you cannot scroll left):
http://www.sijp.co.il/files/fennecrtlbug.html
or with this code:
<html dir=rtl><head><style>div {width:200%;height:100%;background-image:
-moz-linear-gradient(left, red, orange, yellow, green, blue, indigo,
violet);}</style></head><body><div></div></body></html>

And the equivalent good LTR result in this page (you can scroll right):
http://www.sijp.co.il/files/fennecltrnobug.html
or with this code:
<html><head><style>div {width:200%;height:100%;background-image:
-moz-linear-gradient(left, red, orange, yellow, green, blue, indigo,
violet);}</style></head><body><div></div></body></html>
Comment 7 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-03-01 06:03:20 PST
Adding Ben in cc since it should understand much better than me what's happening here.

Ben, The view is cut of on the right side because when we receive a MozScrolledAreaChanged the code doesn't expect to have a negative aEvent.x which result into an incorrect document size reported by the content process. (http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings/browser.js#374)

But fixing this place is not enough since the axis off the page is on the top/right corner instead of top/left and so we're supposed to scroll to a negative x positionning.

I think that some code of chrome/content/binding/browser.js and browser.xml should handle such a page but my understanding of the viewport mechanism seems to weak for finding a quick solution to this.
Do you mind having a look at it?
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2011-03-01 12:44:33 PST
After talking to Ben and Vivien, I don't think this bug will make Fennec 4 due to shear size and risk of the work invovled. Making it a softblocker for now, but I am prepared to push it to 2.0next
Comment 9 Benjamin Stover (:stechz) 2011-03-01 12:48:27 PST
Just to be clear, this may not be a big patch but we don't know a lot about it at this point. I know that Fennec makes a lot of assumptions about 0,0 being the top-left corner of the document, so I get the feeling that finding all the problem areas will not be trivial.

Due to other blockers this is low on priority for me, but if anyone has spare time, feel free to take it and run with it!
Comment 10 Anas Husseini [:linostar] 2011-03-31 05:33:23 PDT
The problem might be here:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.xul#285

Notice that the horizontal scroller assumes that the start is at left, and naturally it won't scroll to a negative value. It ought to be "right=0" when direction is rtl.

However, that alone doesn't solve the autofit issue for rtl pages.
Comment 11 Thomas Arend [:tarend] 2011-04-06 09:45:48 PDT
User Osama A. reported this as well for HTTP://WWW.op-arab.com
Comment 12 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-05-17 16:01:15 PDT
Created attachment 533103 [details] [diff] [review]
wip v0.1

Not final yet but works on:
 * HTTP://WWW.op-arab.com
 * http://www.sijp.co.il/files/fennecrtlbug.html
 * ttp://alwatan.kuwait.tt/

Ben, I would like your opinion mostly on the MozScrolledAreaChanged code. I have no idea why it is checking for aEvent.x <= 0 for the moment and I don't want to regress something...
Comment 13 Benjamin Stover (:stechz) 2011-05-17 16:04:38 PDT
That code is from long long ago, back from the days when Roy introduced it into Fennec. I'd originally thought that code was supposed to *help* with RTL, but your change shouldn't break anything LTR so this looks great to me.
Comment 14 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-05-18 07:40:35 PDT
Created attachment 533276 [details] [diff] [review]
Patch

There is still some RTL issues during some panning operations but I think those should be tracked per-bug and none prevent the user to view the full website like what this patch resolve.
Comment 15 Benjamin Stover (:stechz) 2011-05-19 11:20:20 PDT
Comment on attachment 533276 [details] [diff] [review]
Patch

>-        if (json.scrollX >= 0 && json.scrollY >= 0) {
>+        if (json.scrollX || json.scrollY) {

http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/bindings/browser.xml#962

We currently use -1, -1 as a means of saying "don't set scrolling". Perhaps a new object on the JSON object would be best here.

>-        // Adjust width and height from the incoming event properties so that we
>-        // ignore changes to width and height contributed by growth in page
>-        // quadrants other than x > 0 && y > 0.
>-        let scrollOffset = this.getScrollOffset(content);
>-        let x = aEvent.x + scrollOffset.x;
>-        let y = aEvent.y + scrollOffset.y;
>-        let width = aEvent.width + (x < 0 ? x : 0);
>-        let height = aEvent.height + (y < 0 ? y : 0);
>-
>         sendAsyncMessage("MozScrolledAreaChanged", {
>-          width: width,
>-          height: height
>+          width: aEvent.width,
>+          height: aEvent.height,
>+          left: aEvent.x

Much better :)

>               case "MozScrolledAreaChanged":
>-                self._contentDocumentWidth = aMessage.json.width;
>-                self._contentDocumentHeight = aMessage.json.height;
>+                self._contentDocumentWidth = json.width;
>+                self._contentDocumentHeight = json.height;
>+                self._contentDocumentLeft = (json.left < 0) ? json.left : 0;

Why aren't we just saying json.left here? Can json.left be positive?


>-            let position = this.getPosition();
>-            x = Math.floor(Math.max(0, Math.min(self.contentDocumentWidth,  position.x + x)) - position.x);
>-            y = Math.floor(Math.max(0, Math.min(self.contentDocumentHeight, position.y + y)) - position.y);
>             self.contentWindow.scrollBy(x, y);
>           },

This is for local browsers, right? If so, this is much better! :)

>-            if (this._contentView && this._pixelsPannedSinceRefresh > 0) {
>+            if (this._contentView && Math.abs(this._pixelsPannedSinceRefresh) > 0)
>               this._updateCacheViewport();

Nice catch.

Let's fix the -1, -1 issue and re-review.
Comment 16 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-05-19 14:57:19 PDT
Created attachment 533806 [details] [diff] [review]
Patch v0.2

(In reply to comment #15)
> Comment on attachment 533276 [details] [diff] [review] [review]
> Patch
> 
> >-        if (json.scrollX >= 0 && json.scrollY >= 0) {
> >+        if (json.scrollX || json.scrollY) {
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/bindings/
> browser.xml#962
> 
> We currently use -1, -1 as a means of saying "don't set scrolling". Perhaps
> a new object on the JSON object would be best here.

I don't really need this modification, I was probably too angry in my "don't check >= 0" for X rage. I've removed this part.

> >               case "MozScrolledAreaChanged":
> >-                self._contentDocumentWidth = aMessage.json.width;
> >-                self._contentDocumentHeight = aMessage.json.height;
> >+                self._contentDocumentWidth = json.width;
> >+                self._contentDocumentHeight = json.height;
> >+                self._contentDocumentLeft = (json.left < 0) ? json.left : 0;
> 
> Why aren't we just saying json.left here? Can json.left be positive?

As far as I understand the document on http://mdn.beonex.com/en/DOM/Detecting_document_width_and_height_changes it can be.
Comment 17 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-05-20 03:21:15 PDT
http://hg.mozilla.org/mozilla-central/rev/e945acd6e58f
Comment 18 Anna (Waverley) 2011-07-06 06:58:58 PDT
VERIFIED FIXED on:

Build Id: Mozilla /5.0 (Android;Linux armv7l;rv:8.0a1) Gecko/20110706 Firefox/8.0a1 Fennec/8.0a1 

Build Id:Mozilla /5.0 (Android;Linux armv7l;rv:7.0a2) Gecko/20110706 Firefox/7.0a2 Fennec/7.0a2 

Device: HTC Desire Z (Android 2.2)

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