Closed Bug 846772 Opened 9 years ago Closed 9 years ago

Enable dynamic (hide on scroll) toolbar by default

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: cwiiis, Assigned: cwiiis)

References

Details

Attachments

(9 files, 1 obsolete file)

Bug 716403 has the feature, but disabled as it has a few issues and many tests fail. Let's track those issues here.
One issue; opening the tab tray has the right side either removed or transparent
(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.
Issue #2 - sometimes when flinging back up you see content in the area where chrome is supposed to be quickly flash on the screen
Pref disabled, landscape, opening and closing tab try.

What is this, I don't even.
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.
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-
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+
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)
Observe the dynamic toolbar pref instead of just listening to it. This will be handy for robocop tests.
Attachment #723893 - Flags: review?(bugmail.mozilla)
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)
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+
Attachment #723895 - Flags: review?(matspal) → review+
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+
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.
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!
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.
(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: 854289
No longer depends on: 854289
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.