Off-screen UI should slide into view when dragged

VERIFIED FIXED in fennec1.0a1

Status

Fennec Graveyard
General
P2
normal
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: madhava, Assigned: mfinkle)

Tracking

Trunk
fennec1.0a1
Bug Flags:
wanted-fennec1.0 +

Details

(Whiteboard: UI polish)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

10 years ago
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
(Reporter)

Updated

10 years ago
Whiteboard: UI polish

Updated

10 years ago
Assignee: nobody → mark.finkle
Flags: wanted-fennec1.0+
Priority: -- → P2
(Assignee)

Comment 1

10 years ago
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?
(Assignee)

Comment 2

10 years ago
Created attachment 336848 [details] [diff] [review]
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.
(Assignee)

Comment 3

10 years ago
Created attachment 337223 [details] [diff] [review]
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.
Attachment #336848 - Attachment is obsolete: true

Updated

10 years ago
Blocks: 436077
(Assignee)

Comment 4

10 years ago
Created attachment 337537 [details] [diff] [review]
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.
Attachment #337223 - Attachment is obsolete: true
Attachment #337537 - Flags: review?(gavin.sharp)
Attachment #337537 - Flags: review?(gavin.sharp)
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.
(Assignee)

Comment 7

10 years ago
(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
(Assignee)

Comment 8

10 years ago
Created attachment 337904 [details] [diff] [review]
Changes per review comments and tweaked panning to work a bit better

This patch performs a bit better.
Attachment #337537 - Attachment is obsolete: true
Attachment #337878 - Attachment is obsolete: true
Attachment #337904 - Flags: review?(gavin.sharp)
(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).
Created attachment 337969 [details] [diff] [review]
Unbitrotted patch
Attachment #337904 - Attachment is obsolete: true
Attachment #337969 - Flags: review?(gavin.sharp)
Attachment #337904 - Flags: review?(gavin.sharp)
Attachment #337969 - Flags: review?(gavin.sharp) → review+
http://hg.mozilla.org/mobile-browser/rev/6a1ec6f70ab2
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
verified in beta3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.