This was reported in https://groups.google.com/forum/#!topic/mozilla.dev.media/CY7FNwpF7Xk - specific to the mic/camera doorhanger in gUM. People don't know where doorhangers go when they disappear (Bug 1004055), and they seem to go away a little too easily in some cases where arguably the user never meant to "dismiss" them. Two of those cases are: 1. User leaves the browser and comes back (adjust volume, keyboard mode, incoming call) 2. User switches tab and back. I think in these two cases, regardless of the other bug, the doorhangers should not go away, and instead there when they come back. I think in people's minds, these actions don't "touch" the page, which we should respect and present it as they left it, and not infer any action from it (such as dismissal). It would presumably still go away when you click on the page (though not on the first window-re-activating click perhaps when the window is inactive).

Review commit: https://reviewboard.mozilla.org/r/52503/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52503/

This WIP doesn't dispense with the prompt dismissal entirely (Not Now and X still work as before), but it ensures that the permission prompts persist in app or tab switches and when clicking anywhere else, but not on tab navigation. I believe that is everything the design spec asks for, with the exception of Not Now and X, which will probably be dealt in other bugs. The prompts affected are all the permission prompts except for the following: - click-to-play-plugins (spec says to change this too, but I want to confirm as it is quite different) - xpinstall-disabled, addon-install-blocked, addon-install-failed, addon-install-restart, addon-theme-change (same here) - servicesInstall (a specific kind of add-on I'd think) - enable-e10s (should we make it persistent for consistency, or leave it as is for minimum annoyance?) - a11y_enabled_with_e10s (same as above) - offline-app-usage, offline-app-requested- (not very important as they are AppCache-related I believe, which is already deprecated, but I don't know how to test them) I haven't run any automated tests yet. Ash, can you advise on the remaining prompts?

(In reply to Panos Astithas [:past] from comment #5) > This WIP doesn't dispense with the prompt dismissal entirely (Not Now and X > still work as before), but it ensures that the permission prompts persist in > app or tab switches and when clicking anywhere else, but not on tab > navigation. I believe that is everything the design spec asks for, with the > exception of Not Now and X, which will probably be dealt in other bugs. > > The prompts affected are all the permission prompts except for the following: > > - click-to-play-plugins (spec says to change this too, but I want to confirm > as it is quite different) > - xpinstall-disabled, addon-install-blocked, addon-install-failed, > addon-install-restart, addon-theme-change (same here) > - servicesInstall (a specific kind of add-on I'd think) > - enable-e10s (should we make it persistent for consistency, or leave it as > is for minimum annoyance?) > - a11y_enabled_with_e10s (same as above) > - offline-app-usage, offline-app-requested- (not very important as they are > AppCache-related I believe, which is already deprecated, but I don't know > how to test them) > > I haven't run any automated tests yet. > > Ash, can you advise on the remaining prompts? I think for consistency-sake it is ok to treat them all the same if the user switches tabs and comes back or leaves the browser and comes back - though for this second case (leaving browser) - I want to make sure I understand what that means. I'm assuming it means switch to another application and then switch back to Firefox not quit firefox and re-open it?

Correct, switch to another app with Firefox remaining in the background. Another thing to consider about the plugin & add-on prompts is that they have some complicated timeout behavior where the prompt remains visible for some time (5 minutes IIRC) and then can be dismissed. This makes me think that there must have been some meticulous design around that behavior that we might have to consider. Granted, that might have been an attempt to overcome the easy dismissal issue, but perhaps you know who to ask to confirm (I think Larissa Co designed at least the one for plugins).

(In reply to Panos Astithas [:past] from comment #7) > Correct, switch to another app with Firefox remaining in the background. > > Another thing to consider about the plugin & add-on prompts is that they > have some complicated timeout behavior where the prompt remains visible for > some time (5 minutes IIRC) and then can be dismissed. This makes me think > that there must have been some meticulous design around that behavior that > we might have to consider. Granted, that might have been an attempt to > overcome the easy dismissal issue, but perhaps you know who to ask to > confirm (I think Larissa Co designed at least the one for plugins). Add-ons have a whole team that have been re-working the installation flow so I'm not worried about us not handling improving that flow. Plug-ins I think its ok to migrate into the new design because we're removing ways to accidentally dismiss by making the notification modal and therefore shouldn't need any time-based design.

https://reviewboard.mozilla.org/r/52503/#review49880 ::: toolkit/modules/PopupNotifications.jsm:764 (Diff revision 1) > > + if (notificationsToShow.some(n => n.options.persistent)) { > + this.panel.setAttribute("noautohide", "true"); Note that @noautohide introduces many problems which I learned about through the only other current Fx consumer (UITour). Many of the problems should be fixed in the platform but never get prioritized though I saw one bug (bug 1206133) had some activity recently. See bug 1207419 for a collection of issues (mostly related to @noautohide). Examples: * minimizing (bug 1109868 would fix this) * window dragging repositioning lag (related to one of the bugs connected to bug 1206133) * fullscreen mode (bug 1209960 and bug 1109868 for the case of the toolbar auto-hiding on non-Mac therefore losing the anchor) * Issues with the arrow of the panel becoming wrong upon window movement (related to one of the bugs connected to bug 1206133) * <panel noautohide="true"> is particularly weird on Linux with some window managers since the <panel> is then seen as a top-level window. I think we should get bug 1109868 prioritized by platform as it's something UITour and this prompt (will) have to continually workaround. I think @noautohide is the proper (perhaps only) tool for the job but the implementation is buggy (probably because nothing else was using it in stock Fx with <panel> before UITour) and it's why I think this change will cause a lot of follow-ups. We should definitely get QE verification early on. ::: toolkit/modules/PopupNotifications.jsm:765 (Diff revision 1) > + if (notificationsToShow.some(n => n.options.persistent)) { > + this.panel.setAttribute("noautohide", "true"); > + } else { > + this.panel.removeAttribute("noautohide"); > + } Note that changing the attribute has no effect if the panel is already shown. Please check if that's a problem here.

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52503/diff/1-2/

Patch updated to cover all relevant prompts. I'll look into tests and Matt's comments next.

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #9) > Note that changing the attribute has no effect if the panel is already > shown. Please check if that's a problem here. I don't believe that the panel can be shown here, as this code is called after the panel is explicitly hidden via this._hidePanel().

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52503/diff/2-3/

I could reproduce most of the issues mentioned in comment 9 on Linux, but not on OS X. I haven't tried Windows yet, but I'll see if I can raise the priority of some of these bugs. Fixing test browser_bug553455.js is what is blocking review at the moment.

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52503/diff/3-4/

https://reviewboard.mozilla.org/r/52503/#review51556 ::: browser/base/content/browser-addons.js:235 (Diff revision 4) > + persistent: true, > timeout: Date.now() + 30000, So this is what fixed that test: letting the main add-on permission notifications persist across reloads. This is something that I'm not sure we want to carry on forward, but debating that with the add-ons team will take a while and I'm in no particular rush to do so.

To add a bit of context about our plans, once this change is approved for landing it will go to the elm twig, not m-c, while we get the rest of the UX design implemented. Once elm gets to a shape where it presents a consistent end state to the user, we will merge it back to m-c.

https://reviewboard.mozilla.org/r/52503/#review51568 (In reply to Panos Astithas [:past] from comment #5) > The prompts affected are all the permission prompts except for the following: What about password manager? ::: toolkit/modules/PopupNotifications.jsm:297 (Diff revision 4) > * timeout: A time in milliseconds. The notification will not > * automatically dismiss before this time. > * persistWhileVisible: > * A boolean. If true, a visible notification will always > * persist across location changes. > + * persistent: A boolean. If true, the notification will always pesist Typo: "pesist"

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52503/diff/4-5/

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #18) > What about password manager? Oops, I had seen that it is opened dismissed and ignored it, but I see now that its behavior is more complicated than that. I'll change it too. > Typo: "pesist" Thanks, fixed.

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). https://reviewboard.mozilla.org/r/52503/#review52476 ::: browser/base/content/browser-addons.js:236 (Diff revision 5) > var notificationID = aTopic; > - // Make notifications persist a minimum of 30 seconds > + // Make notifications persistent > var options = { > displayURI: installInfo.originatingURI, > + persistent: true, > timeout: Date.now() + 30000, It looks like you removed most "timeout" values, but not this one. Is there a reason why this notification is different? ::: browser/components/nsBrowserGlue.js:2570 (Diff revision 5) > } > > if (!aOptions) > aOptions = {}; > aOptions.displayURI = requestPrincipal.URI; > + aOptions.persistent = aOptions.persistent || true; What's the expected behavior of this line? ::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js (Diff revision 5) > promptMsg, > "password-notification-icon", > mainAction, > secondaryActions, > { > - timeout: Date.now() + 10000, Can you explain with words the expected behavior change here? It seems to me that you are replacing a persitance accross location changes (for 10s) with a persistence across tab/app switches.

(In reply to Florian Quèze [:florian] [:flo] from comment #21) > It looks like you removed most "timeout" values, but not this one. Is there > a reason why this notification is different? This is what I mentioned in comment 16. There is an add-on test that expects the installation prompt to persist across reloads. I don't know if we want to support this going forward, but in this patch I chose not to break that expectation until I get buy-in from both UX and the add-ons team. > > + aOptions.persistent = aOptions.persistent || true; > > What's the expected behavior of this line? My thought was that perhaps we would want to differentiate the behavior of some DOM permissions in the future with respect to prompt persistence. Like, maybe geolocation would be persistent, but pointer lock wouldn't be. This is just to allow for that option, but it's such a trivial change that I don't really mind changing it to "aOptions.persistent = true;" if you want. > > - timeout: Date.now() + 10000, > > Can you explain with words the expected behavior change here? > > It seems to me that you are replacing a persitance accross location changes > (for 10s) with a persistence across tab/app switches. Actually not in this case, persistence across location changes is guaranteed by persistWhileVisible:true. What the timeout was used for in my understanding was to keep the prompt around even in the face of no location changes, which should no longer be necessary.

(In reply to Panos Astithas [:past] from comment #22) > (In reply to Florian Quèze [:florian] [:flo] from comment #21) > > > + aOptions.persistent = aOptions.persistent || true; > > > > What's the expected behavior of this line? > > My thought was that perhaps we would want to differentiate the behavior of > some DOM permissions in the future with respect to prompt persistence. Like, > maybe geolocation would be persistent, but pointer lock wouldn't be. This is > just to allow for that option, but it's such a trivial change that I don't > really mind changing it to "aOptions.persistent = true;" if you want. I would prefer that, yes. The line currently in the patch really looks like dead code to me. If the point of it is to show this is the right place to change a behavior in the future, then I think adding a comment is better. > > > - timeout: Date.now() + 10000, > > > > Can you explain with words the expected behavior change here? > > > > It seems to me that you are replacing a persitance accross location changes > > (for 10s) with a persistence across tab/app switches. > > Actually not in this case, persistence across location changes is guaranteed > by persistWhileVisible:true. What the timeout was used for in my > understanding was to keep the prompt around even in the face of no location > changes, which should no longer be necessary. I misread this code due to the broken indent on this line http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm?rev=30e050c04c4e#438 :( (would you mind fixing it if you edit this file again soon?) So, persistWhileVisible will make the notification persist across location changes only if the panel is open (which should always be the case with the new persistent:true). The timeout field is only used inside the "locationChange" method. I think it was used to preserve the notification across location changes, if the notification was already dismissed (ie. an icon in the location bar remained, but the panel was closed). If dismissing the notification now requires an explicit user action, the timeout is probably indeed unnecessary.

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52503/diff/5-6/

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). https://reviewboard.mozilla.org/r/52503/#review52894 The patch looks reasonable to me. A few details of the behavior that seem strange when testing the patch (none of them should block landing IMHO) : - when switching between applications, the persistent notification stays visible, but when switching between tabs, we have an animation when the panel reopens. I think the reason why we have this animation is that showing notifications automatically after switching tab happened only when a notification had been triggered in a background tab and never shown to the user before. In the case introduced by this patch, I think it would make sense to suppress the animation when reshowing a notification the user has already seen. - when a web page has both a persistent and a non-persistent notification, and the non-persistent notification is currently shown, clicking anywhere outside the panel (including switching to another appliation) causes the non-persistent notification to be dismissed and the persistent notification to reopen with an animation, which is distracting/surprising. I don't have any suggestion to improve this, as the patch seems to be behavior as specified here; it's probably just an unfortunate corner case. - if we keep the [X] icon (but I think we intend to remove it soon), I think clicking the anchor icon should also dismiss the notification (even when it's persistent). Have you considered writing a test for the persistent:true feature? I think it would really be useful to have it covered. ::: toolkit/modules/PopupNotifications.jsm:298 (Diff revision 5) > * automatically dismiss before this time. > * persistWhileVisible: > * A boolean. If true, a visible notification will always > * persist across location changes. > + * persistent: A boolean. If true, the notification will always > + * persist even across tab and app changes (but not across Should this comment also say that the notification will never be dismissed implicitly?

(In reply to Florian Quèze [:florian] [:flo] from comment #25) > - when a web page has both a persistent and a non-persistent notification, > and the non-persistent notification is currently shown, clicking anywhere > outside the panel (including switching to another appliation) causes the > non-persistent notification to be dismissed and the persistent notification > to reopen with an animation, which is distracting/surprising. I don't have > any suggestion to improve this, as the patch seems to be behavior as > specified here; it's probably just an unfortunate corner case. If you want to try, the testcase I used is: data:text/html,<iframe src="https://people.mozilla.org/~fqueze2/webrtc/" height=300></iframe><iframe src="https://people.mozilla.org/~fqueze2/webrtc/" height=300></iframe> where I shared the microphone in the first iframe, and clicked the "video" button in the second one.

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52503/diff/6-7/

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). https://reviewboard.mozilla.org/r/52503/#review53142 The test doesn't cover the following cases that are the main reasons we are fixing this bug: - ensuring the notification panel is still open after switching to another tab, and back. - ensuring the notification is still open after switching to another window, and back. Also, are we sure Escape shouldn't dismiss a persistent notification? Shouldn't we instead use EventUtils.synthesizeMouse to fake a click on the webpage? ::: browser/base/content/test/popupNotifications/browser_popupNotification_4.js:311 (Diff revision 7) > + // dismissed. > + { id: "Test#15", > + run: function* () { > + this.oldSelectedTab = gBrowser.selectedTab; > + gBrowser.selectedTab = gBrowser.addTab("about:blank"); > + yield promiseTabLoadEvent(gBrowser.selectedTab, "http://example.com/"); Does this test really need to load example.com in a new tab? It doesn't seem to do anything that's tab-specific. ::: browser/base/content/test/popupNotifications/browser_popupNotification_4.js:322 (Diff revision 7) > + }, > + onShown: function* (popup) { > + this.complete = false; > + > + // Notification should persist after dismissal via the ESC key. > + dismissNotification(popup); If dismissing somehow becomes async, your test will pass even if the feature it's testing is broken.

https://reviewboard.mozilla.org/r/52503/#review52894 > Should this comment also say that the notification will never be dismissed implicitly? I added this in my next iteraton of the patch.

https://reviewboard.mozilla.org/r/52503/#review53142 > Does this test really need to load example.com in a new tab? It doesn't seem to do anything that's tab-specific. It only loads the new URIs in the same tab to test the doorhanger behavior after navigation (promiseTabLoadEvent calls gBrowser.selectedTab.linkedBrowser.loadURI).

https://reviewboard.mozilla.org/r/52503/#review53142 > It only loads the new URIs in the same tab to test the doorhanger behavior after navigation (promiseTabLoadEvent calls gBrowser.selectedTab.linkedBrowser.loadURI). I don't understand. yield promiseTabLoadEvent; is before the notification is created, so there's no load event while the notification exists.

https://reviewboard.mozilla.org/r/52503/#review53142 > I don't understand. yield promiseTabLoadEvent; is before the notification is created, so there's no load event while the notification exists. Sorry, I was looking at MozReview's single line of context and I thought you were talking about the one in onShown! This was boilerplate code copied from another test, but in general I prefer to use new tabs for tests that affect the contents of the tab (in this case via navigation) to make sure no expectations of following tests are violated.

Changing the bug summary to reflect what is being implemented since it's quite different that the current one.

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52503/diff/7-8/

https://reviewboard.mozilla.org/r/52503/#review53142 Added tests for both cases and changed the other test to use a mouse click instead of using Escape.

https://reviewboard.mozilla.org/r/52503/#review53142 > If dismissing somehow becomes async, your test will pass even if the feature it's testing is broken. This may be true, but I'm having a hard time imagining how to avoid such an intangible potential case (modulo the hacky solution of using a timeout). I'm sure other tests will misbehave if such a change is made without going through existing tests and verifying their correct behavior, similarly to how switching from sync to async promises in devtools in the past was a rocky ride.

https://reviewboard.mozilla.org/r/52503/#review49880 > Note that @noautohide introduces many problems which I learned about through the only other current Fx consumer (UITour). Many of the problems should be fixed in the platform but never get prioritized though I saw one bug (bug 1206133) had some activity recently. See bug 1207419 for a collection of issues (mostly related to @noautohide). Examples: > * minimizing (bug 1109868 would fix this) > * window dragging repositioning lag (related to one of the bugs connected to bug 1206133) > * fullscreen mode (bug 1209960 and bug 1109868 for the case of the toolbar auto-hiding on non-Mac therefore losing the anchor) > * Issues with the arrow of the panel becoming wrong upon window movement (related to one of the bugs connected to bug 1206133) > * <panel noautohide="true"> is particularly weird on Linux with some window managers since the <panel> is then seen as a top-level window. > > I think we should get bug 1109868 prioritized by platform as it's something UITour and this prompt (will) have to continually workaround. > > I think @noautohide is the proper (perhaps only) tool for the job but the implementation is buggy (probably because nothing else was using it in stock Fx with <panel> before UITour) and it's why I think this change will cause a lot of follow-ups. We should definitely get QE verification early on. I have so far confirmed that OS X doesn't suffer from these issues, but Linux does exhibit some of them and I expect Windows may do so as well. I am trying to get some help with these platform bugs, but in the meantime I don't think this should block this patch from landing, as it will land on elm first until prompts are ready and we can determine when to merge it to m-c later.

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). MozReview is weird. Matt, feel free to review if you have the time and inclination, but I didn't mean to pile on your already big stack of reviews. And I certainly didn't mean to get a review from myself; I always r- those.

(In reply to Panos Astithas [:past] from comment #37) > https://reviewboard.mozilla.org/r/52503/#review49880 > > > Note that @noautohide introduces many problems which I learned about through the only other current Fx consumer (UITour). Many of the problems should be fixed in the platform but never get prioritized though I saw one bug (bug 1206133) had some activity recently. See bug 1207419 for a collection of issues (mostly related to @noautohide). Examples: > > * minimizing (bug 1109868 would fix this) Reproducible on OS X 10.10 > > * window dragging repositioning lag (related to one of the bugs connected to bug 1206133) Reproducible on OS X 10.10 but I didn't look at STR so I only could reproduce when the panel anchor is off screen. The panel will jump between being anchored at the anchor and the edge of the screen with a lag. > > * fullscreen mode (bug 1209960 and bug 1109868 for the case of the toolbar auto-hiding on non-Mac therefore losing the anchor) > > * Issues with the arrow of the panel becoming wrong upon window movement (related to one of the bugs connected to bug 1206133) Reproducible on OS X 10.10 (see above which is about the lag). > > * <panel noautohide="true"> is particularly weird on Linux with some window managers since the <panel> is then seen as a top-level window. > > > > I think we should get bug 1109868 prioritized by platform as it's something UITour and this prompt (will) have to continually workaround. > > > > I think @noautohide is the proper (perhaps only) tool for the job but the implementation is buggy (probably because nothing else was using it in stock Fx with <panel> before UITour) and it's why I think this change will cause a lot of follow-ups. We should definitely get QE verification early on. > > I have so far confirmed that OS X doesn't suffer from these issues, but > Linux does exhibit some of them and I expect Windows may do so as well. I am > trying to get some help with these platform bugs, but in the meantime I > don't think this should block this patch from landing, as it will land on > elm first until prompts are ready and we can determine when to merge it to > m-c later. I don't know how you're testing but I definitely can reproduce these issues on OS X 10.10.5. Of course it's fine to land on Elm (and ideally rebase to m-c later instead of merging) but I'm pretty sure we won't want to ship without some <panel> fixes.

Perhaps I haven't made this clear, but my testing for the platform bugs has only been in relation to the permission prompts and not UI tour or other doorhangers. This makes some issues irrelevant as, for instance, the anchor will always be visible. And I have only tested OS X on 10.11. Let's get together next week to compare notes and perhaps you could walk me through reproducing the issues on OS X with my patched local build.

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). https://reviewboard.mozilla.org/r/52503/#review54782 I kept "Open an Issue" checked for all the following comments, but they may not all need code changes. ::: browser/base/content/test/popupNotifications/browser_popupNotification_5.js:118 (Diff revision 8) > + // Test that persistent allows the notification to persist until explicitly > + // dismissed. > + { id: "Test#5", > + run: function* () { > + this.oldSelectedTab = gBrowser.selectedTab; > + gBrowser.selectedTab = gBrowser.addTab("about:blank"); I still don't see why this test needs to happen in a new tab opened specifically for it. In comment 32 you said you prefer new tabs for tests that "affect the contents of the tab (in this case via navigation)", but unless you think the click in the center of the body is likely to be unfortunate enough to hit a link, I don't think there's any navigation here. ::: browser/base/content/test/popupNotifications/browser_popupNotification_5.js:169 (Diff revision 8) > + }, > + // Second part of the previous test that compensates for the limitation in > + // runNextTest that expects a single onShown/onHidden invocation per test. > + { id: "Test#6b", > + run: function* () { > + ok(gNotification.id.endsWith("Test#6a"), "Should have found the notification from Test6a"); Won't this pass even if the notification isn't there anymore? (I mean, it looks like gNotification will keep a reference anyway) ::: browser/base/content/test/popupNotifications/browser_popupNotification_5.js:170 (Diff revision 8) > + // Second part of the previous test that compensates for the limitation in > + // runNextTest that expects a single onShown/onHidden invocation per test. > + { id: "Test#6b", > + run: function* () { > + ok(gNotification.id.endsWith("Test#6a"), "Should have found the notification from Test6a"); > + ok(PopupNotifications.isPanelOpen, "Should have shown the popup again after getting back to the tab"); Are we sure this will be consistently true? Not waiting for a popupshown notification makes me a bit nervous here, as there are animations when (re)opening the panel. ::: browser/base/content/test/popupNotifications/browser_popupNotification_5.js:193 (Diff revision 8) > + let anchor = win.document.getElementById("default-notification-icon"); > + win.PopupNotifications._reshowNotifications(anchor); > + ok(win.PopupNotifications.panel.childNodes.length == 0, > + "no notification displayed in new window"); > + win.close(); > + ok(this.notification.id.endsWith("Test#7"), "Should have found the notification from Test7"); Like for the previous test, I think this is testing a value on a local object that we created, and will pass even if the notification no longer exists. ::: browser/base/content/test/popupNotifications/browser_popupNotification_5.js:194 (Diff revision 8) > + win.PopupNotifications._reshowNotifications(anchor); > + ok(win.PopupNotifications.panel.childNodes.length == 0, > + "no notification displayed in new window"); > + win.close(); > + ok(this.notification.id.endsWith("Test#7"), "Should have found the notification from Test7"); > + ok(PopupNotifications.isPanelOpen, "Should have shown the popup again after getting back to the window"); Should we also test this while the second window is above the first one (ie. before the win.close() call) ?

(In reply to Panos Astithas [:past] from comment #40) > Perhaps I haven't made this clear, but my testing for the platform bugs has > only been in relation to the permission prompts and not UI tour or other > doorhangers. This makes some issues irrelevant as, for instance, the anchor > will always be visible. Well, it could be off-screen like I mentioned. > And I have only tested OS X on 10.11. Let's get > together next week to compare notes and perhaps you could walk me through > reproducing the issues on OS X with my patched local build. I was testing with your patch using the geolocation prompt on maps.google.com and then minimized the window.

https://reviewboard.mozilla.org/r/52503/#review54782 > I still don't see why this test needs to happen in a new tab opened specifically for it. In comment 32 you said you prefer new tabs for tests that "affect the contents of the tab (in this case via navigation)", but unless you think the click in the center of the body is likely to be unfortunate enough to hit a link, I don't think there's any navigation here. I need the navigation here for synthesizeMouse to work with a regular web page. Using it on the current tab with about:blank throws.

https://reviewboard.mozilla.org/r/52503/#review54782 > Won't this pass even if the notification isn't there anymore? (I mean, it looks like gNotification will keep a reference anyway) This part will pass, but the next line will fail in that case. The reason I put this additional check in is to guard against other popups from previous tests lingering around and fooling the next line that everything is peachy.

https://reviewboard.mozilla.org/r/52503/#review54782 > Like for the previous test, I think this is testing a value on a local object that we created, and will pass even if the notification no longer exists. See my previous comment for line 169.

https://reviewboard.mozilla.org/r/52503/#review54782 > Are we sure this will be consistently true? Not waiting for a popupshown notification makes me a bit nervous here, as there are animations when (re)opening the panel. You are worried about intermittents, right? I don't think this is going to be a problem here, because isPanelOpen returns true in both "showing" and "open" states, so it will be true even if the test reaches this statement before the animation completes. I never saw this check fail locally when running with --run-until-failure, so that's some additional reassurance. That said, if it becomes a problem I could consider completely rewriting this test to not use the same format as the other tests in this directory. But since this is going to be significant effort, I would prefer to spend the time only if it becomes a problem in practice.

Addressed the other comments and improved the notification id check per our discussion on IRC.

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52503/diff/8-9/

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #42) > (In reply to Panos Astithas [:past] from comment #40) > > Perhaps I haven't made this clear, but my testing for the platform bugs has > > only been in relation to the permission prompts and not UI tour or other > > doorhangers. This makes some issues irrelevant as, for instance, the anchor > > will always be visible. > > Well, it could be off-screen like I mentioned. > > > And I have only tested OS X on 10.11. Let's get > > together next week to compare notes and perhaps you could walk me through > > reproducing the issues on OS X with my patched local build. > > I was testing with your patch using the geolocation prompt on > maps.google.com and then minimized the window. Oh, are both issues about the reposition/hiding lag? Because that I can reproduce, just not the problem of the panel remaining open when minimizing, the lag on movement or the arrow flipping.

(In reply to Panos Astithas [:past] from comment #46) > https://reviewboard.mozilla.org/r/52503/#review54782 > > > Are we sure this will be consistently true? Not waiting for a popupshown notification makes me a bit nervous here, as there are animations when (re)opening the panel. > > You are worried about intermittents, right? Right. > I don't think this is going to > be a problem here, because isPanelOpen returns true in both "showing" and > "open" states, so it will be true even if the test reaches this statement > before the animation completes. Ok. > That said, if it becomes a problem I could consider completely rewriting > this test to not use the same format as the other tests in this directory. > But since this is going to be significant effort, I would prefer to spend > the time only if it becomes a problem in practice. Makes sense, let's hope it won't cause new oranges :-).

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). https://reviewboard.mozilla.org/r/52503/#review55686 Looks good to me, thanks for the updates!

Landed on elm: https://hg.mozilla.org/projects/elm/rev/9aab50bd0e19

(In reply to Panos Astithas [:past] from comment #49) > (In reply to Matthew N. [:MattN] (behind on reviews) from comment #42) > > (In reply to Panos Astithas [:past] from comment #40) > > > Perhaps I haven't made this clear, but my testing for the platform bugs has > > > only been in relation to the permission prompts and not UI tour or other > > > doorhangers. This makes some issues irrelevant as, for instance, the anchor > > > will always be visible. > > > > Well, it could be off-screen like I mentioned. > > > > > And I have only tested OS X on 10.11. Let's get > > > together next week to compare notes and perhaps you could walk me through > > > reproducing the issues on OS X with my patched local build. > > > > I was testing with your patch using the geolocation prompt on > > maps.google.com and then minimized the window. > > Oh, are both issues about the reposition/hiding lag? Because that I can > reproduce, just not the problem of the panel remaining open when minimizing, > the lag on movement or the arrow flipping. No, the panel remains visible on the screen.

Comment on attachment 8752266 [details] Use a zip instead of an xpi file to test document states in order to avoid the permission prompt (bug 1004061). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52503/diff/9-10/

Review commit: https://reviewboard.mozilla.org/r/61930/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61930/

Comment on attachment 8767455 [details] Make permission doorhangers persistent (bug 1004061). This has already been reviewed.

Looking at the elm twig test results there were some accessibility test failures that I originally thought were unrelated, but it turned out to be caused by this change. The test causing the failures (test_doc_busy.html) was not among the ones identified by treeherder, but it was causing a download of an xpi file to test the document status. Since the other patch made the add-on permission prompt persistent, it would remain there even after the test finished and eventually cause other subsequent tests to time out. Changing the test to explicitly close the permission prompt seemed very hard, as it is a plain mochitest without sufficient access to the browser chrome. But since the file extension doesn't seem to matter for this particular test I made it download a zip file instead and the test still works as before.

> Changing the test to explicitly close the permission prompt seemed very > hard, as it is a plain mochitest without sufficient access to the browser > chrome. But since the file extension doesn't seem to matter for this > particular test I made it download a zip file instead and the test still > works as before. Actually, its a lot more similar to mochitest-chrome so you can poke at whatever chrome only stuff you like and close the window. That might be nice since I think it would solve the intermitant test concerns in bug 446469. so, with the zip file does a popup appear and then go away? or what happens in that case? If there is a popup for some period of time this is probably fine, if not I think you are just making the test not test anything.

(In reply to Trevor Saunders (:tbsaunde) from comment #59) > so, with the zip file does a popup appear and then go away? or what happens > in that case? If there is a popup for some period of time this is probably > fine, if not I think you are just making the test not test anything. Yes, this is precisely what happens, a popup window to select the download location appears and then closes. The number of pass messages is the same as without the patch.

(In reply to Panos Astithas [:past] from comment #60) > (In reply to Trevor Saunders (:tbsaunde) from comment #59) > > so, with the zip file does a popup appear and then go away? or what happens > > in that case? If there is a popup for some period of time this is probably > > fine, if not I think you are just making the test not test anything. > > Yes, this is precisely what happens, a popup window to select the download > location appears and then closes. The number of pass messages is the same as > without the patch. Sounds good. I'm not actually sure if the number of messages would change, but I'll believe the popup appears for downloads.

Pushed by pastithas@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/db1c3781f5ff Use a zip instead of an xpi file to test document states in order to avoid the permission prompt . r=tbsaunde

Posted a rebased version of the patch on top of fx-team tip.

Pushed by paolo.mozmail@amadzone.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/33ea134bf627 Make most doorhanger notifications persistent. r=florian

Comment on attachment 8812582 [details] Bug 1004061 - Make most doorhanger notifications persistent. Given you landed this in comment 69, I assume setting the review flag again was just a reviewboard accident, please set the flag again if I got this wrong.

Build ID: 20170202030211 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0 Verified as fixed on on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64 on Firefox Nightly 54.0a1, Firefox Nightly 53.0a1 and Aurora 53.0a2.