Closed Bug 885717 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

24 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 26
Tracking Status
firefox24 --- verified
firefox25 --- verified
firefox26 --- verified
fennec 24+ ---

People

(Reporter: TeoVermesan, Assigned: wesj)

References

Details

(Keywords: regression)

Attachments

(2 files)

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
I'm unable to reproduce this on the HTC One (Android 4.1, Nightly 06/21). Anyways, this is a regression from bug 872388.
Blocks: 872388
Flags: needinfo?(wjohnston)
Keywords: regression
I'm not able to reproduce this on latest Nightly, but I can reproduce on latest Aurora.
Regression from super toasts landing.
tracking-fennec: --- → ?
(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?
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
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.
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.
I've logged Bug 900385 for the issue described in comment 6.
Duplicate of this bug: 900385
(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.
tracking-fennec: ? → 24+
Assignee: nobody → wjohnston
Attached patch anim2Splinter Review
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.
Attachment #792441 - Flags: review?(lucasr.at.mozilla)
Flags: needinfo?(wjohnston)
(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?
Attached patch Alt patchSplinter Review
Yeah. This works too.
Comment on attachment 793000 [details] [diff] [review]
Alt patch

Happy to land either.
Attachment #793000 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #793000 - Flags: review?(lucasr.at.mozilla) → review+
Attachment #792441 - Flags: review?(lucasr.at.mozilla)
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
Attachment #793000 - Flags: approval-mozilla-beta?
Attachment #793000 - Flags: approval-mozilla-aurora?
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
Forgot lucas' comment. Fixed and relanded:
https://hg.mozilla.org/integration/fx-team/rev/e71c147de2d7
Discussed with wes,the branch approvals/landings are awaiting clean fx-team landing per comment# 19.
https://hg.mozilla.org/mozilla-central/rev/e71c147de2d7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Comment on attachment 793000 [details] [diff] [review]
Alt patch

low risk Fx24 regression , approving for uplift
Attachment #793000 - Flags: approval-mozilla-beta?
Attachment #793000 - Flags: approval-mozilla-beta+
Attachment #793000 - Flags: approval-mozilla-aurora?
Attachment #793000 - Flags: approval-mozilla-aurora+
Keywords: verifyme
Verified as fixed on:
Device: LG Nexus 4 (Android 4.2.2)
Build: Firefox 26 Nightly (2013-08-27)
Verified as fixed on:
Device: LG Nexus 4 (Android 4.2.2)
Build: Firefox 25 Aurora (2013-09-03)
Verified as fixed on:
Device: LG Nexus 4 (Android 4.2.2)
Build: Firefox 24 Beta 8
tracking-fennec: 24+ → ---
Duplicate of this bug: 926147
tracking-fennec: --- → 24+
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.