Enable dynamic (hide on scroll) toolbar by default

RESOLVED FIXED in Firefox 22

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

Trunk
Firefox 22
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Bug 716403 has the feature, but disabled as it has a few issues and many tests fail. Let's track those issues here.
Created attachment 720009 [details]
Inbound (03/01) - Screenshot

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

Comment 2

5 years ago
(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?
(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.
Created attachment 720038 [details]
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
Created attachment 720039 [details]
Inbound (03/01) - Screenshot (Issue #2.5)

Pref disabled, landscape, opening and closing tab try.

What is this, I don't even.
Depends on: 849246
Created attachment 722827 [details]
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.
(Assignee)

Comment 7

5 years ago
Created attachment 723429 [details] [diff] [review]
Add prefs listening capability to PrefsHelper/browser.js

This will be needed if we want to dynamically toggle the header on/off.
Attachment #723429 - Flags: review?(bugmail.mozilla)
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.
Attachment #723429 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 9

5 years ago
Created attachment 723482 [details] [diff] [review]
Add prefs listening capability to PrefsHelper/browser.js (v2)

Review comments addressed.
Attachment #723429 - Attachment is obsolete: true
Attachment #723482 - Flags: review?(bugmail.mozilla)
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.
Attachment #723482 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 723892 [details] [diff] [review]
Enable dynamic toolbar pref

Just a patch to flip the pref - obviously won't push until test failures and dependent bugs are sorted out.
Attachment #723892 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 12

5 years ago
Created attachment 723893 [details] [diff] [review]
Observe dynamic toolbar pref

Observe the dynamic toolbar pref instead of just listening to it. This will be handy for robocop tests.
Attachment #723893 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 13

5 years ago
Created attachment 723894 [details] [diff] [review]
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.
Attachment #723894 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 14

5 years ago
Created attachment 723895 [details] [diff] [review]
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.
Attachment #723895 - Flags: review?(matspal)
Attachment #723895 - Flags: review?(jwatt)
Attachment #723893 - Flags: review?(bugmail.mozilla) → review+
Attachment #723894 - Flags: review?(jmaher)
Attachment #723894 - Flags: review?(bugmail.mozilla)
Attachment #723894 - Flags: review+
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.
Attachment #723892 - Flags: review?(bugmail.mozilla) → review+
Attachment #723895 - Flags: review?(jwatt) → review+

Updated

5 years ago
Attachment #723895 - Flags: review?(matspal) → review+
(Assignee)

Updated

5 years ago
Depends on: 849958
Comment on attachment 723894 [details] [diff] [review]
Disable dynamic toolbar on robocop tests

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

this looks harmless.
Attachment #723894 - Flags: review?(jmaher) → review+
(Assignee)

Comment 17

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

Comment 19

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

Comment 20

5 years ago
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
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Are we still using this bug for the issues reported in the screenshots?
(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

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

Comment 26

5 years ago
(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.
Depends on: 852163
Depends on: 854289

Updated

5 years ago
No longer depends on: 854289
You need to log in before you can comment on or make changes to this bug.