476423 - [RTL] Fennec must support rtl

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[dynamis (Tomoya ASAI)]

I made Arabic language pack for Fennec:
http://people.mozilla.com/~dynamis/fennec/arlangpacktest/fennec-1.0a2.ar.langpack-0.0.2009020217.xpi
Install and chang "general.useragent.locale" to "ar" and run fennec.
Then you'll see broken fennec UI:
http://people.mozilla.com/~dynamis/fennec/arlangpacktest/screenshot.png

As you see, smartbar's items are ordered wrongly.
This is because current Fennec UI don't support ltr language properly I believe.

note: I also made Italian langpack in the same way and it works file:
http://people.mozilla.com/~dynamis/fennec/itlangpacktest/fennec-1.0a2.it.langpack-0.0.2009020218.xpi
http://people.mozilla.com/~dynamis/fennec/itlangpacktest/screenshot-1.png
http://people.mozilla.com/~dynamis/fennec/itlangpacktest/screenshot-2.png
http://people.mozilla.com/~dynamis/fennec/itlangpacktest/screenshot-3.png

and Japanese Langpack (original) works of course file:
http://ftp-developer.mozilla-japan.org/pub/mozilla-japan/fennec/nightly/langpack/

So, I believe broken UI of Arabic Fennec is caused by Fennec's bug not supporting ltr (not bug of my script to make langpack).

Comment 1

[dynamis (Tomoya ASAI)]

I meant rtl, not ltr. sorry.

Comment 2

[dynamis (Tomoya ASAI)]

I reproduced on Windows Vista (Vista itself is Japanese) too:
http://people.mozilla.com/~dynamis/fennec/arlangpacktest/screenshot.vista.png

on the other hand linostart say:
there is still no RTL support in the UI, but it's not broken as in your snapshot
http://www.khaledhosny.org/files/tmp/fennec1.png
http://www.khaledhosny.org/files/tmp/fennec2.png

So, this may be one of
 * bug only for win/mac
 * bug relating OS language
   # win/mac screenshot by me is taken on Japanese OS
   # linux screenshotmy linostart is taken on Arabic OS

Comment 3

[Anas Husseini [:linostar]]

Most probably it's a theme-related. In dynamis' shot there is an RTL support but the UI is broken. But in my shot, there is no RTL support because I've manually installed the langpack; when using the addon manager to install it, I got the same results as dynamis.

I believe now that some .css files content need to be modified.

Comment 4

[Tomer Cohen :tomer]

You might be interested in the following article, which document some of the process used for the desktop applications. Fennec is based on the same platform, so it will be also useful for it.

https://developer.mozilla.org/En/Making_Sure_Your_Theme_Works_with_RTL_Locales


(I'm adding 'rtl' keyword to the bug)

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.1

Add support for RTL for:
 * Toolbar
 * Sidebars
 * Right Panels
 * Checkbox

It's a first draft patch.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Fennec toolbar

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Sidebar

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Right Panel

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.1 with the good hg options

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Code clean up and CSS tweaks for checkbox

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

Small changes

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: The images add for RTL

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

Sorry for the spam, I've forgotten the BookmarkList...

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

I think we could update the patch based on bug 478416. However, I'm not sure when that patch will land (and where)

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I'll provide an updated patch :
 * based on 478416
 * Without altering the sidebars position
  Madhava, do we really need to switch their position depending on the locale direction?

Comment 16

[Stuart Parmenter]

-ing this for 1.0

Comment 17

[Tomer Cohen :tomer]

(In reply to comment #16)
> -ing this for 1.0

Please note that unless Fennec will have some basic RTL support, it might be too broken in RTL language packs, and this will block our l10n efforts.

Comment 18

[Axel Hecht [:Pike]]

Anas, Tomer, could we get an update on how Fennec is doing with respect to RTL right now?

The dependent bugs are fixed, and this one is the only open bug with the rtl keyword set. Hard to triage right now, we do have builds for ar and he, but they're missing a good deal of strings right now, too.

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.4

moz-locale-dir is awesome!

This patch add a basic RTL support for Fennec, i supposed some images are bad (go button for example) but this can be adressed as followup.

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.5

some minimal changes

Comment 22

[Tomer Cohen :tomer]

(In reply to comment #20)
> Created an attachment (id=399732) [details]
> Patch v0.5
> 
> some minimal changes

Is there some test build available for testing? I hope this won't miss the 1.0 release.

Do you mind if I'll set it as 'wanted-fennec1.0?'?

Comment 23

[Tomer Cohen :tomer]

I am using Mozilla/5.0 (X11; U; Linux i686;he; rv:1.9.2b1pre) Gecko/20091009 Fennec/1.0b4 and found that chrome://global/locale/global.dtd is hardcoded to 'ltr' because it retrive its data from /fennec/toolkit/chrome/en-US.jar instead of /fennec/chrome/he.jar. Currently this blocking me from fixing bug 522982, and might cause some troubles also for this patch.  

I see it is used by patch v0.5 here - https://bugzilla.mozilla.org/attachment.cgi?id=399732&action=diff#a/chrome/content/browser.xul_sec1

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 399732 [details] [diff] [review]
Patch v0.5

This patch is probably bitrotted now.

I need to check with Mark if the approach was ok, and in this case I'll provide an updated patch as soon as possible.

Comment 25

[Tomer Cohen :tomer]

Tested rc1 - still having some RTL theme issues.

Comment 26

[Juha Seppänen]

Tested with Arabic on Gecko/20100407 Namaroka/3.7a4pre Fennec/1.1a2pre on Mac OS X and the sidestrip for tabs can fill full screen. Couple of screenshots in http://www.flickr.com/photos/33996899@N00/sets/72157623794755302/

Comment 27

[Axel Linder]

Attachment: New and updated version of the patch

This patch is an updated version of the previous one. It fixes some further style issues but doesn't fix the problem of "the sidestrip for tabs can fill full screen" (hint - this _might_ be due to the add-tab icon sticking to the far right). Further the settings-open-rtl-64.png does differ from the one in ltr (at that position) but was provided before - so I used it (just to mention it).

Comment 28

[Jan Arne Petersen]

Attachment: Update patch (and also add more support for rtl tabs)

Comment 29

[Matt Brubeck (:mbrubeck)]

Comment on attachment 462597 [details] [diff] [review]
Update patch (and also add more support for rtl tabs)

># User Axel Linder <[e-mail]>
>Bug 476423 - Fennec must support rtl
>diff -r d122fcec536b chrome/content/browser.js
>--- a/chrome/content/browser.js	Tue Aug 03 13:28:30 2010 -0700
>+++ b/chrome/content/browser.js	Tue Aug 03 16:44:07 2010 -0700
>@@ -979,8 +979,20 @@
>+    let leftbarCBR;
>+    let ritebarCBR;
>+
>+    // Workaround...
>+    // in case of rtl the containers are swapped...so we swap them here, too!
>+    if( (document.getElementById('tabs-container').getBoundingClientRect().left) > (document.getElementById('browser-controls').getBoundingClientRect().left) ) {
>+      // RTL
>+      ritebarCBR  = document.getElementById('tabs-container').getBoundingClientRect();
>+      leftbarCBR = document.getElementById('browser-controls').getBoundingClientRect();
>+    } else {
>+      // LTR
>+      leftbarCBR = document.getElementById('tabs-container').getBoundingClientRect();
>+      ritebarCBR = document.getElementById('browser-controls').getBoundingClientRect();
>+    }

Let's simplify that to something like this:

> let leftbarCBR = document.getElementById('tabs-container').getBoundingClientRect();
> let ritebarCBR = document.getElementById('browser-controls').getBoundingClientRect();
> if( (leftbarCBR..left > ritebarCBR.left) )
>   [ritebarCBR, leftbarCBR] = [leftbarCBR, ritebarCBR]; // RTL



>+          if ((document.getElementById('tabs-container').getBoundingClientRect().left) > (document.getElementById('browser-controls').getBoundingClientRect().left)) {
>+              let controlsWidth = document.getElementById('browser-controls').getBoundingClientRect().width;
>+              let completeWidth = controlsWidth + this.lastChild.boxObject.x + 8;
>+
>+              let boundingBox = this.firstChild.getBoundingClientRect();
>+              let columnsCount = Math.ceil(this.childNodes.length / Math.floor(this.style.height / boundingBox.height));
>+              if (this._columnsCount != columnsCount) {
>+                this.style.width = ( (this.firstChild.boxObject.x - completeWidth) + (2 * boundingBox.width) ) + "px";
>+                this._columnsCount = columnsCount;
>+              }
>+            } else {
>+              let boundingBox = this.firstChild.getBoundingClientRect();
>+              let columnsCount = Math.ceil(this.childNodes.length / Math.floor(this.style.height / boundingBox.height));
>+              if (this._columnsCount != columnsCount) {
>+                this.style.width = (this.lastChild.boxObject.x + boundingBox.width) + "px";
>+                this._columnsCount = columnsCount;
>+              }

Let's get rid of the duplicate code.  Something like this (untested):

>           let tabRect = document.getElementById('tabs-container').getBoundingClientRect();
>           let controlsRect = document.getElementById('browser-controls').getBoundingClientRect();
>           let rtl = (tabRect.left > controlsRect.left);
> 
>           let boundingBox = this.firstChild.getBoundingClientRect();
>           let columnsCount = Math.ceil(this.childNodes.length / Math.floor(this.style.height / boundingBox.height));
>           if (this._columnsCount != columnsCount) {
>             if (rtl) {
>               let completeWidth = controlsRect.width + this.lastChild.boxObject.x + 8;
>               this.style.width = ( (this.firstChild.boxObject.x - completeWidth) + (2 * boundingBox.width) ) + "px"; // rtl
>             } else {
>               this.style.width = (this.lastChild.boxObject.x + boundingBox.width) + "px";
>             }
>             this._columnsCount = columnsCount;
>           }

I'm worried about the magic "8" in there - is that for padding/margins?  Can we read that from the appropriate style instead?  W



>+#identity-box:-moz-locale-dir(rtl) {
>+  min-height: 64px;
>+  min-width: 64px;
>+  background: url("images/rightcap-default-64.png") top left no-repeat;
>+  -moz-box-align: center;
>+  -moz-box-pack: center;
>+}

Only override the "background" property, please - don't duplicate properties that you aren't changing.

> /* make sure this endcap matches the other endcap */
>-#urlbar-icons {
>+#urlbar-icons:-moz-locale-dir(ltr) {
>   min-height: 64px;
>   min-width: 64px;
>   background: url("images/rightcap-default-64.png") top right no-repeat;
>   -moz-box-pack: center;
> }
> 
>+#urlbar-icons:-moz-locale-dir(rtl) {
>+  min-height: 64px;
>+  min-width: 64px;
>+  margin-left: +8px;
>+  background: url("images/leftcap-default-64.png") top right no-repeat;
>+  -moz-box-pack: center;
>+}

Same here - leave the original ruleset alone, and just add a new one overriding the "background" property.

If you can, please use "-moz-margin-end" in a shared rule set instead of margin-left in a rtl rule set.

Remove the "+" from "+8px".

>-#tool-panel-open {
>+#tool-panel-open:-moz-locale-dir(ltr) {
>   list-style-image: url("chrome://browser/skin/images/settings-default-64.png");
>   margin-right: -40px; /* big number just to make sure the image overflows the edge */
> }
> 
>-#tool-panel-open:not([disabled="true"]):hover:active {
>+#tool-panel-open:-moz-locale-dir(rtl) {
>+  list-style-image: url("chrome://browser/skin/images/settings-default-64.png");
>+  margin-left: 0px; /* big number just to make sure the image overflows the edge */
>+}

The comment is no longer accurate.

Can you replace these with a single rule set using -moz-margin-end?

>+#tool-panel-open:not([disabled="true"]):hover:active:-moz-locale-dir(ltr) {
>+  list-style-image: url("chrome://browser/skin/images/settings-active-64.png");
>+}
>+
>+#tool-panel-open:not([disabled="true"]):hover:active:-moz-locale-dir(rtl) {
>   list-style-image: url("chrome://browser/skin/images/settings-active-64.png");
> }

Aren't these the same?

> /* button overflows off the left edge */
>-#tool-panel-close {
>+#tool-panel-close:-moz-locale-dir(ltr) {
>   list-style-image: url("chrome://browser/skin/images/settings-open-64.png");
>   margin-left: -40px; /* big number just to make sure the image overflows the edge */
> }
> 
>+#tool-panel-close:-moz-locale-dir(rtl) {
>+  list-style-image: url("chrome://browser/skin/images/settings-open-64.png");
>+  margin-right: 0px; /* big number just to make sure the image overflows the edge */

Same as above.

>-#tabs-container {
>+#tabs-container:-moz-locale-dir(ltr) {
>   -moz-padding-start: 4px; /* allow the thumbnails to get close to the edge */
>   -moz-padding-end: 8px; /* core spacing */
>   padding-bottom: 8px; /* core spacing */
>   border-right: 3px solid #262629;
> }
> 
>-#tabs {
>+#tabs-container:-moz-locale-dir(rtl) {
>+  display: moz-box;
>+  -moz-column-width: 128px;
>+  -moz-box-align: end;
>+  -moz-box-pack: end;
>+  -moz-padding-start: 4px; /* allow the thumbnails to get close to the edge */
>+  -moz-padding-end: 8px; /* core spacing */
>+  max-width: -moz-max-content;
>+  padding-bottom: 8px; /* core spacing */
>+  border-right: 3px solid #262629;
>+}

