982181 - Tapping on History panel buttons interact with banner (Android 2.3)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Edwin Wong [:edwong]]

1. setup fxa+sync
2. open new tab 
3. swipe to history view
4. tap on either the clock or 'tabs' buttons at bottom

actual: i'm taken to 'account verified' hosted page in settings

Comment 1

[Aaron Train [:aaronmt]]

I imagine this will also be fixed on Aurora (03/12), tomorrow's build, via bug 975469.

Setting a reminder for you to check on tomorrow's build, or via testing today's Nightly build.

Comment 2

[Edwin Wong [:edwong]]

android 2.3 only - droid x.  No repro on S4 with android 4.2

Comment 3

[Aaron Train [:aaronmt]]

I don't know what the means in the context of comment #1, does that mean you tested Nightly, or that this is still reproducible on Nightly with only that device or what. #confused

Comment 4

[Edwin Wong [:edwong]]

:aaronmt just adding more info. This is repro'd on Nightly on Android 2.3 droid x.  My only other device tested is android 4.2 where I can't repro this issue.

Comment 5

[Aaron Train [:aaronmt]]

This has to do with the Sync promo banner. Margaret, are you able to reproduce? I see this on my Nexus S (2.3).

Comment 6

[:Margaret Leibovic]

Attachment: Disable the home banner on pre-honeycomb devices

At this point, I think we should just disable the banner on older devices. I'm not totally happy with the patch here, since we'll still do things like fetch snippets in the background, which are totally useless if there's no banner, but disabling those involves flipping a gecko pref.

I feel like ideally we should come up with a solution to have this banner work properly on gingerbread, but a fix there would involve changing how we set visibility, and given the history of this banner, that's too risky to land at this point.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8389480 [details] [diff] [review]
Disable the home banner on pre-honeycomb devices

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

So, that setVisibility() hack doesn't work after all?

