885717 - The bookmark toast notification appears every time you tap on the screen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Build: Firefox for Android 24.0a1 (2013-06-21)
Device: LG Nexus 4
OS: Android 4.2.2

Steps to reproduce:
1. Open Firefox
2. Go to a website
3. Tap on the Bookmark button
4. Wait for the toast notification to dissappear
5. Tap on the screen where the notification was

Expected results:
- After step 5, the toast notifcation should not be displayed every time you tap. 

Actual results:
- The toast notification appears for a short period every time you tap on the position where the notification was. 
Please see the video: https://www.youtube.com/watch?v=_cBA5pv3i7k

Comment 1

[Aaron Train [:aaronmt]]

I'm unable to reproduce this on the HTC One (Android 4.1, Nightly 06/21). Anyways, this is a regression from bug 872388.

Comment 2

[Teodora Vermesan (:TeoVermesan)]

I'm not able to reproduce this on latest Nightly, but I can reproduce on latest Aurora.

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Regression from super toasts landing.

Comment 4

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

(In reply to Teodora Vermesan (:TeoVermesan) from comment #2)
> I'm not able to reproduce this on latest Nightly, but I can reproduce on
> latest Aurora.

Can we get a "fixed-range" on Nightly to figure out what fixed the problem?

Comment 5

[u421692]

The regression window for inbound: 
the commit that fixed the bug:

BAD build: 1373402577
GOOD build:  1373404317

-pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b5af330b0bd0&tochange=7997be475fe8

Comment 6

[u421692]

Seems that Bug 880454 fixed this issue. However, when a link is on the same position where the toast was, it is not clickable, so the toast is hidden but still on front on gecko layer. I will log a separate issue for this.

Comment 7

[u421692]

To consistently reproduce this issue please replace step 5 from STR in comment 0 with: Tap on the screen where the Option button from notification was.

Comment 8

[u421692]

I've logged Bug 900385 for the issue described in comment 6.

Comment 10

[Wesley Johnston (:wesj)]

(In reply to Pop Mihai from comment #6)
> Seems that Bug 880454 fixed this issue. However, when a link is on the same
> position where the toast was, it is not clickable, so the toast is hidden
> but still on front on gecko layer. I will log a separate issue for this.

I think these are the same issue. I"m just duping this.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: anim2

I think this is an Android bug. Our callbacks are all called and we set the View's visibility to GONE. The bug also exists on nightly builds, but we have some filters to ensure we don't fire clicks on the button if its not active that make this harder to see there. Stack overflow gave me:

http://stackoverflow.com/questions/9333220/buttons-within-view-are-still-clickable-even-though-views-visibility-is-gone
http://stackoverflow.com/questions/4728908/android-view-with-view-gone-still-receives-ontouch-and-onclick

Neither of which attempt to explain anything, but do say that using an animation defined in XML worked for someone. It also works for us... Alternatively, people suggest clearing the animation when its done. I haven't tried that.

I think I'll have to dig into some source to really understand what's happening here, but I'd like to have a patch up for beta.

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #11)
> Created attachment 792441 [details] [diff] [review]
> anim2
> 
> I think this is an Android bug. Our callbacks are all called and we set the
> View's visibility to GONE. The bug also exists on nightly builds, but we
> have some filters to ensure we don't fire clicks on the button if its not
> active that make this harder to see there. Stack overflow gave me:
> 
> http://stackoverflow.com/questions/9333220/buttons-within-view-are-still-
> clickable-even-though-views-visibility-is-gone
> http://stackoverflow.com/questions/4728908/android-view-with-view-gone-still-
> receives-ontouch-and-onclick
> 
> Neither of which attempt to explain anything, but do say that using an
> animation defined in XML worked for someone. It also works for us...
> Alternatively, people suggest clearing the animation when its done. I
> haven't tried that.
> 
> I think I'll have to dig into some source to really understand what's
> happening here, but I'd like to have a patch up for beta.

Just wondering: have you tried using our PropertyAnimator bits instead?

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Alt patch

Yeah. This works too.

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 793000 [details] [diff] [review]
Alt patch

Happy to land either.

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 793000 [details] [diff] [review]
Alt patch

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

::: mobile/android/base/widget/ButtonToast.java
@@ +103,5 @@
> +            // Using Android's animation frameworks will not correctly turn off clicking.
> +            // See bug 885717.
> +            PropertyAnimator animator = new PropertyAnimator(duration);
> +            animator.attach(mView, PropertyAnimator.Property.ALPHA, 0.0f);
> +            animator.setPropertyAnimationListener(new PropertyAnimator.PropertyAnimationListener () {

The API has changede to addPropertyAnimatonListener.

Comment 16

[Wesley Johnston (:wesj)]

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

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 793000 [details] [diff] [review]
Alt patch

This is NOT fixed on 25. Just feels like it is because we fight harder not to show the toasts there. You can see a deadzone for panning though.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 872388
User impact if declined: Toasts that live invisibly forever. Annoying
Testing completed (on m-c, etc.): Landed on mc now. Tested locally on both branches
Risk to taking this patch (and alternatives if risky): Low risk. Just changes how an animation is done.
String or IDL/UUID changes made by this patch: none

Comment 18

[Matt Brubeck (:mbrubeck)]

Sorry, backed out on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/ba7d72697344

because of build error:
/builds/slave/fx-team-and-x86-00000000000000/build/mobile/android/base/widget/ButtonToast.java:155: cannot find symbol
symbol  : method setPropertyAnimationListener(<anonymous org.mozilla.gecko.animation.PropertyAnimator.PropertyAnimationListener>)
location: class org.mozilla.gecko.animation.PropertyAnimator
            animator.setPropertyAnimationListener(new PropertyAnimator.PropertyAnimationListener () {

https://tbpl.mozilla.org/php/getParsedLog.php?id=26950453&tree=Fx-Team

Comment 19

[Wesley Johnston (:wesj)]

Forgot lucas' comment. Fixed and relanded:
https://hg.mozilla.org/integration/fx-team/rev/e71c147de2d7

Comment 20

[bhavana bajaj [:bajaj]]

Discussed with wes,the branch approvals/landings are awaiting clean fx-team landing per comment# 19.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e71c147de2d7

Comment 22

[bhavana bajaj [:bajaj]]

Comment on attachment 793000 [details] [diff] [review]
Alt patch

low risk Fx24 regression , approving for uplift

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f7ce0ff85d0d
https://hg.mozilla.org/releases/mozilla-beta/rev/bb7690aa31da

Comment 24

[u421692]

Verified as fixed on:
Device: LG Nexus 4 (Android 4.2.2)
Build: Firefox 26 Nightly (2013-08-27)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Would help if I landed the right patch. Fail.

https://hg.mozilla.org/releases/mozilla-aurora/rev/579bf437a830
https://hg.mozilla.org/releases/mozilla-beta/rev/a4d747b13d79

Comment 26

[u421692]

Verified as fixed on:
Device: LG Nexus 4 (Android 4.2.2)
Build: Firefox 25 Aurora (2013-09-03)

Comment 27

[u421692]

Verified as fixed on:
Device: LG Nexus 4 (Android 4.2.2)
Build: Firefox 24 Beta 8