Keep the shared values in one ruleset.  You probably want -moz-border-end instead of border-right.

I don't understand why moz-box, moz-column-width, and max-width are needed in rtl only; can you explain?

>-#tabs {
>+#tabs:-moz-locale-dir(ltr) {
>   display: block;
>   -moz-column-width: 128px;
>   -moz-column-gap: 0;
>@@ -740,9 +813,20 @@
>   background-color: transparent;
> }
> 
>+#tabs:-moz-locale-dir(rtl) {
>+  display: block;
>+  -moz-column-width: 128px;
>+  -moz-column-gap: 0;
>+  -moz-user-focus: ignore;
>+  max-width: -moz-max-content;
>+  margin: 0;
>+  padding: 0;
>+  background-color: transparent;
>+}

Same as above - override only the properties that differ.  Why are padding/parming needed?

>-#tabs-controls {
>+#tabs-controls:{

This change is a syntax error.

>   margin-top: 8px; /* core spacing */
>-  -moz-box-pack: start;
>+  -moz-box-pack: end;
> }

I think -moz-back-pack should still be "start".  (You can see this once the selector is fixed, above.)

>diff -r d122fcec536b themes/core/jar.mn

The new image files are not included in this patch.  I assume they are just mirror images of the ltr versions?

> radio:not([disabled=true]):last-child:active:hover,
> radio:last-child[selected] {
>   -moz-border-image: url("chrome://browser/skin/images/toggleright-active-64.png") 8 repeat repeat;
> }
> 
>+radio:last-child[selected]:-moz-locale-dir(rtl) {
>+  -moz-border-image: url("chrome://browser/skin/images/toggleleft-active-64.png") 8 repeat repeat;
>+}

Looks like all of these radio styles fail to override the :active:hover styles.

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

I agree with all of Matt's suggestions

Comment 31

[Jan Arne Petersen]

Attachment: Updated patch.

Fixed issues:

* Simplified Javascript
* Consistently use original ruleset + ruleset for rtl with only modified values
* Use -moz-margin-start/-moz-margin-end where possible instead of a rtl ruleset
* Remove not required #tabs-container, #tabs rules
* Fix syntax error and wrong rule in #tabs-controls
* Fix radio :active:hover styles
* Attached rtl icons to the patch (created with ImageMagick: convert -flop icon.png icon-rtl.png)

Comment 32

[Matt Brubeck (:mbrubeck)]

Comment on attachment 464973 [details] [diff] [review]
Updated patch.

>+++ b/chrome/content/browser.js
>+    if (leftbarCBR.left > ritebarCBR.left) {
>+      // RTL
>+      [ritebarCBR, leftbarCBR] = [leftbarCBR, ritebarCBR];
>+    }

Please put the comment on the same line as the body, and remove curly braces.

>+++ b/chrome/content/browser.xul
>+              if (rtl) {
>+                width = firstBox.right - lastBox.left;
>+              }

Remove curly braces.

>+++ b/themes/core/browser.css
>+#urlbar-icons:-moz-locale-dir(rtl) {
>+  margin-left: 8px;
>+  background: url("images/leftcap-default-64.png") top right no-repeat;
>+}

Why is margin-left needed here?  It adds spacing that is not there in LTR.

