Closed Bug 982181 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

29 Branch
All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- verified
firefox30 --- fixed
firefox31 --- verified
fennec 29+ ---

People

(Reporter: edwong, Assigned: Margaret)

Details

(Keywords: reproducible, Whiteboard: [2.3 only])

Attachments

(1 file, 4 obsolete files)

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
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.
Flags: needinfo?(edwong)
android 2.3 only - droid x.  No repro on S4 with android 4.2
Flags: needinfo?(edwong)
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
: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.
Blocks: 799726
Priority: -- → P1
This has to do with the Sync promo banner. Margaret, are you able to reproduce? I see this on my Nexus S (2.3).
No longer blocks: 799726
Component: Android Sync → General
Flags: needinfo?(margaret.leibovic)
Keywords: reproducible
Product: Android Background Services → Firefox for Android
Summary: tapping history clock and tabs buttons takes me the 'account verified' page → Tapping on History panel buttons interact with banner (Android 2.3)
Whiteboard: [2.3 only]
Priority: P1 → --
tracking-fennec: --- → ?
Assignee: nobody → margaret.leibovic
Flags: needinfo?(margaret.leibovic)
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.
Attachment #8389480 - Flags: review?(lucasr.at.mozilla)
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 on attachment 8389480 [details] [diff] [review]
Disable the home banner on pre-honeycomb devices

Clearing review for extra clarification.
Attachment #8389480 - Flags: review?(lucasr.at.mozilla)
(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.
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.
Attachment #8389480 - Attachment is obsolete: true
Attachment #8390568 - Flags: review?(lucasr.at.mozilla)
Attached patch Hide home baner on animation end (obsolete) — Splinter Review
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.
Attachment #8390622 - Flags: review?(lucasr.at.mozilla)
tracking-fennec: ? → 29+
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.
Attachment #8390622 - Flags: review?(lucasr.at.mozilla) → review-
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.
Attachment #8390568 - Flags: review?(lucasr.at.mozilla)
Attachment #8390568 - Attachment is obsolete: true
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).
Attachment #8390622 - Attachment is obsolete: true
Attachment #8392591 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8392591 - Flags: review?(lucasr.at.mozilla) → feedback+
(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.
(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.
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.
Attachment #8392591 - Attachment is obsolete: true
Attachment #8392976 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8392976 [details] [diff] [review]
(v2) Make home banner animation pre-HC friendly

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

Awesome.
Attachment #8392976 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/5e94da020be1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
I'll request uplift after this bakes for a bit.
Flags: needinfo?(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
Attachment #8392976 - Flags: approval-mozilla-beta?
Attachment #8392976 - Flags: approval-mozilla-aurora?
Flags: needinfo?(margaret.leibovic)
Attachment #8392976 - Flags: approval-mozilla-beta?
Attachment #8392976 - Flags: approval-mozilla-beta+
Attachment #8392976 - Flags: approval-mozilla-aurora?
Attachment #8392976 - Flags: approval-mozilla-aurora+
Verified against trunk; this is WFM on my Nexus S (2.3). Thanks Margaret.
Status: RESOLVED → VERIFIED
Has conflicts on Aurora. Please provide a branch patch or request approval for whatever else this depends on.
Flags: needinfo?(margaret.leibovic)
Verified fixed on Firefox 29 Beta 4 on HTC Desire S (2.3.3)
You need to log in before you can comment on or make changes to this bug.