[RTL] Arabic pages don't render correctly

VERIFIED FIXED in Firefox 7

Status

Fennec Graveyard
General
P3
major
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: tarend, Assigned: vingtetun)

Tracking

({rtl})

Trunk
Firefox 7
ARM
Android

Details

(Whiteboard: [fennec-softblocker])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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!
Bug 526477 seems like it was closed off when it should not have been.
tracking-fennec: ? → 2.0+
Viven - is this a dupe?
Assignee: nobody → 21
(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.
Duplicate of this bug: 636978
See bug 636978 for a different testcase
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>
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?
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
Whiteboard: [fennec-softblocker]
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!
tracking-fennec: 2.0+ → 2.0next+
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.
(Reporter)

Comment 11

7 years ago
User Osama A. reported this as well for HTTP://WWW.op-arab.com
Summary: RTL: Arabic pages don't render correctly → [RTL] Arabic pages don't render correctly
tracking-fennec: 2.0next+ → 6+
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...
Attachment #533103 - Flags: feedback?(ben)
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.
Attachment #533103 - Flags: feedback?(ben) → feedback+
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.
Attachment #533103 - Attachment is obsolete: true
Attachment #533276 - Flags: review?(ben)
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.
Attachment #533276 - Flags: review?(ben) → review-
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.
Attachment #533276 - Attachment is obsolete: true
Attachment #533806 - Flags: review?(ben)
Attachment #533806 - Flags: review?(ben) → review+
http://hg.mozilla.org/mozilla-central/rev/e945acd6e58f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 18

6 years ago
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)
Status: RESOLVED → VERIFIED
tracking-fennec: 6+ → 7+
status-firefox7: --- → fixed
Depends on: 681621
Target Milestone: --- → Firefox 7
You need to log in before you can comment on or make changes to this bug.