454902 - Titlebar should fade in when side-UI is pulled into view

Status: VERIFIED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

When the control strip or the tab strip are pulled into the viewport, the titlebar also appears, "hovering" over the content.  At the moment, it just appears suddenly, which is a little jarring.

A fade in would (a) be less jarring and (b) would let us communicate that it's arriving because on of the sidebars is being pulled into view -- i.e., as you pull more of the sidebar into view, before you break contact with the screen, the titlebar gets more opaque.

In the interim, if real fading in is hard, we could at least have a two-stage fade, where, when a side bar is pulled in, the titlebar appears but not a full opacity.  It could disappear again if the user doesn't pull the sidebar in completely so that it snaps back out of sight.

Comment 1

[Daniel Brooks [:db48x]]

Attachment: 454902-1.diff

After playing with it a bit, I decided to slide it in rather than fade it in; it just felt better.

Comment 2

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

(In reply to comment #1)

> After playing with it a bit, I decided to slide it in rather than fade it in;
> it just felt better.

What's this feel like on the device? One reason we went with fade in was the horrible performance of slide animations.

Comment 3

[Daniel Brooks [:db48x]]

It's no slower than the existing animations.

Comment 4

[Stuart Parmenter]

this can be made to fade, but i'm not sure what you're trying to achieve.

if you fade in the urlbar when the solid sidebar comes on, you're going to end up with a weird looking hard line at the top of the sidebar and a partially visible urlbar... not sure that will look very good.

Comment 5

[Daniel Brooks [:db48x]]

Attachment: 454902-2.diff

Comment 6

[Stuart Parmenter]

Comment on attachment 343141 [details] [diff] [review]
454902-1.diff

this patch isn't right with the widgetstack code

Comment 7

[Stuart Parmenter]

Comment on attachment 350196 [details] [diff] [review]
454902-2.diff

the variable renames aren't necessary.  please get rid of those and get vlad to review the WidgetStack changes

Comment 8

[Daniel Brooks [:db48x]]

Attachment: 454902-3.diff

Comment 9

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

Comment on attachment 350199 [details] [diff] [review]
454902-3.diff

>diff -r 7b2454dd24b2 -r 1ff833d57771 chrome/content/WidgetStack.js
> 
>+      if (state.dependent)
>+      {
>+        var s = Components.utils.Sandbox(window);
>+        s.widgets = this._widgetState;
>+        s.self = state;
>+        s.viewrect = vr;
>+        s.dx = dx;
>+        s.dy = dy;
>+
>+        Components.utils.evalInSandbox(state.depExpression, s);
>+      }
>+

Couldn't we convert this expression to a Function object right away? Kinda like this:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/tabs.xml#23

Then we wouldn't have the overhead of creating a sandbox and evalInSandbox.

>       vpRelative: false,
>+      dependent: false,
>+      depExpression: "",

Just store the Function reference, not the expression as a string

>+        else if (s == "dependent") {
>+          state.dependent = true;
>+          state.depExpression = w.getAttribute("depexpression");

Convert to Function here

>
>   _commitState: function (state) {
>     // if the widget is frozen, don't actually update its left/top;
>     // presumably the caller is managing those directly for now.
>-    if (state.frozen)
>-      return;
>+    //if (state.frozen)
>+    //  return;

You should do some explaining as to why the comment and the code are not in sync anymore.


Personally, I'm not a big fan of the way this patch pushes JS into the XUL via depexpression. I'd rather see some unobtrusive JS techniques used instead.

Comment 10

[Daniel Brooks [:db48x]]

The js in the xul can be greatly simplified, to "y = max(-h, min(0, -h + abs(viewrect.x)))", with a few changes to the environment it executes in. Of course, this means that it can't be made into a function call with new Function…

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 350199 [details] [diff] [review]
454902-3.diff

So this is kind of the wrong approach to this.  We don't need any modifications to WidgetStack at all for this -- there's already a drag notification function that figures out whether a sidebar is visible or not.  If it's transitioning from closed to open, then just freeze the urlbar and move it into place like it is now -- if you need to animate that, set up a timer to deal with changing opacity/slide-in position/whatever.

There really shouldn't be any other changes needed other than just modifying that one function and adding a timer (with appropriate cancelling if the sidebar panel is closed etc).  Or if you want the urlbar to slide in in sync with the sidebar sliding in, that math can be done in that function as well.

Comment 12

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

Madhava, do we still want this?

Comment 13

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

(In reply to comment #12)
> Madhava, do we still want this?

I can do a crappy imitation of Madhava. But we can't do the fade. We haven't been hurting for it either.

We can reopen if this becomes feasible.

Comment 14

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

verified as wontfix.