Closed Bug 634386 Opened 13 years ago Closed 13 years ago

[RTL] Arabic pages don't render correctly

Categories

(Firefox for Android Graveyard :: General, defect, P3)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
ARM
Android
Type:
defect

Tracking

(firefox7 fixed, fennec7+)

Status:
VERIFIED FIXED
Milestone:
Firefox 7
Tracking Flags:
Tracking Status
firefox7 --- fixed
fennec 7+ ---

People

(Reporter: tarend, Assigned: vingtetun)

References

Details

(Keywords: rtl, Whiteboard: [fennec-softblocker])

Attachments

(2 files, 2 obsolete files)

Page cut off on the left
724.06 KB, video/x-ms-wmv
Details
Patch v0.2
8.67 KB, patch
stechz
: review+
Details | Diff | Splinter Review 
(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.
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.
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+
Attached patch wip v0.1 (obsolete) — — Splinter Review 
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+
Attached patch Patch (obsolete) — — Splinter Review 
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-
Attached patch Patch v0.2 — — Splinter Review 
(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
Closed: 13 years ago
Resolution: --- → FIXED
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.

Attachment

General

Creator:
Created:
Updated:
Size: