849958 - Hidden toolbar should only reappear on swipe directly down (not up-then-down or side-then-down scrolling)

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

The new hiding toolbar reappears too easily. Any downward scroll will reveal the toolbar.

 Compare with the stock browser's toolbar. It only reappears when the user presses down and scrolls directly down. The stock browser's toolbar does not reappear if the user is scrolling up and down or sideways and down without lifting their finger. The stock behavior makes scrolling and reading a long article much easier.

Comment 1

[Chris Lord [:cwiiis]]

Agreed, will implement.

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Add axis locking + scroll threshold to dynamic toolbar

Does as suggested.

The only possible difference is that if the toolbar starts visible on a swipe, you can hide and expose it multiple times - that feels expected to me, you shouldn't be able to hide, then not reveal the toolbar in a single swipe.

Comment 3

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

Comment on attachment 723967 [details] [diff] [review]
Add axis locking + scroll threshold to dynamic toolbar

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

::: mobile/android/base/BrowserApp.java
@@ +262,5 @@
> +                    if (deltaY > 0 && toolbarY == toolbarHeight) {
> +                        mToolbarLocked = true;
> +                        return super.onInterceptTouchEvent(view, event);
> +                    }
> +                } else if (Math.abs(eventX - mLastTouchX) > threshold) {

For more parallel-looking code, add a deltaX definition up next to deltaY and use that here. Also add a comment here (I think something like "Any horizontal scroll above the threshold amount causes the toolbar to lock"?)

Comment 4

[Chris Lord [:cwiiis]]

Addressed and pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/af753b784e28

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Something in this push (along with bug 849845) was causing robocop-1/2 to timeout. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c0d137f08e

Possibly of help - one of the runs shows this:
https://tbpl.mozilla.org/php/getParsedLog.php?id=20578472&tree=Mozilla-Inbound

INFO | automation.py | Application pid: 0
0 INFO SimpleTest START
1 INFO TEST-START | testHistoryTab
2 INFO TEST-PASS | testHistoryTab | Awesomebar URL typed properly - http://mochi.test:8888/tests/robocop/robocop_big_link.html should equal http://mochi.test:8888/tests/robocop/robocop_big_link.html
3 INFO TEST-PASS | testHistoryTab | Awesomebar URL typed properly - http://mochi.test:8888/tests/robocop/robocop_blank_01.html should equal http://mochi.test:8888/tests/robocop/robocop_blank_01.html
INFO | automation.py | Application ran for: 0:00:57.216669
INFO | automation.py | Reading PID log: /var/folders/jr/dj51k_ns4vq2yg4mcygb26gw0000gn/T/tmpRCOathpidlog
PROCESS-CRASH | java-exception | java.lang.NoSuchMethodError: android.view.KeyEvent.getSource	at org.mozilla.gecko.BrowserApp.onKey(BrowserApp.java:329)
WARNING | automationutils.processLeakLog() | refcount logging is off, so leaks can't be detected!

INFO | runtests.py | Running tests: end.

Note that the timeouts were always after testAllPagesTab.

Comment 6

[Chris Lord [:cwiiis]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> Something in this push (along with bug 849845) was causing robocop-1/2 to
> timeout. Backed out.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/14c0d137f08e
> 
> Possibly of help - one of the runs shows this:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=20578472&tree=Mozilla-
> Inbound

> PROCESS-CRASH | java-exception | java.lang.NoSuchMethodError:
> android.view.KeyEvent.getSource	at
> org.mozilla.gecko.BrowserApp.onKey(BrowserApp.java:329)

This exception would've been caused by bug 849845, I'll re-land this when the tree opens/when I wake up (whichever comes first :))

Comment 7

[Chris Lord [:cwiiis]]

Pushed again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4399862345e

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d4399862345e

Comment 9

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
-build: Firefox for Android 23.0a1 (2013-04-08)
-device: Acer Iconia A500  
-OS: Android 3.2.1