::: mobile/android/base/BrowserApp.java
@@ +1696,5 @@
>              final HomeBanner homeBanner = (HomeBanner) findViewById(R.id.home_banner);
> +
> +            // Disable the home banner on pre-honeycomb devices.
> +            if (Build.VERSION.SDK_INT < 11) {
> +                mHomePagerContainer.removeView(homeBanner);

I wouldn't even bother removing the view.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8389480 [details] [diff] [review]
Disable the home banner on pre-honeycomb devices

Clearing review for extra clarification.

Comment 9

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #7)
> Comment on attachment 8389480 [details] [diff] [review]
> Disable the home banner on pre-honeycomb devices
> 
> Review of attachment 8389480 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, that setVisibility() hack doesn't work after all?

The problem is that when we animate down, we don't actually call setVisibility. The other bug we fixed for gingerbread did fix the fact that setVisibility(GONE) wasn't working, but it didn't handle this clicks-intercepted-when-translated issue.

I feel like a proper solution here would involve actually calling setVisibility(GONE) when the banner is translated off screen, but I'm nervous about introducing more visibility logic if this is something we want to uplift to aurora before the merge.

> ::: mobile/android/base/BrowserApp.java
> @@ +1696,5 @@
> >              final HomeBanner homeBanner = (HomeBanner) findViewById(R.id.home_banner);
> > +
> > +            // Disable the home banner on pre-honeycomb devices.
> > +            if (Build.VERSION.SDK_INT < 11) {
> > +                mHomePagerContainer.removeView(homeBanner);
> 
> I wouldn't even bother removing the view.

I figured we could save a bit of memory. This actually reminded me of bug 968640, we should fix that.

Comment 10

[:Margaret Leibovic]

Attachment: (v2) Disable the home banner on pre-honeycomb devices

This doesn't even bother removing the view. Once we fix bug 968640, we can avoid inflating the banner view to begin with if it's not enabled.

Comment 11

[:Margaret Leibovic]

Attachment: Hide home baner on animation end

Here's a patch that actually fixes the bug, but it's more invasive :(

Unfortunately, we're currently using the visibility to track whether or not we should be animating the banner up, so this patch has to change around the visibility logic to avoid relying on that. Instead, this patch checks whether or not there's text in the banner to decide whether or not to show it.

I was debating adding another boolean flag to keep track of this, but decided I don't like boolean flags. I also considered making clearMessage() and hasMessage() helper methods, let me know if you think that would be better.

I'm going to continue testing this on different devices and in different situations.

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 8390622 [details] [diff] [review]
Hide home baner on animation end

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

Using the TextView contents as a way to know whether the banner is enabled or not is a bit too hacky :-) Here's a simpler/cleaner alternative: use the banner view's enabled state to track this. In other words, use View.setEnabled() and View.isEnabled() instead.

::: mobile/android/base/home/HomeBanner.java
@@ +242,5 @@
> +
> +            @Override
> +            public void onPropertyAnimationEnd() {
> +                // Hide the banner to avoid intercepting clicks on pre-honeycomb devices.
> +                setVisibility(View.GONE);

Just wondering: does animateUp() work fine after this change? setVisibility() will call clearAnimation() (pre-HC devices) on the banner which effectively resets the translationY to 0 (see AnimatorProxy for reference). This change will break animateUp() if it assumes the initial transition is properly set.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8390568 [details] [diff] [review]
(v2) Disable the home banner on pre-honeycomb devices

Clearing review, I think we should go with the other approach.

Comment 14

[:Margaret Leibovic]

Attachment: Make home banner animation pre-HC friendly

You were right that this change broke animateUp. I also found that it broke some assumptions we made in handleHomeTouch, and I found another bug that when the banner is translated off screen in handleHomeTouch, we need to be sure to set the visibility to GONE, otherwise clicks will be intercepted on the default panel as well.

To summarize the changes here:

* setEnabled/isEnabled used to track whether or not the banner has content to show.

* The banner height is now cached, so that we don't use the wrong height if the banner is hidden.

* The banner visibility is set to GONE whenever it is animated off screen, this is handled in handleHomeTouch now by making sure we go through animateDown.

* Before the banner is set to VISIBLE, it is always translated off screen, so that animating up will work as expected (both in animateUp and handleHomeTouch).

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 8392591 [details] [diff] [review]
Make home banner animation pre-HC friendly

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

Looking pretty good. Not a fan of the hardcoded height but it's sane workaround for the layout issues (I assume you're getting height = 0 just after showing the banner?). Just want to see an updated patch before giving r+.

::: mobile/android/base/home/HomeBanner.java
@@ +174,1 @@
>                  if (TextUtils.isEmpty(id) || TextUtils.isEmpty(text)) {

Not in this patch but, this check should be done before posting to the UI thread to avoid unnecessary overhead.

@@ +244,5 @@
>      }
>  
>      private void animateDown() {
>          // Don't try to animate if the banner is already translated.
>          if (ViewHelper.getTranslationY(this) == getHeight()) {

Use mHeight for consistency? Audit the rest of the file.

@@ +277,5 @@
> +        if (getVisibility() == View.GONE) {
> +            // Translate the banner off screen before setting it to VISIBLE.
> +            ViewHelper.setTranslationY(this, mHeight);
> +            setVisibility(View.VISIBLE);
> +        }

Factor out this code into a private method to avoid unnecessary code duplication.

@@ +317,5 @@
> +                if (mSnapBannerToTop) {
> +                    animateUp();
> +                } else {
> +                    animateDown();
> +                    mUserSwipedDown = true;

Don't you have to set the visibility to GONE here too? I mean: what happens if you swipe down while the banner is active then swipe to another panel? I think you'll still be able to reproduce this bug in this case unless I'm missing something.

Comment 16

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #15)

> ::: mobile/android/base/home/HomeBanner.java
> @@ +174,1 @@
> >                  if (TextUtils.isEmpty(id) || TextUtils.isEmpty(text)) {
> 
> Not in this patch but, this check should be done before posting to the UI
> thread to avoid unnecessary overhead.

Well, technically this is in the patch now, because previously we would set the visibility to GONE before returning.

> @@ +244,5 @@
> >      }
> >  
> >      private void animateDown() {
> >          // Don't try to animate if the banner is already translated.
> >          if (ViewHelper.getTranslationY(this) == getHeight()) {
> 
> Use mHeight for consistency? Audit the rest of the file.

Oops, looks like this was the only one I missed.

> @@ +317,5 @@
> > +                if (mSnapBannerToTop) {
> > +                    animateUp();
> > +                } else {
> > +                    animateDown();
> > +                    mUserSwipedDown = true;
> 
> Don't you have to set the visibility to GONE here too? I mean: what happens
> if you swipe down while the banner is active then swipe to another panel? I
> think you'll still be able to reproduce this bug in this case unless I'm
> missing something.

I'm not sure what case you're talking about here. The animateDown() call should take care of setting the visibility to GONE.

Comment 17

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #16)
> I'm not sure what case you're talking about here. The animateDown() call
> should take care of setting the visibility to GONE.

Aha, you're right. Missed this.

Comment 18

[:Margaret Leibovic]

Attachment: (v2) Make home banner animation pre-HC friendly

Here's an updated patch.

I tried pretty aggressively to see if I could end up in a busted state on my 2.3 device, but I didn't discover any problems.

However, this patch has turned into a pretty big change, and I think we should consider just disabling the banner on pre-HC devices on aurora/beta, instead of trying to uplift this patch. At the very least, we'd want to let this bake on m-c a while, but it would be good to address this click-intercepting problem on beta before we release an update.

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 8392976 [details] [diff] [review]
(v2) Make home banner animation pre-HC friendly

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

Awesome.

Comment 20

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/5e94da020be1

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e94da020be1

Comment 22

[:Margaret Leibovic]

I'll request uplift after this bakes for a bit.

Comment 23

[:Margaret Leibovic]

Comment on attachment 8392976 [details] [diff] [review]
(v2) Make home banner animation pre-HC friendly

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 960359 (combined with enabling snippets/sync promo banner)
User impact if declined: hidden banner will intercept clicks on pre-honeycomb devices
Testing completed (on m-c, etc.): landed on m-c 3/19
Risk to taking this patch (and alternatives if risky): this changes around how we show/hide the banner, which has been risky in the past, but I've thoroughly tested this locally, and it's baked on m-c for almost a week with no complaints 
String or IDL/UUID changes made by this patch: none

Comment 24

[Aaron Train [:aaronmt]]

Verified against trunk; this is WFM on my Nexus S (2.3). Thanks Margaret.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Has conflicts on Aurora. Please provide a branch patch or request approval for whatever else this depends on.

Comment 26

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c062fecb0698
https://hg.mozilla.org/releases/mozilla-beta/rev/1333165baf22

Comment 27

[Catalin Suciu [:csuciu]]

Verified fixed on Firefox 29 Beta 4 on HTC Desire S (2.3.3)