>+++ b/themes/core/platform.css
> radio.checkbox-radio-on[selected] {
>   -moz-border-image: url("chrome://browser/skin/images/toggleon-active-64.png") 8 repeat repeat;
> }
> 
>+radio.checkbox-radio-on[selected]:-moz-locale-dir(rtl) {
>+  -moz-border-image: url("chrome://browser/skin/images/toggleon-active-rtl-64.png") 8 repeat repeat;
>+}
>+
> radio.checkbox-radio-off {
>   -moz-border-image: url("chrome://browser/skin/images/toggleoff-inactive-64.png") 8 repeat repeat;
> }
> 
> radio.checkbox-radio-off[selected] {
>   -moz-border-image: url("chrome://browser/skin/images/toggleoff-active-64.png") 8 repeat repeat;
> }

RTL rules are missing for .checkbox-radio-off and possibly some other radio styles.  (See screenshot.)

We need an RTL version of the dropmarker-* images for the menulist. (See screenshot.)

r=mbrubeck with those minor adjustments.  Looks great!

Comment 33

[Matt Brubeck (:mbrubeck)]

Attachment: minor problems with radio, menulist

Comment 34

[Matt Brubeck (:mbrubeck)]

Attachment: minor problem with FindHelperUI

A few more nits:

- margin-left and margin-right for #content-navigator .next-button and .previous-button should be changed to -moz-margin-start and -moz-margin-end.  (See screenshot.)

Need to swap the "arrowright" and "arrowleft" images in #form-helper-autofill for RTL, and change padding-left/border-left/border-right to start/end.

Probably need an RTL version of task-back-40.png, and the #tool-back and #tool-forward button icons.  (I *really* wish we could reverse images in CSS...)

Comment 35

[Jan Arne Petersen]

Yes, I already started yesterday to fix also the smaller issues in #content-navigator and .autocomplete (there icons are shown on the wrong side in rtl).

I will update the patch later today or tomorrow to fix the minor problems from review and add more rtl fixes, which are either mentioned in comment 34 or which I found yesterday.

Comment 36

[Jan Arne Petersen]

Attachment: Updated patch v2

Addressed all mentioned issues.

I also looked for some more rtl-issues in the css files and fixed them.

Comment 37

[Matt Brubeck (:mbrubeck)]

Comment on attachment 465474 [details] [diff] [review]
Updated patch v2

This looks great, thanks!  (And now my brain hurts after testing Fennec in RTL mode.)

Comment 38

[Mark Finkle (:mfinkle) (use needinfo?)]

Great! Let's hold off from checking in until alpha1 wraps up.

Comment 39

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 465474 [details] [diff] [review]
Updated patch v2

This patch switch the right (tabs) and left (controls) sidebars of Fennec. I would like to heard about RTL's people if we want to switch the sidebars or if it is better to keep them as if?

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>--- a/chrome/content/browser.js
>+++ b/chrome/content/browser.js
>@@ -1008,16 +1008,19 @@ var Browser = {
>     }
> 
>     if (!dx) dx = 0;
>     if (!dy) dy = 0;
> 
>     let leftbarCBR = document.getElementById('tabs-container').getBoundingClientRect();
>     let ritebarCBR = document.getElementById('browser-controls').getBoundingClientRect();
> 
>+    if (leftbarCBR.left > ritebarCBR.left)
>+      [ritebarCBR, leftbarCBR] = [leftbarCBR, ritebarCBR]; // switch in RTL case
>+

You could probably add a helper function into BrowserUI to know if the UI is RTL or not (see my patch version 3 or 4 I think)

