455113 - Tabbar and control-bar should not show/hide with the same drag event

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Oleg Romashin (:romaxa)]

Steps: 
1. Drag from left -> right
2. Tab Bar is visible
3. Drag from right -> left

EXPECTED OUTCOME:
Tab Bar disappear, only layout visible

ACTUAL OUTCOME:
Tab Bar disappear, Control bar appears on the right side....

I think this is not very good from usability point of view...
Sometime it is very hard to make visible only layout on screen (without any bars...)

Comment 2

[Oleg Romashin (:romaxa)]

I think I have to reopen this bug.
I still very often able to open Tabbar while closing "Control bar" and vice versa.

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Proposed fix

Comment 4

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

Comment on attachment 341724 [details] [diff] [review]
Proposed  fix

First pass review:

Looks like you wanted to use draggingData.downUiMode to hold the original "mode", but then added this.mouseDownMode. Let's switch to use dragginData.uiMode instead of this.mouseDownMode.

I still need to apply the patch and test.

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Updated from comment patch

Comment 6

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

Comment on attachment 341744 [details] [diff] [review]
Updated from comment patch

>diff -r 9f1ea2560899 chrome/content/browser-ui.js
>--- a/chrome/content/browser-ui.js	Sat Oct 04 03:52:16 2008 -0400
>+++ b/chrome/content/browser-ui.js	Sat Oct 04 16:17:40 2008 -0400
>@@ -208,17 +208,18 @@ var BrowserUI = {
>     dragging : false,
>     startX : 0,
>     startY : 0,
>     dragX : 0,
>     dragY : 0,
>     lastX : 0,
>     lastY : 0,
>     sTop : 0,
>-    sLeft : 0
>+    sLeft : 0,
>+    uiMode : UIMODE_NONE
>   },
> 
>   _scrollToolbar : function bui_scrollToolbar(aEvent) {
>     var [scrollWidth, ] = Browser.content._contentAreaDimensions;
>     var [viewportW, ] = Browser.content._effectiveViewportDimensions;
> 
>     var pannedUI = false;
> 
>@@ -282,16 +283,30 @@ var BrowserUI = {
>         let contentW = gContentBox.boxObject.width;
> 
>         // Limit the panning
>         if (newLeft > 0)
>           newLeft = 0;
>         else if (newLeft < -(tabbarW + sidebarW))
>           newLeft = -(tabbarW + sidebarW);
> 
>+        // Set the UI mode based on where we ended up
>+        if (newLeft > -(tabbarW - tabbarW / 3) && newLeft <= 0) {
>+          if (this._dragData.uiMode == UIMODE_CONTROLS)
>+            return;
>+          this.mode = UIMODE_TABS;
>+        }
>+        else if (newLeft >= -(tabbarW + sidebarW) && newLeft < -(tabbarW + sidebarW / 3)) {
>+          if (this._dragData.uiMode == UIMODE_TABS)
>+            return;
>+          this.mode = UIMODE_CONTROLS;
>+        }
>+        else
>+          this.mode = (gContentBox.style.marginTop == "0px" ? UIMODE_NONE : UIMODE_URLVIEW);
>+

A couple of nits here:
* this.mode should be reset, even if we return early
* use braces on the "else"

Here's a possible version that fixes both of those:

        // Set the UI mode based on where we ended up
        var noneMode = (gContentBox.style.marginTop == "0px" ? UIMODE_NONE : UIMODE_URLVIEW);
        if (newLeft > -(tabbarW - tabbarW / 3) && newLeft <= 0) {
          if (this._dragData.uiMode == UIMODE_CONTROLS) {
            this.mode = noneMode;
            return;
          }
          this.mode = UIMODE_TABS;
        }
        else if (newLeft >= -(tabbarW + sidebarW) && newLeft < -(tabbarW + sidebarW / 3)) {
          if (this._dragData.uiMode == UIMODE_TABS) {
            this.mode = noneMode;
            return;
          }
          this.mode = UIMODE_CONTROLS;
        }
        else {
          this.mode = noneMode;
        }


r+ with those addressed
>         tabbar.left = newLeft;
> 
>         // Never let the toolbar pan off the screen
>         let newToolbarLeft = newLeft;
>         if (newToolbarLeft < 0)
>           newToolbarLeft = 0;
>         toolbar.left = newToolbarLeft;
> 
>@@ -299,24 +314,16 @@ var BrowserUI = {
>         if (newLeft + tabbarW != 0)
>           toolbar.top = 0;
>         else
>           toolbar.top = this._contentTop - toolbar.boxObject.height;
> 
>         this._setContentPosition("left", newLeft + tabbarW);
>         sidebar.left = newLeft + tabbarW + contentW;
>         panelUI.left = newLeft + tabbarW + contentW + sidebarW;
>-
>-        // Set the UI mode based on where we ended up
>-        if (newLeft > -(tabbarW - tabbarW / 3) && newLeft <= 0)
>-          this.mode = UIMODE_TABS;
>-        else if (newLeft >= -(tabbarW + sidebarW) && newLeft < -(tabbarW + sidebarW / 3))
>-          this.mode = UIMODE_CONTROLS;
>-        else
>-          this.mode = (gContentBox.style.marginTop == "0px" ? UIMODE_NONE : UIMODE_URLVIEW);
> 
>         pannedUI = true;
>       }
>     }
> 
>     if (pannedUI) {
>       aEvent.stopPropagation();
> 
>@@ -798,19 +805,21 @@ var BrowserUI = {
>       case "mousedown":
>         this._dragData.dragging = true;
>         this._dragData.dragX = 0;
>         this._dragData.dragY = 0;
>         this._dragData.screenX = this._dragData.lastX = aEvent.screenX;
>         this._dragData.screenY = this._dragData.lastY = aEvent.screenY;
>         this._dragData.sTop = document.getElementById("toolbar-main").top;
>         this._dragData.sLeft = document.getElementById("tab-list-container").left;
>+        this._dragData.uiMode = this.mode;
>         break;
>       case "mouseup":
>         this._dragData.dragging = false;
>+        this._dragData.uiMode = UIMODE_NONE;
>         // Cause the UI to snap, if needed
>         this._showPanel(this.mode);
>         break;
>       case "mousemove":
>         this._scrollToolbar(aEvent);
>         break;
>       // Window size events
>       case "resize":

Comment 7

[Oleg Romashin (:romaxa)]

Attachment: addon to 341744

Comment 8

[Oleg Romashin (:romaxa)]

Pushed in:
http://hg.mozilla.org/mobile-browser/rev/09147f896864
http://hg.mozilla.org/mobile-browser/rev/aba146d25d5d

Comment 9

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

this logic has been reworked as of beta3.