846772 - Enable dynamic (hide on scroll) toolbar by default

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Lord [:cwiiis]]

Bug 716403 has the feature, but disabled as it has a few issues and many tests fail. Let's track those issues here.

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Inbound (03/01) - Screenshot

One issue; opening the tab tray has the right side either removed or transparent

Comment 2

[Chris Lord [:cwiiis]]

(In reply to Aaron Train [:aaronmt] from comment #1)
> Created attachment 720009 [details]
> Inbound (03/01) - Screenshot
> 
> One issue; opening the tab tray has the right side either removed or
> transparent

is this just with it enabled, or with it disabled too?

Comment 3

[Aaron Train [:aaronmt]]

(In reply to Chris Lord [:cwiiis] from comment #2)
> (In reply to Aaron Train [:aaronmt] from comment #1)
> > Created attachment 720009 [details]
> > Inbound (03/01) - Screenshot
> > 
> > One issue; opening the tab tray has the right side either removed or
> > transparent
> 
> is this just with it enabled, or with it disabled too?

When it is enabled.

Comment 4

[Aaron Train [:aaronmt]]

Attachment: Inbound (03/01) - Screenshot (Issue #2)

Issue #2 - sometimes when flinging back up you see content in the area where chrome is supposed to be quickly flash on the screen

Comment 5

[Aaron Train [:aaronmt]]

Attachment: Inbound (03/01) - Screenshot (Issue #2.5)

Pref disabled, landscape, opening and closing tab try.

What is this, I don't even.

Comment 6

[Peter Retzer (:pretzer)]

Attachment: Nightly (03/08) - Screenshot (Issue #3)

I found another issue with the pref enabled. There is a small gap between the tabs tray button and the rest of the toolbar. It looks like the toolbar shadow that is usually there is missing.

I'm trying this on a Galaxy S2, so this issue might be specific to phones with a HW menu button.

Let me know if a dedicated bug suites you better, then I'll file one.

Comment 7

[Chris Lord [:cwiiis]]

Attachment: Add prefs listening capability to PrefsHelper/browser.js

This will be needed if we want to dynamically toggle the header on/off.

Comment 8

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

Comment on attachment 723429 [details] [diff] [review]
Add prefs listening capability to PrefsHelper/browser.js

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

Looks good but r- for missing testPrefsObserver file. Some minor nits below but nothing that would block an r+ otherwise.

::: build/mobile/robocop/Actions.java.in
@@ +24,5 @@
>          public String blockForEventData();
>  
> +        /**
> +         * Blocks until the event has been received, or until the timeout has been exceeded and
> +         * returns the data associated with the event if applicable.

s/exceeded and returns/exceeded. Returns/

The current phrasing is hard to parse with "or" and "and".

::: build/mobile/robocop/FennecNativeActions.java.in
@@ +117,5 @@
>          public synchronized void blockForEvent() {
> +            blockForEvent(MAX_WAIT_MS, true);
> +        }
> +
> +        protected synchronized void blockForEvent(long millis, boolean failOnTimeout) {

Make this private

::: mobile/android/base/PrefsHelper.java
@@ +83,5 @@
>                  try {
>                      PrefHandler callback;
>                      synchronized (PrefsHelper.class) {
>                          try {
> +                            int requestId = Integer.parseInt(message.getString("requestId"));

Why did you change to Integer.parseInt(message.getString(..)) from message.getInt()? Does the getInt() fail in some cases?

::: mobile/android/base/tests/robocop.ini
@@ +32,5 @@
>  [testSystemPages]
>  # [testPermissions] # see bug 757475
>  # [testJarReader] # see bug 738890
>  [testDistribution]
> +[testPrefsObserver]

I think you forgot to add this file to the patch

::: mobile/android/chrome/content/browser.js
@@ +983,5 @@
> +      let i = 0;
> +      while (i < requestIds.length) {
> +        if (requestIds[i] != aRequestId) {
> +          i++;
> +          continue;

Can you use requestIds.indexOf(aRequestId) instead here?

@@ +992,5 @@
> +      // If there are no more request IDs, remove the observer
> +      if (requestIds.length == 0) {
> +        Services.prefs.removeObserver(prefName, this);
> +      } else {
> +        newPrefObservers[prefName] = requestIds;

You could also use 'delete this._prefObservers[prefName]' instead of having newPrefObservers, I think. This is fine though, your call.

Comment 9

[Chris Lord [:cwiiis]]

Attachment: Add prefs listening capability to PrefsHelper/browser.js (v2)

Review comments addressed.

Comment 10

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

Comment on attachment 723482 [details] [diff] [review]
Add prefs listening capability to PrefsHelper/browser.js (v2)

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

r=me with the comment fixed

::: mobile/android/base/tests/testPrefsObserver.java.in
@@ +11,5 @@
> +/**
> + * Basic test to check bounce-back from overscroll.
> + * - Load the page and verify it draws
> + * - Drag page downwards by 100 pixels into overscroll, verify it snaps back.
> + * - Drag page rightwards by 100 pixels into overscroll, verify it snaps back.

Comment needs deletion and/or updating.

Comment 11

[Chris Lord [:cwiiis]]

Attachment: Enable dynamic toolbar pref

Just a patch to flip the pref - obviously won't push until test failures and dependent bugs are sorted out.

Comment 12

[Chris Lord [:cwiiis]]

Attachment: Observe dynamic toolbar pref

Observe the dynamic toolbar pref instead of just listening to it. This will be handy for robocop tests.

Comment 13

[Chris Lord [:cwiiis]]

Attachment: Disable dynamic toolbar on robocop tests

Current robocop tests are just not written with a non-static UI in mind. Disable it by default and we can enable it for specific tests.

Comment 14

[Chris Lord [:cwiiis]]

Attachment: Fuzz the two failing reftests

Fuzz svg-clipPath-01 and scroll-rounding. These tests both fail due to slightly different subpixel alignment.

A real failure on the first would manifest in red squares, and a real failure on the second would result in incorrect ellipsis - neither of these happen. Also, the second is already heavily fuzzed on Android, so I'm just increasing it slightly.

Comment 15

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

Comment on attachment 723892 [details] [diff] [review]
Enable dynamic toolbar pref

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

This is fine, just make sure it's the last patch in the queue you land, since it depends on the other patches.

Comment 16

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

Comment on attachment 723894 [details] [diff] [review]
Disable dynamic toolbar on robocop tests

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

this looks harmless.

Comment 17

[Chris Lord [:cwiiis]]

Pushed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f6c3698fb492
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/22569c8ec88a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1a4957bd3d85
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0661df360cc2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9cd9a1e658ff

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Not enough fuzzing apparently. Backed out for Android reftest-3 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/68d039b40df9

https://tbpl.mozilla.org/php/getParsedLog.php?id=20583615&tree=Mozilla-Inbound

Comment 19

[Chris Lord [:cwiiis]]

This failure was actually caused by an unexpected pass - jwatt r+'d the removal of the fails-if - it now passes as the browser element is slightly bigger. Will re-push when the tree is open.

Comment 20

[Chris Lord [:cwiiis]]

jwatt r+'d the change, pushed again:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/88c999e8aee1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb6e67fae0b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/73f641ace00e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b3db97a3cbb7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e36e2d2b62ec

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/88c999e8aee1
https://hg.mozilla.org/mozilla-central/rev/3bb6e67fae0b
https://hg.mozilla.org/mozilla-central/rev/73f641ace00e
https://hg.mozilla.org/mozilla-central/rev/b3db97a3cbb7
https://hg.mozilla.org/mozilla-central/rev/e36e2d2b62ec

Comment 22

[Aaron Train [:aaronmt]]

Are we still using this bug for the issues reported in the screenshots?

Comment 23

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

(In reply to Aaron Train [:aaronmt] from comment #22)
> Are we still using this bug for the issues reported in the screenshots?

New bugs!

Comment 24

[Paul [pwd]]

Why has such a broken feature been enabled by default? The fact that all touch points are off should be enough to back this out. That's not a matter of needing more testing, it's a matter of no testing before implementation. I really can't understand why there's been an assumption that if a user scrolls on a page, then they obviously don't want to change tabs or use the overwhelming majority of the app. There should be a visible pref for this behaviour at the very least.

Comment 25

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

(In reply to Paul [sabret00the] from comment #24)
> Why has such a broken feature been enabled by default? The fact that all
> touch points are off should be enough to back this out. That's not a matter
> of needing more testing, it's a matter of no testing before implementation.
> I really can't understand why there's been an assumption that if a user
> scrolls on a page, then they obviously don't want to change tabs or use the
> overwhelming majority of the app. There should be a visible pref for this
> behaviour at the very least.

You're using Nightly. We enabled it to get more testing. If regressions are not fixed soon enough, we might disable it again. Please disable locally with browser.chrome.dynamictoolbar=false, or try moving to Aurora while we get this feature working better on Nightly.

Comment 26

[Chris Lord [:cwiiis]]

(In reply to Paul [sabret00the] from comment #24)
> Why has such a broken feature been enabled by default? The fact that all
> touch points are off should be enough to back this out. That's not a matter
> of needing more testing, it's a matter of no testing before implementation.
> I really can't understand why there's been an assumption that if a user
> scrolls on a page, then they obviously don't want to change tabs or use the
> overwhelming majority of the app. There should be a visible pref for this
> behaviour at the very least.

I apologise for your poor experience - unfortunately something changed between my local tree and the tree I committed to that has caused this. Such an obvious bug would have certainly been fixed if it had come up.

Due to the way we develop, we often have to 'rebase' patches before pushing them, and sometimes things have changed between patch development and patch committing in unforseen ways that can cause errors like this. As this was part of a large series of patches, the burden of having to locally rebase frequently got to the point where my vigilance declined.

As a temporary work-around, you can either disable the feature as Mark suggests, or entering the settings menu and going back will fix the issue for the lifetime of your session.

I'm looking into this issue right now. You can follow along in bug 850690.

I would suggest that if issues like this bother you, you may also want to use Aurora - Despite appearances, Nightly is not intended for daily use and may be unstable for considerable lengths of time.