868998 - Reveal title bar by scrolling a certain distance.

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Ian Barlow (:ibarlow)]

We discussed this on IRC last week, just making it official as a bug :)

Currently in Nightly, we reveal the title bar when you pull down from the top portion of the screen. This is great for keeping you in full screen mode, but can also feel a bit clumsy when trying to get the title bar back if you either don't know or have forgotten about having to pull down from the top. 

We'd like to compare this interaction with something distance based. In other words, reveal the header after the user has scrolled down, say 40dp or so, from anywhere on screen.

Comment 1

[Wesley Johnston (:wesj)]

Have we talked about playing with speed as well? It seems like if you fling towards the top quickly, you're likely trying to reach the top of the page (and urlbar), while if you pan slowly you're likely already at the top or just reading?

All of this gives me a vague feeling of deja vu.

Comment 2

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #1)
> Have we talked about playing with speed as well? It seems like if you fling
> towards the top quickly, you're likely trying to reach the top of the page
> (and urlbar), while if you pan slowly you're likely already at the top or
> just reading?
> 
> All of this gives me a vague feeling of deja vu.

Yup, but we never tried any of it. https://bugzilla.mozilla.org/show_bug.cgi?id=716403#c6. I believe we were just trying to get the title bar to scroll off reliably first.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Personally, I believe tapping near the area where the awesomebar would appear should also make it appear. It's a fairly natural interaction.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #4)
> Personally, I believe tapping near the area where the awesomebar would
> appear should also make it appear.

That would get in the way of actually tapping links that happen to be at that position.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

>That would get in the way of actually tapping links that happen to be at that 
>position.

Scrolling can put away the part of the page that you want to read. 

If the user taps and the awesomebar appears, the solution to hit the link is natural: scroll down. The way to get the awesombar to appear right now sin't natural at all.

Are we able to detect when the tap didn't "hit" anything on the webpage?

Comment 7

