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

VERIFIED FIXED

Status

defect
--
major
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: linostar, Assigned: linostar)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Blocks: 653369, 647841
Assignee: nobody → linux.anas
Status: NEW → ASSIGNED
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #528825 - Flags: review?(21)
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).
Posted patch patch v1.1 (obsolete) — Splinter Review
More variable renaming that I forgot to add earlier.
Attachment #528825 - Attachment is obsolete: true
Attachment #528825 - Flags: review?(21)
Attachment #529054 - Flags: review?(21)
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...
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.
(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.
Posted patch patch v2Splinter Review
Attachment #529054 - Attachment is obsolete: true
Attachment #529054 - Flags: review?(21)
Attachment #529507 - Flags: review?(21)
http://hg.mozilla.org/mozilla-central/rev/1c85998fb15f

Thanks for spending time fixing our rtl's failures.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
So can I mark this bug as verified fixed?
(In reply to comment #9)
> So can I mark this bug as verified fixed?

Yes. Please go ahead.
Ok, thank you Anas:)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.