Closed Bug 868998 Opened 12 years ago Closed 12 years ago

Reveal title bar by scrolling a certain distance.

Categories

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

23 Branch
All
Android
defect
Not set
normal

Tracking

(firefox23 verified, firefox24 verified, fennec23+)

VERIFIED FIXED
Firefox 24
Tracking Status
firefox23 --- verified
firefox24 --- verified
fennec 23+ ---

People

(Reporter: ibarlow, Assigned: cwiiis)

References

Details

Attachments

(1 file)

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.
tracking-fennec: --- → ?
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.
(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.
Component: General → Theme and Visual Design
OS: Mac OS X → Android
Hardware: x86 → All
tracking-fennec: ? → 23+
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.
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)
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+
(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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Verified fixed on: Build: Firefox for Android 24.0a1 (2013-06-09) Device: Samsung Galaxy R OS: Android 2.3.4
Depends on: 881240
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.
Verified fixed on: Build: Firefox for Android 23.0b4(2013-07-09) Device: LG Nexus 4 OS: Android 4.2.2
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: