Reveal title bar by scrolling a certain distance.

VERIFIED FIXED in Firefox 23

Status

()

VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: ibarlow, Assigned: cwiiis)

Tracking

23 Branch
Firefox 24
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox23 verified, firefox24 verified, fennec23+)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

6 years ago
Blocks: 716403

Updated

6 years ago
tracking-fennec: --- → ?
status-firefox23: --- → affected
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.
(Reporter)

Comment 2

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

Updated

6 years ago
Duplicate of this bug: 868607
Component: General → Theme and Visual Design
OS: Mac OS X → Android
Hardware: x86 → All
tracking-fennec: ? → 23+

Updated

6 years ago
Version: unspecified → Firefox 23
Personally, I believe tapping near the area where the awesomebar would appear should also make it appear. It's a fairly natural interaction.
(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.
>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?
(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.
(Assignee)

Comment 8

6 years ago
Created attachment 755406 [details] [diff] [review]
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.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Attachment #755406 - Flags: review?(bugmail.mozilla)
Attachment #755406 - Flags: feedback?(ibarlow)
(Reporter)

Comment 9

6 years ago
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 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.
Attachment #755406 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 11

6 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
https://hg.mozilla.org/mozilla-central/rev/4fe5735bca9e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24

Comment 14

5 years ago
Verified fixed on:
Build: Firefox for Android 24.0a1 (2013-06-09)
Device: Samsung Galaxy R
OS: Android 2.3.4
status-firefox24: --- → verified
(Reporter)

Updated

5 years ago
Depends on: 881240
(Assignee)

Comment 15

5 years ago
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.
Attachment #755406 - Flags: feedback?(ibarlow) → approval-mozilla-beta?
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.
Attachment #755406 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to lsblakk@mozilla.com [: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.

Updated

5 years ago
Duplicate of this bug: 888659

Comment 20

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

Updated

5 years ago
status-firefox23: fixed → verified

Updated

5 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Updated

5 years ago
Duplicate of this bug: 925741
You need to log in before you can comment on or make changes to this bug.