653370 - [RTL] Fix computeSidebarVisibility() so that it doesn't confuse tabs panel with controls panel

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anas Husseini [:linostar]]

Apparently, this function should always return the tabs-panel visibility as a first return value and controls-panel visibility as second return value, but for some reason this isn't happening. This is blocking many rtl tab-panel related bugs.

I'll try to fix this issue, and rename some variables to make the code less confusing.

Comment 1

[Anas Husseini [:linostar]]

Attachment: patch v1

Comment 2

[Anas Husseini [:linostar]]

This is a rather a big patch, so I'll explain briefly what I did:

1) renamed varibales (leftXXX and rightXXX to tabsXXX and controlsXXX), to evade the confusion that resulted in several RTL issues.

2) removed the the swapping of sidebars in case of RTL at the begining of computeSidebarVisibility(), because Elements.tabs.X and Elements.controls.X assign sidebars properly independently of browser dir.

3) fixed snapSidebars() so it operates properly on RTL fennec after the removal of the sidebar swapping in 2).

Comment 3

[Anas Husseini [:linostar]]

Attachment: patch v1.1

More variable renaming that I forgot to add earlier.

Comment 4

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

Comment on attachment 529054 [details] [diff] [review]
patch v1.1

>diff -r 037ac82142bd chrome/content/browser-ui.js
>   isTabsVisible: function isTabsVisible() {
>     // The _1, _2 and _3 are to make the js2 emacs mode happy
>-    let [leftvis,_1,_2,_3] = Browser.computeSidebarVisibility();
>-    return (leftvis > 0.002);
>+    let [tabsvis,_1,_2,_3] = Browser.computeSidebarVisibility();
>+    return (tabsvis > 0.002);
>   },

Nit: rename 'tabsvis' to 'tabsVis'

>diff -r 037ac82142bd chrome/content/browser.js
> 
>-    let [leftSidebar, rightSidebar] = [Elements.tabs.getBoundingClientRect(), Elements.controls.getBoundingClientRect()];
>-    if (leftSidebar.left > rightSidebar.left)
>-      [rightSidebar, leftSidebar] = [leftSidebar, rightSidebar]; // switch in RTL case
>+    let [tabsSidebar, controlsSidebar] = [Elements.tabs.getBoundingClientRect(), Elements.controls.getBoundingClientRect()];
>+//    if (tabsSidebar.left > controlsSidebar.left)
>+//      [controlsSidebar, tabsSidebar] = [tabsSidebar, controlsSidebar]; // switch in RTL case

You want to remove this code, right?
 
>   /**
>@@ -889,22 +889,27 @@
>    * @return scrollBy dx needed to make snap happen
>    */
>   snapSidebars: function snapSidebars() {
>-    let [leftvis, ritevis, leftw, ritew] = Browser.computeSidebarVisibility();
>+    let [tabsVis, controlsVis, tabsW, controlsW] = Browser.computeSidebarVisibility();
> 
>     let snappedX = 0;
> 
>-    if (leftvis != 0 && leftvis != 1) {
>-      if (leftvis >= 0.6666) {
>-        snappedX = -((1 - leftvis) * leftw);
>+    // determine browser dir first to know which direction to snap to
>+    let chromeReg = Cc["@mozilla.org/chrome/chrome-registry;1"].
>+                     getService(Ci.nsIXULChromeRegistry);
>+    let dirVal = chromeReg.isLocaleRTL("global") ? -1 : 1;
>+
>+    if (tabsVis != 0 && tabsVis != 1) {
>+      if (tabsVis >= 0.6666) {
>+        snappedX = -((1 - tabsVis) * tabsW) * dirVal;
>       } else {
>-        snappedX = leftvis * leftw;
>+        snappedX = tabsVis * tabsW * dirVal;
>       }
>     }
>-    else if (ritevis != 0 && ritevis != 1) {
>-      if (ritevis >= 0.6666) {
>-        snappedX = (1 - ritevis) * ritew;
>+    else if (controlsVis != 0 && controlsVis != 1) {
>+      if (controlsVis >= 0.6666) {
>+        snappedX = (1 - controlsVis) * controlsW * dirVal;
>       } else {
>-        snappedX = -ritevis * ritew;
>+        snappedX = -controlsVis * controlsW * dirVal;
>       }
>     }
> 

>diff -r 037ac82142bd chrome/content/common-ui.js

>       let toolbarH = BrowserUI.toolbarH;
>-      browserRect = new Rect(leftOffset - rightOffset, Math.max(0, browserRect.top - toolbarH) + toolbarH,
>-                             browserRect.width + leftOffset - rightOffset, browserRect.height - toolbarH);
>+      browserRect = new Rect(tabsOffset - controlsOffset, Math.max(0, browserRect.top - toolbarH) + toolbarH,
>+                             browserRect.width + tabsOffset - controlsOffset, browserRect.height - toolbarH);
>     }

Using tabsOffset - controlsOffset sounds confusing to me because I have to mentaly switch the sidebar in my head.


> 
>     // Ensure parts of the arrowbox are not outside the window
>     let arrowboxRect = Rect.fromRect(container.getBoundingClientRect());
>     if (left + arrowboxRect.width > window.innerWidth)
>       left -= (left + arrowboxRect.width - window.innerWidth);
>-    else if (left < leftOffset)
>-      left += (leftOffset - left);
>+    else if (left < tabsOffset)
>+      left += (tabsOffset - left);
>     container.left = left;

Same as above

>diff -r 037ac82142bd chrome/tests/browser_sidebars.js
>-function checkSidebars(aLeftVisible, aRightVisible) {
>-  let [leftVisibility, rightVisibility, leftWidth, rightWidth] = Browser.computeSidebarVisibility();
>-  let left = Math.abs(leftVisibility - aLeftVisible),
>-      right = Math.abs(rightVisibility - aRightVisible);
>-  ok(left < 0.2, (leftWidth * aLeftVisible) + "px of the left sidebar should be visible (got " + left + ")");
>-  ok(right < 0.2, (rightWidth * aRightVisible) + "px of the right sidebar should be visible (got " + right + ")");
>+function checkSidebars(aTabsVisible, aControlsVisible) {
>+  let [tabsVisibility, controlsVisibility, tabsWidth, controlsWidth] = Browser.computeSidebarVisibility();
>+  let left = Math.abs(tabsVisibility - aTabsVisible),
>+      right = Math.abs(controlsVisibility - aControlsVisible);

I want to think a bit more before deciding if we want the rename, in some ways it sounds to make things more straighforward but on the other hand there is some case where I need to think about the UI direction in my head before doing math...

Comment 5

[Anas Husseini [:linostar]]

If you wish, I can remove the renaming for now to open the way for the other fixes in this patch. We may discuss the renaming afterwards then.

Comment 6

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

(In reply to comment #5)
> If you wish, I can remove the renaming for now to open the way for the other
> fixes in this patch. We may discuss the renaming afterwards then.

I would like it, the renaming could be done into an other bug if needed.

Comment 7

[Anas Husseini [:linostar]]

Attachment: patch v2

Comment 8

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

http://hg.mozilla.org/mozilla-central/rev/1c85998fb15f

Thanks for spending time fixing our rtl's failures.

Comment 9

[Andreea Pod]

So can I mark this bug as verified fixed?

Comment 10

[Anas Husseini [:linostar]]

(In reply to comment #9)
> So can I mark this bug as verified fixed?

Yes. Please go ahead.

Comment 11

[Andreea Pod]

Ok, thank you Anas:)