Closed
Bug 1079303
Opened 10 years ago
Closed 9 years ago
Doorhangers animation is missing
Categories
(Firefox :: Theme, defect)
Firefox
Theme
Tracking
()
People
(Reporter: u428464, Assigned: Gijs)
References
()
Details
(Keywords: regression, testcase)
Attachments
(3 files, 6 obsolete files)
9.21 KB,
patch
|
Dolske
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
36.32 KB,
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
47.38 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Keywords: regression,
regressionwindow-wanted
Assignee | ||
Comment 3•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
This regression moved to Aurora, so the fix (once we have one) needs to be uplifted too.
status-firefox35:
--- → affected
status-firefox36:
--- → affected
tracking-firefox35:
--- → ?
tracking-firefox36:
--- → ?
Updated•10 years ago
|
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.
Comment 7•10 years ago
|
||
Bug 1067367 looks suspicious. I'm willing to bet that it's the regressor.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 8•10 years ago
|
||
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.)
Blocks: 1067367
status-firefox33:
--- → affected
status-firefox34:
--- → affected
tracking-firefox34:
--- → ?
Assignee | ||
Comment 9•10 years ago
|
||
(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)
Assignee | ||
Updated•10 years ago
|
Keywords: regressionwindow-wanted
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
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
Assignee | ||
Comment 13•10 years ago
|
||
There's no getter for transitionsEnabled, that's why this doesn't work.
Assignee | ||
Comment 14•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8508261 -
Flags: review?(mconley)
Comment 15•10 years ago
|
||
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+
Updated•10 years ago
|
Flags: needinfo?(jaws)
Assignee | ||
Comment 16•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8508261 -
Attachment is obsolete: true
Attachment #8508261 -
Flags: review+
Updated•10 years ago
|
Assignee | ||
Comment 17•10 years ago
|
||
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)
Assignee | ||
Comment 18•10 years ago
|
||
(please feel free to take this if you have a better patch; I'm pretty overloaded as it is)
Updated•10 years ago
|
Iteration: 36.1 → 36.2
Comment 19•10 years ago
|
||
(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)
Assignee | ||
Comment 20•10 years ago
|
||
(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. ;-) )
Comment 21•10 years ago
|
||
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)
Comment 22•10 years ago
|
||
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)
Assignee | ||
Comment 23•10 years ago
|
||
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)
Assignee | ||
Comment 24•10 years ago
|
||
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)
Updated•10 years ago
|
Iteration: 36.2 → 36.3
Comment 25•10 years ago
|
||
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)
Assignee | ||
Comment 26•10 years ago
|
||
(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.
Comment 27•10 years ago
|
||
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.
Assignee | ||
Comment 28•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8520052 -
Attachment is obsolete: true
Attachment #8520052 -
Flags: review?(dolske)
Assignee | ||
Comment 29•10 years ago
|
||
try for this patch: remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7e1af91cbd7a
Updated•10 years ago
|
Attachment #8525620 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 30•10 years ago
|
||
(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%...
Assignee | ||
Comment 31•10 years ago
|
||
(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)
Assignee | ||
Comment 32•10 years ago
|
||
(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...
Assignee | ||
Updated•10 years ago
|
Attachment #8525620 -
Attachment is obsolete: true
Assignee | ||
Comment 33•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1a3f2b2fb8d8
Flags: needinfo?(florian)
Flags: needinfo?(dolske)
Assignee | ||
Comment 34•9 years ago
|
||
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)
Comment 35•9 years ago
|
||
This is now a wontfix for 34. I would like to see this fixed early in the 35 beta cycle if possible.
Assignee | ||
Comment 36•9 years ago
|
||
Should be returning the right thing, s/this._ignoreDismissal/this._ignoreDismissal.promise/, d'oh...
Attachment #8528008 -
Flags: review?(dolske)
Assignee | ||
Updated•9 years ago
|
Attachment #8527870 -
Attachment is obsolete: true
Attachment #8527870 -
Flags: review?(dolske)
Assignee | ||
Comment 37•9 years ago
|
||
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.
Assignee | ||
Comment 38•9 years ago
|
||
Oh, um, I guess it makes sense to include the test fixes I did make...
Attachment #8528043 -
Flags: review?(dolske)
Assignee | ||
Updated•9 years ago
|
Attachment #8528043 -
Attachment is obsolete: true
Attachment #8528043 -
Flags: review?(dolske)
Updated•9 years ago
|
Iteration: 36.3 → 37.1
Updated•9 years ago
|
Iteration: 37.1 → 37.2
Assignee | ||
Comment 40•9 years ago
|
||
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 41•9 years ago
|
||
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 42•9 years ago
|
||
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+
Assignee | ||
Comment 43•9 years ago
|
||
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
Assignee | ||
Comment 44•9 years ago
|
||
remote: https://hg.mozilla.org/integration/fx-team/rev/577fdd522d0c remote: https://hg.mozilla.org/integration/fx-team/rev/eef487a50fb0
Assignee | ||
Comment 45•9 years ago
|
||
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).
Updated•9 years ago
|
Iteration: 37.2 → 37.3
Assignee | ||
Comment 46•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=56e2582d5bcb
Assignee | ||
Comment 47•9 years ago
|
||
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...)
Comment 48•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6766cfb95e93 https://hg.mozilla.org/mozilla-central/rev/a76ba87bf502
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment 49•9 years ago
|
||
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
Comment 50•9 years ago
|
||
Too late to consider this for 35 uplift (and we've already shipped it at least twice) so please nominate for aurora uplift.
status-firefox37:
--- → fixed
Comment 51•9 years ago
|
||
Verified on Firefox 37 Nightly according to comment 49.
QA Contact: cornel.ionce
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 52•9 years ago
|
||
Assignee | ||
Comment 53•9 years ago
|
||
aurora try push: remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dd52cf0c9d20
Assignee | ||
Comment 54•9 years ago
|
||
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?
Assignee | ||
Comment 55•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8544548 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8528008 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 56•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ffff4dc528d https://hg.mozilla.org/releases/mozilla-aurora/rev/46551ecd2686
Comment 57•9 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•