>diff --git a/chrome/content/tabs.xml b/chrome/content/tabs.xml
>--- a/chrome/content/tabs.xml
>+++ b/chrome/content/tabs.xml
>@@ -121,20 +121,28 @@
>           ]]>
>         </body>
>       </method>
> 
>       <field name="_columnsCount">1</field>
>       <method name="_updateWidth">
>         <body>
>           <![CDATA[
>-            let boundingBox = this.firstChild.getBoundingClientRect();
>-            let columnsCount = Math.ceil(this.childNodes.length / Math.floor(this.style.height / boundingBox.height));
>+            let tabRect = document.getElementById('tabs-container').getBoundingClientRect();
>+            let controlsRect = document.getElementById('browser-controls').getBoundingClientRect();
>+            let rtl = (tabRect.left > controlsRect.left);
>+

Please use the helper to know if we are RTL or not and use this.parentNode if possible to avoid using document.getElementById("tabs-container");
Also please use " instead of ' :)

Thanks a lot for moving forward the patch and doing this work. It was kind of out of my sigh for a while!

Comment 40

[Khaled Hosny]

(In reply to comment #39)
> This patch switch the right (tabs) and left (controls) sidebars of Fennec. I
> would like to heard about RTL's people if we want to switch the sidebars or if
> it is better to keep them as if?

Generally, it is preferred to swap all UI elements. However, I don't see much difference in either position here (since neither denote a start nor an end), but I'd prefer switching the sidebars to be on the safe side.

Comment 41

[(no longer active)]

(In reply to comment #39)
> Comment on attachment 465474 [details] [diff] [review]
> Updated patch v2
> 
> This patch switch the right (tabs) and left (controls) sidebars of Fennec. I
> would like to heard about RTL's people if we want to switch the sidebars or if
> it is better to keep them as if?

Agreed with Khaled here.

> >diff --git a/chrome/content/tabs.xml b/chrome/content/tabs.xml
> >--- a/chrome/content/tabs.xml
> >+++ b/chrome/content/tabs.xml
> >@@ -121,20 +121,28 @@
> >           ]]>
> >         </body>
> >       </method>
> > 
> >       <field name="_columnsCount">1</field>
> >       <method name="_updateWidth">
> >         <body>
> >           <![CDATA[
> >-            let boundingBox = this.firstChild.getBoundingClientRect();
> >-            let columnsCount = Math.ceil(this.childNodes.length / Math.floor(this.style.height / boundingBox.height));
> >+            let tabRect = document.getElementById('tabs-container').getBoundingClientRect();
> >+            let controlsRect = document.getElementById('browser-controls').getBoundingClientRect();
> >+            let rtl = (tabRect.left > controlsRect.left);
> >+
> 
> Please use the helper to know if we are RTL or not and use this.parentNode if
> possible to avoid using document.getElementById("tabs-container");
> Also please use " instead of ' :)

Why can't we just get the computed style for the direction property on the documentElement?

> Thanks a lot for moving forward the patch and doing this work. It was kind of
> out of my sigh for a while!

I'm willing to test a try server patch (which runs on the desktop) and give you guys feedback here.

Comment 42

[Jan Arne Petersen]

(In reply to comment #39)
> You could probably add a helper function into BrowserUI to know if the UI is
> RTL or not (see my patch version 3 or 4 I think)
> 
> Please use the helper to know if we are RTL or not and use this.parentNode if
> possible to avoid using document.getElementById("tabs-container");
> Also please use " instead of ' :)

(In reply to comment #40)
> Why can't we just get the computed style for the direction property on the
> documentElement?

Yes I will add a helper function, will clean the code a bit. I guess it should return the direction of the "controls-scrollbox" element or document (can the interface be mixed ltr/rtl?).

Comment 43

[(no longer active)]

(In reply to comment #42)
> Yes I will add a helper function, will clean the code a bit. I guess it should
> return the direction of the "controls-scrollbox" element or document (can the
> interface be mixed ltr/rtl?).

Yes.  For example, if we choose to display URLs in a piece of UI, that part should be LTR.  It's usually best to get the computed direction style of the element you're dealing with.

Comment 44

[Jan Arne Petersen]

Attachment: Updated patch v4.

Remove the ltr/rtl condition in tabs _updateWidth and just use Math.max.

Instead of adding a helper function to check for rtl, just calculate what is really wanted there.

Comment 45

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mobile-browser/rev/8e1a796c8f49

Comment 46

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Note: for the most part RTL is working in arabic if the plugin is used.  setting to verified : 

verified FIXED on build:
Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b7pre)Gecko/20100916 Firefox/4.0b7pre Fennec/2.0b1pre

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100916 Firefox/Fennec/2.0b1pre

Comment 47

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Initial test cases:
https://litmus.mozilla.org/show_test.cgi?id=12774
https://litmus.mozilla.org/show_test.cgi?id=12817

Comment 48

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

https://litmus.mozilla.org/show_test.cgi?id=13523
https://litmus.mozilla.org/show_test.cgi?id=13524
https://litmus.mozilla.org/show_test.cgi?id=13525
https://litmus.mozilla.org/show_test.cgi?id=13526
https://litmus.mozilla.org/show_test.cgi?id=12818
https://litmus.mozilla.org/show_test.cgi?id=12819

Comment 49

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Need more tests, but will create them as I go along with more experimental testing.