452069 - Off-screen UI should slide into view when dragged

Status: VERIFIED FIXED

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

Description

[Madhava Enros [:madhava]]

The control strip and the tab area should, when dragged into view, slide into place as if the content area were moving over and bring them along with them.  They should also snap into place when pulled far enough into view.  At the moment, these areas simply appear.

This intended behavior is more thoroughly described here:

https://wiki.mozilla.org/Mobile/UI/Designs/TouchScreen/workingUI#Basic_Usage

and here:

https://wiki.mozilla.org/Mobile/UI/Designs/TouchScreen/workingUI#Tabs

Comment 1

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

How does the floating nature of the titlebar(urlbar) interact with the panning of the controlstrip and tab area? If the web content "pans" to make room for the controlstrip and tab area, does the titlebar too? Does the controlstrip and tab area also pan downward to make room for the titlebar(urlbar)?

How are these things constrained with respect to each other?

Comment 2

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

Attachment: WIP v1: Uses panning to slide the left/right side bars

This patch adds the code needed to slide left and right to make the side UI bars appear. There is code to limit the slide range so we don't pan too far left or right. There is also code to "snap" the UI to the default "no sidebars showing" mode - making it easier for the user to get back to the default view.

This patch does not include some code removal that can be done. A few of the PANELMODE_* modes can be removed and the "overpan" event in deckbrowser & browser-ui can be removed.

Comment 3

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

Attachment: WIP v2: More changes, a bit of cleanup and some code removal

This patch moves the last patch forward. The right side panel UI is now a <deck> to support multiple panels. Buttons exist to select a specific panel. The BrowserUI.show method has be cleaned up a bit.

The "+" button will toggle the panel UI in and out of view. Currently, there is _no_ animated slide of the panel UI. We can add that later, if performance allows.

The only thing I want to change is the Tab bar position relative to the Nav toolbar. Current designs show the Tab bar _under_ the Nav bar. This patch has the Tab bar to the left of the Navbar, and at the top of the window.

Once I add that change, this will be ready for review.

Comment 4

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

Attachment: Ready for review

This patch carries most of the previous patch. I changed the relationship of the tab list and the nav toolbar to match current design. I also added support for floating the nav toolbar into view if either sidebar is visible.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 337537 [details] [diff] [review]
Ready for review

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>   _scrollToolbar : function(aEvent) {
>+    var [scrollWidth, scrollHeight] = Browser.content._contentAreaDimensions;
>+    var [canvasW, canvasH] = Browser.content._effectiveCanvasDimensions;

You could omit the height variables since they aren't used.

I think this function really wants to be putting frequently used height/width variables into local variables instead of calling foo.boxObject.(width|height) all the time - the boxObject getter itself is non-trivial, especially since this is being called many times while panning.

>+      // Window size events
>       case "resize":

Don't you need to reset _layoutControls here?

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>   doCommand : function(cmd) {

>-      case "cmd_downloads":
>-        Cc["@mozilla.org/download-manager-ui;1"].getService(Ci.nsIDownloadManagerUI).show(window);

Why are you removing this?

There was some weirdness while panning with this patch that we talked about, probably need to figure that out before landing this. Otherwise this looks good.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP, alternative aproach

Comment 7

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

(In reply to comment #5)
> (From update of attachment 337537 [details] [diff] [review])
> >diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
> 
> >   _scrollToolbar : function(aEvent) {
> >+    var [scrollWidth, scrollHeight] = Browser.content._contentAreaDimensions;
> >+    var [canvasW, canvasH] = Browser.content._effectiveCanvasDimensions;
> 
> You could omit the height variables since they aren't used.
> 

Done

> I think this function really wants to be putting frequently used height/width
> variables into local variables instead of calling foo.boxObject.(width|height)
> all the time - the boxObject getter itself is non-trivial, especially since
> this is being called many times while panning.


Done

> >+      // Window size events
> >       case "resize":
> 
> Don't you need to reset _layoutControls here?

No, we only want to use _layoutControls for the intial window load. After that, the panning code will position the controls for us.

> 
> >diff --git a/chrome/content/browser.js b/chrome/content/browser.js
> 
> >   doCommand : function(cmd) {
> 
> >-      case "cmd_downloads":
> >-        Cc["@mozilla.org/download-manager-ui;1"].getService(Ci.nsIDownloadManagerUI).show(window);
> 
> Why are you removing this?

We add the download manager to the new UI panel

> 
> There was some weirdness while panning with this patch that we talked about,
> probably need to figure that out before landing this. Otherwise this looks
> good.

Should be working better

Comment 8

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

Attachment: Changes per review comments and tweaked panning to work a bit better

This patch performs a bit better.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> > >-      case "cmd_downloads":
> > >-        Cc["@mozilla.org/download-manager-ui;1"].getService(Ci.nsIDownloadManagerUI).show(window);
> > 
> > Why are you removing this?
> 
> We add the download manager to the new UI panel

You should remove the <command> and associated <key> elements, then (and remove it from supportsCommand as well).

Comment 10

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

Attachment: Unbitrotted patch

Comment 11

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

http://hg.mozilla.org/mobile-browser/rev/6a1ec6f70ab2

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

verified in beta3