Closed Bug 1079303 Opened 5 years ago Closed 5 years ago

Doorhangers animation is missing

Categories

(Firefox :: Theme, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 37
Iteration:
37.3 - 12 Jan
Tracking Status
firefox33 --- wontfix
firefox34 + wontfix
firefox35 + wontfix
firefox36 + verified
firefox37 --- verified

People

(Reporter: ge3k0s, Assigned: Gijs)

References

()

Details

(Keywords: regression, testcase)

Attachments

(3 files, 6 obsolete files)

Since a few days/weeks the doorhangers panels (click to play, password remember, geolocation, etc.) don't have an animation anymore. I don't know what caused the regression but it's worth investigating.
Note that the "classic" toolbar panels show the animation normally.
Also happening on Mac (at least the geolocation panel)
OS: Windows 7 → All
Is this really all/all? Do you have time to get a regression window?
Flags: needinfo?(ge3k0s)
(In reply to :Gijs Kruitbosch from comment #3)
> Is this really all/all? Do you have time to get a regression window?

Not in the next days but I'll try next week.
Flags: needinfo?(ge3k0s)
This regression moved to Aurora, so the fix (once we have one) needs to be uplifted too.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I won't have the occasion to test the regression anytime soon. So if someone has an idea. IIRC the issue appears around the time the e10s click and play bug was implemented, but I may be wrong about this.
Bug 1067367 looks suspicious. I'm willing to bet that it's the regressor.
Flags: needinfo?(gijskruitbosch+bugs)
Yeah, bug 1067367 regressed us here. I'm afraid we're shipping this bug, as that bug got uplifted to 33. :(

[Tracking Requested - why for this release]:

Bug 1067367 got uplifted to 33 (release), and 34 (beta), and that regressed the opening effects on a number of notification panels (click-to-play, for example. Geolocation for another.)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #8)
> Yeah, bug 1067367 regressed us here. I'm afraid we're shipping this bug, as
> that bug got uplifted to 33. :(
> 
> [Tracking Requested - why for this release]:
> 
> Bug 1067367 got uplifted to 33 (release), and 34 (beta), and that regressed
> the opening effects on a number of notification panels (click-to-play, for
> example. Geolocation for another.)

Have you debugged how this regresses us?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mconley)
Not really - it was mostly just a lucky guess and then confirmation. A quick run at it with DOM Inspector shows that we're never actually removing the animate="false" attribute from the popup, but I'm afraid I don't 100% know why at this point.
Flags: needinfo?(mconley)
Taking per discussion with mconley. Trivial testcase from bug 970112.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 36.1
Points: --- → 3
Flags: qe-verify+
Flags: needinfo?(mmucci)
Flags: in-testsuite?
Flags: firefox-backlog+
Keywords: testcase
Added to IT 36.1
Flags: needinfo?(mmucci)
There's no getter for transitionsEnabled, that's why this doesn't work.
Attachment #8508261 - Flags: review?(mconley)
Comment on attachment 8508261 [details] [diff] [review]
add transitions enabled getter to make these work again,

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

Good find. :) r=me, but let's wait for jaws to get back to us on whether or not he can reproduce bug 1067367 on his machine.
Attachment #8508261 - Flags: review?(mconley) → review+
https://www.youtube.com/watch?v=30UpUnfIflU&feature=youtu.be shows this is broken again. I'll see if I can find a way to not break this.
Flags: needinfo?(jaws)
Attachment #8508261 - Attachment is obsolete: true
Attachment #8508261 - Flags: review+
I think bug 1067367 comment #1 is either unclear or partially wrong. Instead of disabling the animation, which isn't really a nice fix, and apparently (see video) disabling only the closing animation isn't good enough anyway, I tried waiting for popuphidden to have occurred before reshowing. This works, but now when you click outside the last popup after showing two popups, the first popup is reshown. I can't figure out why. Florian, can you help elucidate your comment?
Attachment #8508400 - Flags: feedback?(florian)
(please feel free to take this if you have a better patch; I'm pretty overloaded as it is)
Iteration: 36.1 → 36.2
(In reply to :Gijs Kruitbosch from comment #18)
> (please feel free to take this if you have a better patch; I'm pretty
> overloaded as it is)

Gavin - Do you have someone else who can pickup this bug?
Flags: needinfo?(gavin.sharp)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #19)
> (In reply to :Gijs Kruitbosch from comment #18)
> > (please feel free to take this if you have a better patch; I'm pretty
> > overloaded as it is)
> 
> Gavin - Do you have someone else who can pickup this bug?

I'm less busy than I was before, so I might do a better job of actually fixing this this cycle - but I'd still appreciate any ideas from Florian about the patch that's already attached. :-)

(leaving needinfo in case Gavin knows someone who has loads of time and no work. ;-) )
I don't.

Florian, I know you've got the search stuff, but can you provide some feedback here?
Flags: needinfo?(gavin.sharp) → needinfo?(florian)
Ah, I see the state of the bug doesn't reflect the conversation I had with Gijs over IRC.

I looked at the patch a few days ago; it looks good, but when I tried it I also observed the issue Gijs noted in comment 17.

I don't know what's causing this, and one of us needs to debug it. I'm still hoping to find time to do it in the next 2 weeks if Gijs doesn't beat me to it.
Flags: needinfo?(florian)
This fixes the transitions, and fixes bug 1067367, and then fixes the issue with dismissed notifications popping up again (because clearPanel removes nodes and didn't dismiss notifications, after which onPopupHidden got all confused about which bits to dismiss) and also checks so that clicking the arrow-y bit of the urlbar anchor thing doesn't trigger popups. Asking for two reviews not to get it done quicker but to get more eyes on this, because it took me all day to try to figure out how all the bits were meant to work together here, and I'm sure I've missed /something/...
Attachment #8520052 - Flags: review?(florian)
Attachment #8520052 - Flags: review?(dolske)
Comment on attachment 8508400 [details] [diff] [review]
fix popupnotifications code to wait for popuphidden to have happened, f?florian (alternative fix for bug 1067367)

try push for the new patch:

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=36e6f812b84f
Attachment #8508400 - Attachment is obsolete: true
Attachment #8508400 - Flags: feedback?(florian)
Iteration: 36.2 → 36.3
Comment on attachment 8520052 [details] [diff] [review]
always dismiss notifications when clearing the panel,

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

A few questions, but that's it. r+ assuming I'm an idiot. :)

::: toolkit/modules/PopupNotifications.jsm
@@ +496,5 @@
> +    let deferred = Promise.defer();
> +    // popuphidden will reset this back to false:
> +    this._ignoreDismissal = deferred;
> +    if (state != "hiding")
> +      this.panel.hidePopup();

Is the conditional actually needed? Kinda seems like hidePopup() should just be a nop in if called while it's already in the process of hiding.

Also seems like someone calling this while in the |hiding| state already going to be racing with other stuff. For example, onPopupHidden() is only going to resolve the newer promise?

@@ +636,5 @@
>  
>      // If the panel is already open but we're changing anchors, we need to hide
>      // it first.  Otherwise it can appear in the wrong spot.  (_hidePanel is
>      // safe to call even if the panel is already hidden.)
> +    this._hidePanel().then(() => {

Being pessimistic, I wonder how far we're going to need to cascade up the Promise / asyncness. I think any such bugs would already exist, but did you trace though the caller to see if there are lurking bugs?

In other words, most of _showPanel is now running from the then(). The one caller -- _update() -- seems ok with that itself, but what about things calling _update, etc.?
Attachment #8520052 - Flags: review?(florian)
(In reply to Justin Dolske [:Dolske] from comment #25)
> Comment on attachment 8520052 [details] [diff] [review]
> always dismiss notifications when clearing the panel,
> 
> Review of attachment 8520052 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few questions, but that's it. r+ assuming I'm an idiot. :)

Happily/sadly, I don't think you're an idiot at all. :-) / :-(

> ::: toolkit/modules/PopupNotifications.jsm
> @@ +496,5 @@
> > +    let deferred = Promise.defer();
> > +    // popuphidden will reset this back to false:
> > +    this._ignoreDismissal = deferred;
> > +    if (state != "hiding")
> > +      this.panel.hidePopup();
> 
> Is the conditional actually needed? Kinda seems like hidePopup() should just
> be a nop in if called while it's already in the process of hiding.
> 
> Also seems like someone calling this while in the |hiding| state already
> going to be racing with other stuff. For example, onPopupHidden() is only
> going to resolve the newer promise?

Ummm. Dang. Yeah, that seems "interesting". It should probably return the existing promise.

> @@ +636,5 @@
> >  
> >      // If the panel is already open but we're changing anchors, we need to hide
> >      // it first.  Otherwise it can appear in the wrong spot.  (_hidePanel is
> >      // safe to call even if the panel is already hidden.)
> > +    this._hidePanel().then(() => {
> 
> Being pessimistic, I wonder how far we're going to need to cascade up the
> Promise / asyncness. I think any such bugs would already exist, but did you
> trace though the caller to see if there are lurking bugs?
> 
> In other words, most of _showPanel is now running from the then(). The one
> caller -- _update() -- seems ok with that itself, but what about things
> calling _update, etc.?

I've not checked extensively. This is a good point, and I'll try to look at this before resubmitting for review.
Given that we don't yet have a fix, I think we're likely to ship this bug in 34 again. Unless we have a simple patch in the next day or so, I think the right call is to take the fix in 35.
Yeah, so as best I can tell, _hidePanel is only called from _showPanel which is always the last thing in _update which is always the last thing in all its callers which is always the last thing in all its callers, and at that point this is the end of the module (ie would depend on things calling into PopupNotifications). Considering that showPanel has been async for a while now (in that the new panel to be shown would never have been shown sync, even before the animations, AIUI, because the popupshown event isn't fired sync from openPopup) I don't think there should be problems here. I've adjusted the deferred return code stuff.
Attachment #8525620 - Flags: review?(dolske)
Attachment #8520052 - Attachment is obsolete: true
Attachment #8520052 - Flags: review?(dolske)
Attachment #8525620 - Flags: review?(dolske) → review+
(In reply to :Gijs Kruitbosch from comment #29)
> try for this patch:
> 
> remote:  
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7e1af91cbd7a

So unfortunately this was more orange than Dutch football supporters. Some tests are relatively easy to fix, others are... not.

This makes me question my approach (again...) though, as it's clear that we have plenty of consumers that do depend on things happening (semi-) sync. On the other (or rather, yet another) hand, it looks like some of the tests already intermittently fail, perhaps because their assumptions already don't hold 100%...
(In reply to :Gijs Kruitbosch from comment #30)
> (In reply to :Gijs Kruitbosch from comment #29)
> > try for this patch:
> > 
> > remote:  
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7e1af91cbd7a
> 
> So unfortunately this was more orange than Dutch football supporters. Some
> tests are relatively easy to fix, others are... not.
> 
> This makes me question my approach (again...) though, as it's clear that we
> have plenty of consumers that do depend on things happening (semi-) sync. On
> the other (or rather, yet another) hand, it looks like some of the tests
> already intermittently fail, perhaps because their assumptions already don't
> hold 100%...

Florian/Dolske, thoughts? I'm trying to fix the tests, and while e.g. bug1045809.js is fairly easy to fix by just making the test wait for the notification panel to have shown again, applying the same fix to e.g. browser_bug906190.js doesn't work as trivially, it seems...
Flags: needinfo?(florian)
Flags: needinfo?(dolske)
(In reply to :Gijs Kruitbosch from comment #31)
> (In reply to :Gijs Kruitbosch from comment #30)
> > (In reply to :Gijs Kruitbosch from comment #29)
> > > try for this patch:
> > > 
> > > remote:  
> > > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7e1af91cbd7a
> > 
> > So unfortunately this was more orange than Dutch football supporters. Some
> > tests are relatively easy to fix, others are... not.
> > 
> > This makes me question my approach (again...) though, as it's clear that we
> > have plenty of consumers that do depend on things happening (semi-) sync. On
> > the other (or rather, yet another) hand, it looks like some of the tests
> > already intermittently fail, perhaps because their assumptions already don't
> > hold 100%...
> 
> Florian/Dolske, thoughts? I'm trying to fix the tests, and while e.g.
> bug1045809.js is fairly easy to fix by just making the test wait for the
> notification panel to have shown again, applying the same fix to e.g.
> browser_bug906190.js doesn't work as trivially, it seems...

Gotten that to work, but it seems the dismissal notification is firing in test 8 of browser_popupNotification.js . For both notifications. This is because calling the action on one of the notifications ends up calling clearPanel which now dismisses the notifications...

This is very unfortunate because apparently there's code depending on this not being the case, but fixing the test breaks the thing bug 1067367 was meant to fix, so we're back at square one...
Attachment #8525620 - Attachment is obsolete: true
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1a3f2b2fb8d8
Flags: needinfo?(florian)
Flags: needinfo?(dolske)
Attached patch imported patch -v2 (obsolete) — Splinter Review
Basically, I've changed when/what the popupnotifications code clears in terms of existing notifications. This fixes the popupnotification tests that had 'wrong' dismissal calls, and fixes the issue from bug 1067367 and comment 17 (which was that if you click a different popup anchor, the first popup's notifications weren't dismissed). The interdiff with the previous patch should in theory be small (but YMMV with bugzilla...). The try run also uncovered that lots of tests depend on notification.reshow sync-updating the contents of the popupnotification, which this patch changes. Test fixes to follow in a separate patch.
Attachment #8527870 - Flags: review?(dolske)
This is now a wontfix for 34. I would like to see this fixed early in the 35 beta cycle if possible.
Should be returning the right thing, s/this._ignoreDismissal/this._ignoreDismissal.promise/, d'oh...
Attachment #8528008 - Flags: review?(dolske)
Attachment #8527870 - Attachment is obsolete: true
Attachment #8527870 - Flags: review?(dolske)
Renewed try push. I believe this should fix the bc3 issues https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=781f55bd7737

All other orange from the previous push are known intermittent failures.
Oh, um, I guess it makes sense to include the test fixes I did make...
Attachment #8528043 - Flags: review?(dolske)
w/ some cleanup...
Attachment #8528044 - Flags: review?(dolske)
Attachment #8528043 - Attachment is obsolete: true
Attachment #8528043 - Flags: review?(dolske)
Iteration: 36.3 → 37.1
Iteration: 37.1 → 37.2
Comment on attachment 8528008 [details] [diff] [review]
fix popupnotifications.jsm to not hide its async-ness and immediately dismiss open notifications when another anchor is clicked,

Matt, do you have cycles to review this and the other patch?
Attachment #8528008 - Flags: review?(MattN+bmo)
Comment on attachment 8528044 [details] [diff] [review]
part 2: fix tests to not expect sync behaviour from notification.reshow,

rs=me. reflag if there's anything you specifically want me to look at.
Attachment #8528044 - Flags: review?(dolske) → review+
Comment on attachment 8528008 [details] [diff] [review]
fix popupnotifications.jsm to not hide its async-ness and immediately dismiss open notifications when another anchor is clicked,

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

Apologies for the delay.
Attachment #8528008 - Flags: review?(dolske)
Attachment #8528008 - Flags: review?(MattN+bmo)
Attachment #8528008 - Flags: review+
Repushed to try because some of this bitrotted and I'm wary of breaking stuff by landing this:

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=37d72a4df2a9
Backed out because this went perma-orange on Linux asan, and pretty orange on all of linux:

remote:   https://hg.mozilla.org/integration/fx-team/rev/f7f81481bfdb
remote:   https://hg.mozilla.org/integration/fx-team/rev/b8b3358c6e6b

specifically, asan failures included:

11:33:17 INFO - 719 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/plugins/browser_CTP_data_urls.js | uncaught exception - TypeError: PopupNotifications.panel.firstChild._primaryButton is undefined at chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_data_urls.js:89 

which I can repro locally

and the almost-but-not-quite-perma-orange is:

10:44:55 INFO - 570 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_tabMatchesInAwesomebar_perwindowpb.js | undefined assertion name -
10:44:55 INFO - Stack trace:
10:44:55 INFO - chrome://mochitests/content/browser/browser/base/content/test/general/head.js:waitForCondition/interval<:89
10:44:55 INFO - null:null:0
10:45:00 INFO - 572 INFO must wait for focus
10:45:00 INFO - 573 INFO TEST-PASS | browser/base/content/test/general/browser_tabMatchesInAwesomebar_perwindowpb.js | The test tab has 1 or less history entries
10:45:00 INFO - 574 INFO TEST-PASS | browser/base/content/test/general/browser_tabMatchesInAwesomebar_perwindowpb.js | The test tab is on about:blank
10:45:00 INFO - 575 INFO TEST-PASS | browser/base/content/test/general/browser_tabMatchesInAwesomebar_perwindowpb.js | The test tab has no child nodes
10:45:00 INFO - 576 INFO TEST-PASS | browser/base/content/test/general/browser_tabMatchesInAwesomebar_perwindowpb.js | The test tab doesn't have the busy attribute
10:45:00 INFO - 577 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_tabMatchesInAwesomebar_perwindowpb.js | undefined assertion name -
10:45:00 INFO - Stack trace:
10:45:00 INFO - chrome://mochitests/content/browser/browser/base/content/test/general/head.js:waitForCondition/interval<:89
10:45:00 INFO - null:null:0
10:45:32 INFO - Xlib: extension "RANDR" missing on display ":0".
10:45:34 INFO - TEST-INFO | screentopng: exit 0
10:45:34 INFO - 578 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_tabMatchesInAwesomebar_perwindowpb.js | Test timed out - expected PASS
10:45:34 INFO - 579 INFO TEST-OK | browser/base/content/test/general/browser_tabMatchesInAwesomebar_perwindowpb.js | took 45075ms
10:45:34 INFO - 580 INFO checking window state
10:45:34 INFO - 581 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_tabMatchesInAwesomebar_perwindowpb.js | Found a browser window after previous test timed out - expected PASS 

which I can't reproduce easily, and which I also don't understand (in that I don't see why this patch makes that pre-existing random-orange almost perma-fail).
Iteration: 37.2 → 37.3
That try push looks green enough (once more...), so:

remote:   https://hg.mozilla.org/integration/fx-team/rev/6766cfb95e93
remote:   https://hg.mozilla.org/integration/fx-team/rev/a76ba87bf502

(with an extra test fix, and the thing that went perma-fail across linux last time is now disabled on Linux...)
https://hg.mozilla.org/mozilla-central/rev/6766cfb95e93
https://hg.mozilla.org/mozilla-central/rev/a76ba87bf502
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Reproduced the issue on Firefox 36.0a2.
The doorhanger animations are working now properly using latest Nightly, build ID: 20141228030216.
Tested on Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
Too late to consider this for 35 uplift (and we've already shipped it at least twice) so please nominate for aurora uplift.
Verified on Firefox 37 Nightly according to comment 49.
QA Contact: cornel.ionce
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8544548 [details] [diff] [review]
Test fixes for Aurora

Approval Request Comment
[Feature/regressing bug #]: bug 1067367
[User impact if declined]: no doorhanger animations
[Describe test coverage new/current, TBPL]: needs a bunch of test changes, but AFAIK there's no test coverage checking the doorhangers animate
[Risks and why]: medium-low - has had a bunch of baking, and has a bunch of tests that implicitly cover it, and a green try push
[String/UUID change made/needed]: nope
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8544548 - Flags: approval-mozilla-aurora?
Comment on attachment 8528008 [details] [diff] [review]
fix popupnotifications.jsm to not hide its async-ness and immediately dismiss open notifications when another anchor is clicked,

> Approval Request Comment
> [Feature/regressing bug #]: bug 1067367
> [User impact if declined]: no doorhanger animations
> [Describe test coverage new/current, TBPL]: needs a bunch of test changes,
> but AFAIK there's no test coverage checking the doorhangers animate
> [Risks and why]: medium-low - has had a bunch of baking, and has a bunch of
> tests that implicitly cover it, and a green try push
> [String/UUID change made/needed]: nope

The code fix can just be transplanted, the test fix was adapted a bit and attached separately, see the other approval request.
Attachment #8528008 - Flags: approval-mozilla-aurora?
Attachment #8544548 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8528008 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1118768
Confirming the fix on Firefox 36 beta 1, build ID: 20150114125146.
Tested on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit.
Depends on: 1324804
You need to log in before you can comment on or make changes to this bug.