[Mike Hommey [:glandium]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #6)
> If the user taps and the awesomebar appears, the solution to hit the link is
> natural: scroll down. 

The solution is natural, but the interaction is annoying.

Comment 8

[Chris Lord [:cwiiis]]

Attachment: Expose the toolbar based on distance travelled

This replaces the current behaviour of needing to scroll from the top part of the page with just needing to scroll past a certain distance (20% of the screen in this patch, but configurable via browser.ui.show-margins-threshold)

Requesting feedback from ibarlow to see if that value is ok and if we want this behaviour or not.

Comment 9

[Ian Barlow (:ibarlow)]

This feels like the better interaction to me. Accessing the title bar is easy enough from anywhere, but there is enough slack in the interaction to allow for correcting your scroll position a little, if you went a little too far down the page.

One thing I would like to see tweaked here is the way in which the header is revealed. Right now there is a threshold to pull down, and then once you hit that you have to keep pulling down to reveal more title bar. Instead of this, why don't we try animating the title bar in (~250ms animation) as soon as you hit that threshold. I think it would make the UI feel a little snappier.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 755406 [details] [diff] [review]
Expose the toolbar based on distance travelled

Review of attachment 755406 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: mobile/android/base/gfx/LayerMarginsAnimator.java
@@ +78,5 @@
> +    @Override
> +    protected void finalize() throws Throwable {
> +        if (mPrefObserverId != null) {
> +            PrefsHelper.removeObserver(mPrefObserverId);
> +            mPrefObserverId = null;

This will not work. finalize() only gets called when the object is marked for GC, which will never happen as long as PrefsHelper holds a ref to it. I'd rather put this in an explicit destroy() function that you can call from GeckoLayerClient.destroy().

@@ +211,5 @@
>              aMargins[1] = marginEnd + Math.min(marginDelta, aMaxMarginEnd - marginEnd);
>          } else {
>              float marginDelta = Math.max(0, -aDelta - aOverscrollEnd);
>              aMargins[1] = marginEnd - Math.min(marginDelta, marginEnd);
> +            if (-aTouchTravelDistance < exposeThreshold && marginStart == 0) {

I'd be more happy if you used a Math.abs() on aTouchTravelDistance. You could even pull out this check to the top of this function like so:

boolean exposeThresholdExceeded = Math.abs(aTouchTravelDistance) > (viewportSize * SHOW_MARGINS_THRESHOLD);

@@ +286,5 @@
>      public boolean onInterceptTouchEvent(View view, MotionEvent event) {
> +        if (event.getPointerCount() == 1) {
> +            int action = event.getActionMasked();
> +            switch (action) {
> +            case MotionEvent.ACTION_DOWN:

No need for a switch statement here; you can keep the old if (... && ...) condition.

Comment 11

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> Comment on attachment 755406 [details] [diff] [review]
> Expose the toolbar based on distance travelled
> 
> Review of attachment 755406 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed.
> 
> ::: mobile/android/base/gfx/LayerMarginsAnimator.java
> @@ +78,5 @@
> > +    @Override
> > +    protected void finalize() throws Throwable {
> > +        if (mPrefObserverId != null) {
> > +            PrefsHelper.removeObserver(mPrefObserverId);
> > +            mPrefObserverId = null;
> 
> This will not work. finalize() only gets called when the object is marked
> for GC, which will never happen as long as PrefsHelper holds a ref to it.
> I'd rather put this in an explicit destroy() function that you can call from
> GeckoLayerClient.destroy().

Will fix, thanks.

> @@ +211,5 @@
> >              aMargins[1] = marginEnd + Math.min(marginDelta, aMaxMarginEnd - marginEnd);
> >          } else {
> >              float marginDelta = Math.max(0, -aDelta - aOverscrollEnd);
> >              aMargins[1] = marginEnd - Math.min(marginDelta, marginEnd);
> > +            if (-aTouchTravelDistance < exposeThreshold && marginStart == 0) {
> 
> I'd be more happy if you used a Math.abs() on aTouchTravelDistance. You
> could even pull out this check to the top of this function like so:

But we don't want the abs here, we're checking that it's travelled that far in that specific direction - I'd rather not change this - abs would work because we reset the distance when changing direction, but in case we decide not to do this at some point this could lead to odd behaviour.

> boolean exposeThresholdExceeded = Math.abs(aTouchTravelDistance) >
> (viewportSize * SHOW_MARGINS_THRESHOLD);
> 
> @@ +286,5 @@
> >      public boolean onInterceptTouchEvent(View view, MotionEvent event) {
> > +        if (event.getPointerCount() == 1) {
> > +            int action = event.getActionMasked();
> > +            switch (action) {
> > +            case MotionEvent.ACTION_DOWN:
> 
> No need for a switch statement here; you can keep the old if (... && ...)
> condition.

Will do, hang-over from earlier version of the patch.

Comment 12

[Chris Lord [:cwiiis]]

Changes made, pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fe5735bca9e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4fe5735bca9e

Comment 14

[u421692]

Verified fixed on:
Build: Firefox for Android 24.0a1 (2013-06-09)
Device: Samsung Galaxy R
OS: Android 2.3.4

Comment 15

[Chris Lord [:cwiiis]]

Comment on attachment 755406 [details] [diff] [review]
Expose the toolbar based on distance travelled

ack, so I guess this must've missed merge because it isn't in 23...

[Approval Request Comment]
Bug caused by (feature/regressing bug #): The toolbar can only be revealed by scrolling to the top or scrolling from the top third of the screen
User impact if declined: Users not knowing how to reveal the toolbar (e.g. bug 888659)
Testing completed (on m-c, etc.): Been on m-c and aurora for a long tie with no complaint, was intended to be in 23.
Risk to taking this patch (and alternatives if risky): Given the amount of testing this has now had, I'd say reasonably low risk.
String or IDL/UUID changes made by this patch: None.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 755406 [details] [diff] [review]
Expose the toolbar based on distance travelled

we'll take the backout and track bug 809055 hoping for a potential fix that doesn't cause crashes like this.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to [e-mail] [:lsblakk] from comment #16)
> Comment on attachment 755406 [details] [diff] [review]
> Expose the toolbar based on distance travelled
> 
> we'll take the backout and track bug 809055 hoping for a potential fix that
> doesn't cause crashes like this.

Sorry that was for bug 827407, for this bug we'll approve since this low risk.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e0540178e310

Comment 20

[u421692]

Verified fixed on:
Build: Firefox for Android 23.0b4(2013-07-09)
Device: LG Nexus 4
OS: Android 4.2.2