The attached patch adds an additional popuppositioned event that fires in-between popupshowing and popupshown that fires only for arrow panels (for now). The arrow panel uses this to update the arrow position. It also fires whenever the popup is moved or resized in any manner. It is a work in progress to see if it fixes some issues with the UI tour and developer tools arrow panel usage.

Comment on attachment 8662994 [details] [diff] [review] Add popuppositioned event Matt, would you have time to check if this fixes any issues related to the UI tour? It seems to improve things a bit for me on Windows, but I can't reproduce all of the issues.

agibson may have time to check this so I did a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49bcf6454af1 Click on a "B" icon beside your OS and then click the link beside "Build: …" to find the builds. You would want the DMG for OS X.

I tested the build linked in Comment 2 and these are my observations: 1.) I can no longer reproduce Bug 1203904. UITour Info Panels seem to retain their correct size after following the steps to reproduce in that bug :) 2.) The behavior described in Bug 1204480 seems to have changed a little. When I first click the arrow to collapse the Control Center, the info panel now remains positioned exactly where it was. When I click the arrow again to expand, the info panel jumps position (but corrects itself again). 3.) Bug 1188400 does not seem to be fixed. It actually seems slightly worse, as the panel previously corrected itself after a few seconds of being positioned incorrectly. Now it seems to remain off position after the bug occurs (I tested my physically moving the browser window). 4.) Bug 1109868 is not fixed (I understand this may be out of scope for this patch, but thought I should still mention it as it does impact info panel positioning). If there is anything else I can test specifically more than happy to help, please let me know. Thanks, Neil!

(In reply to Alex Gibson [:agibson] from comment #3) > 2.) The behavior described in Bug 1204480 seems to have changed a little. > When I first click the arrow to collapse the Control Center, the info panel > now remains positioned exactly where it was. When I click the arrow again to > expand, the info panel jumps position (but corrects itself again). Matt, what would you say is the ideal behavior for this bug in particular? I understand from a UX perspective it seems odd to have the info panel jump around once it is shown, but I'm also conscious it needs to not break behavior elsewhere (e.g. sub-menus?). Happy to defer to what you think should happen here technically.

OK, thanks for testing. This patch is only intended to address issues with the position/size of the panel when it opens, which is bugs 1203904 and 1188400. My testing showed improvement on Windows for both bugs, but I couldn't reproduce either on Mac.

Comment on attachment 8662994 [details] [diff] [review] Add popuppositioned event Oops, meant to clear feedback instead of needinfo. I didn't realize there were both… (In reply to Alex Gibson [:agibson] from comment #4) > (In reply to Alex Gibson [:agibson] from comment #3) > > 2.) The behavior described in Bug 1204480 seems to have changed a little. > > When I first click the arrow to collapse the Control Center, the info panel > > now remains positioned exactly where it was. I think we need bug 1109868 so that the panel would disappear when the control center is closed (I assume that's what you mean by collapsed and not something related to subviews). Neil, let me know if you wanted something more than the manual testing? Did you want me to give feedback on the patch diff too?

(In reply to Matthew N. [:MattN] (behind on mail) from comment #6) > I think we need bug 1109868 so that the panel would disappear when the > control center is closed (I assume that's what you mean by collapsed and not > something related to subviews). Sorry no, I didn't mean when someone closes the control center. I meant when they trigger the subview (as per the screencast in bug 1204480)

No that is good. I am working on improving the patch to fix tests that fail due to this. Bug 1109868 is a separate issue and would require some other architectural work.

This version makes some improvements: - fixes a crash if a popup is closed while opening - allows more situations for the arrow to update when the panel changes size - fixes a case where the arrow is on the wrong side when the popup needs to be flipped while open

This version makes the event fire synchronously, which fixes all the issues where callers assume that openPopup does that, fixing all tests.

OK, so neither of these two versions is perfect. This version goes back to asynchronous use and fixes and fires the event after the popup has been positioned as it should have done all along. Several tests still fail, mainly due to code that assumes that it can open a popup and then immediately do something.

Since there is a patch in Comment 11 but no review or other flag I wonder wants missing here to get this fixed? As a devtools user I do not much care about this bug, but rather about the various UI bugs it blocks ;)

A number of tests are broken with this patch. In a few cases, the test can be fixed, but others are non-test code that relies on a popup opening synchronously.

So this means that the patch here can only land when the blocked (popup UI related) bugs adapt (change their tests) at the same time? Sounds like a deadlock, unless it's being worked on this and the popup tests at the same time. What's the plan? Do you need help here or will you fix the tests yourself? (just asking, unfortunately, I cannot really offer help myself to be frank due to missing skills and time atm)

It doesn't relate to the blocked bugs. I don't have time right now to fix these tests and the code they relate to isn't areas that I am familiar with. If someone ones to help that would be great. I don't have a complete list of tests that failed, some of them are: addon-sdk/source/test/test-panel.js.test toolkit/components/passwordmgr/test/browser/browser_filldoorhanger.js devtools/client/debugger/test/mochitest/browser_dbg_conditional-breakpoints-02.js devtools/client/debugger/test/mochitest/browser_dbg_server-conditional-bp-02.js browser/base/content/test/general/browser_bug432599.js - opens popup then does some stuff synchronously, including focusing an inputField at editBookmarkOverlay.js:initPanel. This fails as the popup is not yet visible. browser/base/content/test/general/browser_bug455852.js browser/base/content/test/plugins/browser_CTP_drag_drop.js browser/components/privatebrowsing/test/browser/browser_privatebrowsing_geoprompt.js - leaks browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js

I set up a try build to get a more complete list: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73095c8fa7e1

Mac's compiler complains that nsXULPopupPositionedEvent::mIsOpening is never used [1]. From what I can see, this seems to be the case. Is there a reason to we would want to keep this variable? [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=27d55e71542d&selectedJob=20436340

No, it's left over from an earlier version of the patch.

Removed unused variable

I cleaned up this patch containing some possible fixes I had made for some of the failing tests. Some of them I think didn't actually work. I know for sure that browser_filldoorhanger.js didn't work but I don't know about the others. I'm posting this here as a start.

Minor rebase to get the patch to apply on mozilla-central

Can we also get rid of [1] now? If we're now adjusting the arrow position after resize, we shouldn't need to do it after arbitrary DOM modifications anymore. [1]: https://dxr.mozilla.org/mozilla-central/rev/f3f2fa1d7eed5a8262f6401ef18ff8117a3ce43e/browser/components/customizableui/content/panelUI.xml#388

(In reply to Kris Maglione [:kmag] from comment #26) > Can we also get rid of [1] now? If we're now adjusting the arrow position > after resize, we shouldn't need to do it after arbitrary DOM modifications > anymore. > > [1]: > https://dxr.mozilla.org/mozilla-central/rev/ > f3f2fa1d7eed5a8262f6401ef18ff8117a3ce43e/browser/components/customizableui/ > content/panelUI.xml#388 I think so, but I am not sure. Neil?

This test [1] is one of the tests broken by this patch. It seems that it is testing that a panel shown and immediately hidden will never emit show or hide events. Neil, Kris and I have discussed this test and believe that it incorrectly assumes that the panel will not open asynchronously. Therefore, the test should be rewritten such that it accepts receiving neither event or both events (but not just one event). [1] https://dxr.mozilla.org/mozilla-central/rev/674a552743785c28c75866969aad513bd8eaf6ae/addon-sdk/source/test/test-panel.js#282

> I think so, but I am not sure. Neil? I would think so, but you'll just have to try it to verify.

I have written a patch that allows the Jetpack tests to pass. It is currently pretty hacky looking, but it functions as a proof of concept. However, I know close to nothing about Jetpack so I am concerned about making this change to how panels would work in Jetpack. Kris, can you tell me if this concept is sound?

It should be noted that the patch contains a rewrite of the mechanism that enforces the "only one panel open at a time" behavior. However, it currently only enforces this among these 'sdk panels'. Opening/closing other panels should have no effect on the 'sdk panels'. I don't know enough about these panels to know if this is acceptable or not. Kris- I do not know whether you are the right person to look at this. If not, could you please needinfo the correct person? Thanks!

One additional thought - As I have said, I am not very knowledgeable about these 'sdk panels'. Does the "one open at a time" rule need to be enforced this strictly? A different solution could be to change the test to accept the case wherein the other panel is still in the process of closing rather than requiring that it already be closed.

Comment on attachment 8761605 [details] [diff] [review] jetpack_test_fix.diff Review of attachment 8761605 [details] [diff] [review]: ----------------------------------------------------------------- It looks good to me, but I'm not the best person to review Jetpack code. Gijs or Luca might be better here. ::: addon-sdk/source/lib/sdk/panel.js @@ +264,5 @@ > + > + return this; > + }, > + > + _queueShow: function(options, anchor) { The convention in Jetpack code is for private methods to be implemented as non-exported functions rather than as underscore-prefixed members.

(I'll look properly after the weekend, but I suspect Luca or Mossop would be better reviewers. I don't know very much about jetpack internals.)

(In reply to Kris Maglione [:kmag] from comment #34) > The convention in Jetpack code is for private methods to be implemented as > non-exported functions rather than as underscore-prefixed members. There are a number of things I plan to fix about this code. My main question at the moment is whether or not the concept is acceptable. My main concerns are: - Does the "one at a time" nature of these 'sdk panels' need to extend to all panels? - Is "wait for last panel to finish closing before opening the next one" the desired functionality? - Is this sufficiently robust? In particular, testing for individual popup states and listening for expected events? I have been having a hard time finding a list of possible panel states, so I am not totally sure that I am covering all of them. It also just occurred to me that I did not anticipate multiple panels opening at once. If they are all waiting for the showing popup to close before they open, they will end up all opening at once. I work on a new patch that addresses this.

(In reply to Kirk Steuber [:ksteuber] from comment #36) > - Does the "one at a time" nature of these 'sdk panels' need to extend to I phrased that poorly. When an 'sdk panel' opens, should all types of panels be closed, or just the other 'sdk panels'?

I cannot give the final r+ and I've not a great experience with the SDK panel module, but I can give an initial feedback and ask Gabor (who is reviewing most of my SDK patches nowdays) if he can look at it for a final review. I confirm Kris' advice in Comment 34 about the SDK conventions related to private methods (which are usually achieved through a combination of helpers function which takes the needed objects as parameters and are not exported from the module where there are defined, and private data stored in a private namespace, usually defined using the "sdk/core/namespace" module) To answer the first concern, by looking at the previous code (in particular to the `setupAutoHide` helper), the "one at a time" nature of the panels looks like it needs to be extended to all panels and not only the SDK ones (because it closes the SDK panel when any other kind of popup is showing). Another thing that I notice is that in the original version the module keeps only weak references of the current panel opened, and it is something that I think that it have to be preserved in the final version of the SDK patch (Gijs can probably confirm if this my thought is right or if it doesn't matter).

I really don't know enough about this code to be of help here, I'm afraid. I don't really understand why the SDK is manually tracking opening/closing popups. AIUI, unless 'noautohide' is set on any of the panels (which doesn't look like it's the case from a quick glance at the panel.js code, but maybe I'm missing something?) I would expect any user interaction which triggers another panel to also close the previous panel... does the code have to deal with programmatically opened panels, or something?

The documentation at https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/panel just says "Opening a panel will close an already opened panel." But I think you should assume that it means only sdk panels. There isn't currently a way to determine the list of open panels, so I assume the addon sdk keeps track of its own open panel. As long as the criteria holds that opening a new sdk panel closes any existing open sdk panel with this patch applied, let's not spend a lot of time on further analyzing this just to fix a test.

I'm OK with only closing SDK panels, regardless of what the old code did. Closing arbitrary panels that we don't know anything about is bound to have consequences that we can't predict or handle properly. (In reply to :Gijs Kruitbosch from comment #39) > I don't really understand why the SDK is manually tracking opening/closing > popups. AIUI, unless 'noautohide' is set on any of the panels (which doesn't > look like it's the case from a quick glance at the panel.js code, but maybe > I'm missing something?) I would expect any user interaction which triggers > another panel to also close the previous panel... does the code have to deal > with programmatically opened panels, or something? That's a good point. I'd probably expect opening a new popup to close an existing autohide popup. But I don't think it does, since I remember at one point, when we were accidentally opening panels for buttons in the menu popup, we wound up with both popups open at once, with strange layout issues. Maybe it would be easier to just fix that corner case and remove the panel closing logic from the SDK code.

(In reply to Kris Maglione [:kmag] from comment #41) > I'm OK with only closing SDK panels, regardless of what the old code did. > Closing arbitrary panels that we don't know anything about is bound to have > consequences that we can't predict or handle properly. That is what the current SDK code in that module does, it closes only SDK panels if there is any other popup showing (regardless if the new popup showing is an SDK panel or not), but it doesn't close any popup which is not created by the SDK itself. My apologies if it wasn't clear in my previous comment.

I nearly have a patch ready for the Jetpack tests. Who should I seek review from? Luca, you thought I should ask Gabor Krizsanits for a review. Is that the best person for this?

Yes, Gabor is reviewing most of my SDK patches and I think he could be a good reviewer for this one too.

This patch fixes the Jetpack tests that were broken by the popuppositioned patch. The patch includes a rewrite of the system that prevents multiple popups from being open simultaneously. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f767d941405

Comment on attachment 8764016 [details] [diff] [review] jetpack_test_fix.patch Review of attachment 8764016 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Kirk Steuber [:bytesized] from comment #48) > Created attachment 8764016 [details] [diff] [review] > jetpack_test_fix.patch > > This patch fixes the Jetpack tests that were broken by the popuppositioned > patch. The patch includes a rewrite of the system that prevents multiple > popups from being open simultaneously. > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f767d941405 This seems like lot of code to fix a broken test, on an area that no one ever really understood and any attempt to refactor turned out to be a failure... So before we jump in, could you explain me why is this test broken and to fix that why do you need to rewrite this mechanism entirely? I would be much happier for an easier fix. This code is from before I even joined Mozilla, but I think the reason why we need this mechanism on the first place is purely technical. For panels we create a panel in the background (used to be hidden window now days I think it's a windowless docshell) and when we want to display it we swap the frame loader / docshell of that bg panel and an empty panel that belongs to the currently displayed tab. It's also purely technical why is a panel belong to any tab but that is the case (or at least was back then, I have no idea if it has changed but I would assume it has not). My memory is foggy about it and I have never worked on this code, but I'm afraid it's way too easy to mess up something and swap the wrong thing in some edge cases which will result a chain of disastrous events. I'm not sure how good is our test coverage here. Anyway, please describe me the problem first that we are trying to fix here for this test to pass. If we can avoid this rewrite then we should try, if not I will try my best to review it. I'm cancelling the review until then so you won't miss my comment.

Briefly, the issue is that the code is assuming that <xul:panel> will open synchronously and that when multiple panels are open and closed at once that the popupshowing/popupshown/popuphiding/popuphiding events will fire on multiple panels in a predictable order. Kirk, we may want to consider adding a an attribute to <xul:panel> to disable sending the popuppositioned event for these addon panels as a workaround. The addon panels would still have the bugs related to it but at least most panels would not.

Thanks for the explanation! (In reply to Neil Deakin from comment #50) > Briefly, the issue is that the code is assuming that <xul:panel> will open > synchronously Shouldn't we just fix that assumption in the test then? I don't see how the SDK code is relying on that fact. In fact Comment 28 describes exactly that. Were there any attempt to just fix the test? I mean even with this patch the panel.show() will not become sync and the test still had to be fixed. For that by the way I think the right fix is to wait for the show event and then calling hide because the current one will only test if we open and close the panel quickly there won't be any event fired, which seems less useful. In general a lot of the SDK tests were there to alarm the team about fundamental platform changes so they can be evaluated and either fix the SDK accordingly or try to revert the change (if it's just an unwanted regression). In this case I'm still not quite sure what is broken other than this test. Maybe some add-ons make the same assumption as this test, but then even with this patch add-on breakage will happen. > and that when multiple panels are open and closed at once that > the popupshowing/popupshown/popuphiding/popuphiding events will fire on > multiple panels in a predictable order. Is that relevant for the SDK? If we just make sure we shut down the panel that is being shown or about to be shown then we should be golden, no?

To be clear, two tests are broken here: "Hide Before Show"[1] and "Only One Panel Open Concurrently"[2]. (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #51) > Shouldn't we just fix that assumption in the test then? I don't see how the > SDK code is relying on that fact. In fact Comment 28 describes exactly that. > Were there any attempt to just fix the test? The problem is that the test is not the only code making this assumption. The existing autohide mechanism [3] works like this: - When a panel is created, add a system event listener for the "popupshowing" event. - When the listener is called, the panel hides itself With the newly asynchronous panels, this mechanism does not ensure that the visible panel is completely hidden before the new panel is shown. If we just rewrite the tests, we will see events that appear out of order. For example: - panel1.onShow() called - panel2.onShow() called (Uh oh. There are two panels open right now) - panel1.onHide() called This incorrect ordering is currently exactly what the "Only One Panel Open Concurrently" test checks for. Personally, I believe that the test is correct; if two panels cannot be open at once, the events they emit should reflect this. I suppose that if this odd event ordering is acceptable, we could just rewrite the test. I am not sure how the new test should work though. Maybe "When a second panel opens, set a timeout for seeing the first panel close"? What do you think? [1] https://dxr.mozilla.org/mozilla-central/rev/674a552743785c28c75866969aad513bd8eaf6ae/addon-sdk/source/test/test-panel.js#282 [2] https://dxr.mozilla.org/mozilla-central/rev/674a552743785c28c75866969aad513bd8eaf6ae/addon-sdk/source/test/test-panel.js#805 [3] https://dxr.mozilla.org/mozilla-central/rev/c9edfe35619f69f7785776ebd19a3140684024dc/addon-sdk/source/lib/sdk/panel.js#116

Comment on attachment 8764016 [details] [diff] [review] jetpack_test_fix.patch Review of attachment 8764016 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for walking me through and for working on this mess. I think you're doing the right thing, and the patch seem to be correct. You're using |SinglePanelManager| instead of |this| many places, but I'm sure you have your reasons, and I don't want to hold up this patch any longer with nits anyway, but there is one last thing I don't get and I must ask: ::: addon-sdk/source/lib/sdk/panel.js @@ +127,4 @@ > > +var SinglePanelManager = { > + visiblePanel: null, > + enqueuedPanels: [], I wonder if you really need an array here or is it enough to remember the last candidate to be shown only. @@ +169,5 @@ > + view.removeEventListener("popuphidden", SinglePanelManager.onVisiblePanelHidden, true); > + view.removeEventListener("popupshown", SinglePanelManager.onVisiblePanelShown, false); > + while (SinglePanelManager.enqueuedPanels.length > 0) { > + let nextPanelData = SinglePanelManager.enqueuedPanels.pop(); > + let nextPanel = getPanelFromWeakRef(nextPanelData[0]); If I get it right the only case where it is useful where I queue multiple panels to be displayed like: panel1.show() panel2.show() panel3.show() and then by the time I get here I have all three in the array, I pop panel3 and it turns out that it's already GCed / disposed but for some reason panel2 or panel1 isn't so we display that. Seems like an unimportant edge case I wouldn't worry about too much and kept the code simpler instead but I might be missing something... Do you have any specific scenario in mind you're trying to guard against here? Anyway, I can see how this preserves the old behavior so I'm fine leaving it as it is if you think it's necessary.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #53) > You're using > |SinglePanelManager| instead of |this| many places, but I'm sure you have > your reasons Yeah, I'm not super excited about doing it this way, but it was the cleanest way I knew of. The problem is that I want some of the methods to be added as event listeners, then later removed. In order to remove it I need a named function, so anonymous functions are out. However, when I pass in the member function, the value of |this| is lost. I could retain it with bind (or in other ways), but I think that doing so would complicate removing the event listener. Given that it is a named singleton object, it seemed simplest just to always refer to it by name rather than carefully ensuring that the methods are always called with the correct value of |this|. > I wonder if you really need an array here or is it enough to remember the > last candidate to be shown only. You are probably right about this. I was thinking that it made more sense with an array, but the more that I think about it, it seems like it is better without it. It simplifies the code, and I think the effect actually makes more sense. The last panel opened should have closed all panels before it. I will upload the revised patch once I have finished revising and testing it.

Revised patch to fix Jetpack tests that were broken by the popuppositioned patch. The patch includes a rewrite of the system that prevents multiple popups from being open simultaneously. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=909e3cc73835

Comment on attachment 8765572 [details] [diff] [review] jetpack_test_fix.patch Review of attachment 8765572 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Kirk Steuber [:bytesized] from comment #54) > Yeah, I'm not super excited about doing it this way, but it was the cleanest > way I knew of. Makes sense to me. > I will upload the revised patch once I have finished revising and testing it. Looks good, thanks again!

As discussed on IRC, I have a WIP patch that fixes devtools tests (https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8713ec62296517dc950a5eb42a352ef05cf88a7) Still needs a bit of cleanup, will submit for review soon.

jdescottes: I tried incorporating your patch into my testing, but I am seeing a lot of devtools failures. https://treeherder.mozilla.org/#/jobs?repo=try&revision=55737b21bdf9

This patch will hopefully fix the browser chrome test failures caused by the popuppositioned patch. On this run, it looks like it fixes all test failures that were introduced: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdf7ce6f8084 On this run, where it is incorporated with the other test-fix patches, there still seem to be some problems: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55737b21bdf9

(In reply to Kirk Steuber [:bytesized] from comment #68) > jdescottes: I tried incorporating your patch into my testing, but I am > seeing a lot of devtools failures. > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=55737b21bdf9 I'm only seeing 2 devtools failures (other than the known intermittents) : - devtools/client/shared/test/browser_html_tooltip-03.js : did not change since my proposed patch, did anything change on your side? - devtools/client/inspector/rules/test/browser_rules_colorpicker-release-outside-frame.js : changed recently, probably now incompatible with the patch here. You mentioned a lot of failures, am I missing anything? For the record all the other devtools tests that used to fail in your first try run did not reappear here, so looks like they're still good.

Some code in the browser-chrome-testfixes patch affects the behavior of Firefox as opposed to fixing just a test script. That code has been moved to this patch so that there is just one patch that affects browser behavior. A try run of this patch (rebased on mozilla-central) is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=193564ba5226

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75a7f70feaa39d058f883b193b83a1b6a7bbb5c7 Should be green for devtools tests.

Although it looks like the devtools tests are passing in Linux and OSX, it looks like there are still some failures in Windows. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bc8205736cc jdescottes: Could you take a look at these?

Fixes the browser-chrome mochitests that were failing with the popuppositioned patch applied. This try run shows everything but devtools passing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bc8205736cc The try run's revision was created by applying the popuppositioned patch, then the jetpack patch, then the devtools patch, then the browser-chrome patch.

Comment on attachment 8775318 [details] [diff] [review] browser-chrome-testfixes.patch ># HG changeset patch ># User Kirk Steuber <ksteuber@mozilla.com> ># Date 1466628104 25200 ># Wed Jun 22 13:41:44 2016 -0700 ># Node ID 1bc8205736cc855f563ec7794fc3f10b473a5a8f ># Parent 3685da56931f21ee983669470f895c3c866c3f38 >Bug 1206133 - Fix browser chrome mochitests broken by the popuppositioned patch > >diff --git a/browser/base/content/test/urlbar/browser_locationBarCommand.js b/browser/base/content/test/urlbar/browser_locationBarCommand.js >--- a/browser/base/content/test/urlbar/browser_locationBarCommand.js >+++ b/browser/base/content/test/urlbar/browser_locationBarCommand.js >@@ -36,17 +36,20 @@ add_task(function* shift_left_click_test > info("Running test: Shift left click"); > > let newWindowPromise = BrowserTestUtils.waitForNewWindow(); > triggerCommand(true, {shiftKey: true}); > let win = yield newWindowPromise; > > // Wait for the initial browser to load. > let browser = win.gBrowser.selectedBrowser; >- yield BrowserTestUtils.browserLoaded(browser); >+ yield Promise.all([ >+ BrowserTestUtils.browserLoaded(browser), >+ BrowserTestUtils.waitForLocationChange(win.gBrowser, TEST_VALUE) >+ ]); Is the issue here just that about:blank gets loaded? Does this just need to pass the url as the third argument to browserLoaded? (Or, better, simplify this code and pass the second argument to waitForNewWindow? >diff --git a/toolkit/components/passwordmgr/test/browser/browser_notifications.js b/toolkit/components/passwordmgr/test/browser/browser_notifications.js > let notificationElement = PopupNotifications.panel.childNodes[0]; > // Modify the username in the dialog if requested. > if (testCase.usernameChangedTo) { > notificationElement.querySelector("#password-notification-username") >- .setAttribute("value", testCase.usernameChangedTo); >+ .value = testCase.usernameChangedTo; > } > This and the other ones are an issue with the test as is right?

I believe that about:blank does get loaded, but I am not sure whether that is what is causing the problem there. Without waiting for onLocationChange, the test reads that the address bar is empty. It seems that when opening a link in a new window, the location bar is not populated until the location change event fires. With the second one you asked about, I am not sure why the test was failing so consistently with the popuppositioned patch applied since the test seems to have nothing to do with it. However, as mentioned in the documentation [1]: > Using setAttribute() to modify certain attributes, most notably value in XUL, works inconsistently, as the attribute specifies the default value. To access or modify the current values, you should use the properties. For example, use elt.value instead of elt.setAttribute('value', val). [1] https://developer.mozilla.org/en-US/docs/Web/API/Element/setAttribute#Notes

Comment on attachment 8775318 [details] [diff] [review] browser-chrome-testfixes.patch >+ * @resolves When onLocationChange fires. >+ */ >+ waitForLocationChange(tabbrowser, url) { >+ return new Promise((resolve, reject) => { >+ let progressListener = { >+ onLocationChange(aBrowser) { >+ if ((url && aBrowser.currentURI.spec.indexOf(url) == -1) || >+ (!url && aBrowser.currentURI.spec == "about:blank")) { >+ return; >+ } I think the test should just pass the right expected url rather than having this string analysis.

|waitForLocationChange| now tests for url equality rather than performing string analysis. Carrying forward r+

Comment on attachment 8773472 [details] [diff] [review] popuppositioned.patch Neil- Most of this patch is your own code, but I would like you to review the changes I made, which are in these locations: browser/base/content/browser-places.js @@ -248,22 +248,36 browser/components/places/content/editBookmarkOverlay.js @@ -168,16 +168,28 layout/xul/nsXULPopupManager.cpp @@ -1016,16 +1016,34 layout/xul/nsXULPopupManager.cpp @@ -2680,16 +2711,71

Comment on attachment 8773472 [details] [diff] [review] popuppositioned.patch This looks good but I think Marco (:mak) should review the changes to the bookmarks files.

I tried requesting review from him, but apparently he is not accepting review requests.

Marco is on PTO for a couple of weeks, you could send them to Drew (:adw) instead.

new patches for devtools tests : https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e1a350a84aac7e4ac4ada24e0374dfb492ebc73

I see every patch here has an r+ and all the dependencies are resolved. Are these changes ready to land?

I want to do one more try run just to be sure. There have been a lot of bugs related to this patch and I want to make sure that we finally got them all. I will be sure that the try run is kicked off today.

If you want an extra level of testing before Nightly you could consider landing the patch on the Elm twig but check with Panos if that's fine.

I don't really understand what landing on Elm gains us over just applying the patches on mozilla-central and running through try. Why do one over the other?

Elm has fxprivacy developers building on it and testing changes related to permission prompt and control center panels so there are people focused on looking for issues related to panels.

Well that sounds great. How do I push to Elm?

You need to pull from https://hg.mozilla.org/projects/elm and rebase on the tip then push. It might be good to check with Panos though. More info about project repositories: https://wiki.mozilla.org/ReleaseEngineering/DisposableProjectRepositories

Had to rebase the patch on some changes it looks like you made on another patch. Can you take a look at the one spot that caused a merge conflict and make sure it looks ok? layout/xul/nsMenuPopupFrame.cpp @@ -497,17 +497,17

Whoops. Made a little mistake in the last rebase. Here is the corrected one. Still wondering about the same change: layout/xul/nsMenuPopupFrame.cpp @@ -497,17 +497,17

Feel free to land on elm if you want, but I'm regularly merging m-c to it so your changes will end up there shortly after they land in the tree.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c5cb27604f9f Add popuppositioning state and popuppositioned event to improve arrow panel position handling. r=enn, r=adw https://hg.mozilla.org/integration/mozilla-inbound/rev/63acf0fb4c22 Fix for Jetpack bugs caused by the popuppositioned patch. r=gabor https://hg.mozilla.org/integration/mozilla-inbound/rev/e56b51cf75df Fix browser chrome mochitests broken by the popuppositioned patch. r=enn

Backed this out for test failures in the notification tests of the password manager on Linux: https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d79c8362334ee996b7d7743c47e0051062febe https://hg.mozilla.org/integration/mozilla-inbound/rev/1c941f0e1b3ca242ddd46a213c7db96d1ff2a6b0 https://hg.mozilla.org/integration/mozilla-inbound/rev/d4e5dfa481a508b9af8f1be528665b2bf891a60c Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ac598dfc00b28c3217e0354ccc88bcb9de1bb304 For the failures, check especially M(bc3) on the pushes which followed the one containing the patches for this bug. https://treeherder.mozilla.org/logviewer.html#?job_id=34045012&repo=mozilla-inbound > TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_notifications_2.js | Main action button is disabled - false == true - JS frame :: chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_notifications_2.js :: test_empty_password/< :: line 42 https://treeherder.mozilla.org/logviewer.html#?job_id=34050043&repo=mozilla-inbound > TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_notifications.js | Test timed out - Sorry for not pinging you on IRC, wasn't aware of your nick change.

To me it looks like both of the failures that you are pointing out are known intermittents. > TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_notifications_2.js | Main action button is disabled - false == true - JS frame :: chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_notifications_2.js :: test_empty_password/< :: line 42 Bug 1272849 > TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_notifications.js | Test timed out - Bug 1281357 Is there a reason that you think that these failures are not related to the known intermittents I linked?

These intermittents started to occur much more frequent when this bug landed, and from the current results the frequency seems to have dropped after the backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=c001728af0dc37d51deb8b43ba5ac3f3129e40b1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&filter-searchStr=linux%20M(bc3)&selectedJob=34136046

Changes made only to toolkit/modules/PopupNotifications.jsm. It seems that sometimes the remember password dialog is populated before it opens, which does not work properly.

Changes to: toolkit/components/passwordmgr/test/browser/browser_notifications.js toolkit/components/passwordmgr/test/browser/browser_notifications_2.js toolkit/components/passwordmgr/test/browser/browser_notifications_password.js toolkit/components/passwordmgr/test/browser/browser_notifications_username.js

Last attempt at this patch did not pass the linter. This update fixes that. Changes made only to toolkit/modules/PopupNotifications.jsm. It seems that sometimes the remember password dialog is populated before it opens, which does not work properly.

Comment on attachment 8783113 [details] [diff] [review] popuppositioned.patch >+ let popupOpenedPromise = new Promise(resolve => { >+ let target = this.panel; >+ if (target.parentNode) { >+ // NOTIFICATION_EVENT_SHOWN should be fired for the panel before >+ // popupshown because it is used to initialize the panel. >+ // By targeting the panel's parent and using a capturing listener, we >+ // can have our listener called before others waiting for the panel to >+ // be shown (which probably expect the panel to be fully initialized) The code is ok but the comment is a bit misleading as NOTIFICATION_EVENT_SHOWN isn't fired before popupshown. You probably want to rephrase to indicate that NOTIFICATION_EVENT_SHOWN is fired before any bubbling listeners.

I changed the comment and actually the code associated with it as well. I suspect that the code to restart the generator after the yielded promise is resolved at some point waits in the event queue. I want the panel initialization to be done immediately after receiving the popuppositioned event, so I changed the code to reflect that. Again, all code changes are in toolkit/modules/PopupNotifications.jsm

Comment on attachment 8784571 [details] [diff] [review] popuppositioned.patch You should be able to remove the Task.jsm usage and go back to just using then() after _hidePanel.

Reverted unnecessary Task.jsm changes as Neil suggested. Carrying forward r+.

Is this now ready to land?

No. I fixed the 2 bugs pointed out by :aryx, which caused another test[1] to start failing much more frequently. I think I have just about finished fixing that bug, and am in the process of cleaning the patch up and making sure there are no other tests failing more frequently. [1] http://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_bug553455.js

browser_bug553455.js totally rewritten because it was permafailing when other patches in this bug are applied. This patch probably has to be applied after popuppositioned.patch because they both modify PopupNotifications.jsm.

It looks like there is one last test failing: > TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_discovery_install.js | leaked 6 window(s) until shutdown [url = about:addons] I'm taking a look at it now, hopefully afterwards this patch will be ready for check in.

Updated patch slightly to accommodate recent changes to browser_bug553444.js. browser_bug553455.js totally rewritten because it was permafailing when other patches in this bug are applied. This patch probably has to be applied after popuppositioned.patch because they both modify PopupNotifications.jsm.

Oops. Made a little mistake on that last patch. Fixed now. browser_bug553455.js totally rewritten because it was permafailing when other patches in this bug are applied. This patch probably has to be applied after popuppositioned.patch because they both modify PopupNotifications.jsm.

Comment on attachment 8787326 [details] [diff] [review] 553455_fix.patch Review of attachment 8787326 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/PopupNotifications.jsm @@ +735,5 @@ > + // Let tests know that the panel was updated and what notifications it was > + // updated with so that tests can wait for the correct notifications to be > + // added. > + let event = new this.window.CustomEvent("PanelUpdated", > + {'detail': notificationIds}); Nit: use double quotes @@ +797,4 @@ > // required to display the panel has happened. > context.panel.dispatchEvent(new context.window.CustomEvent("Shown")); > + let event = new context.window.CustomEvent("PanelUpdated", > + {'detail': notificationIds}); ditto

Only change made to toolkit/modules/PopupNotifications.jsm. Fixed bug where event listeners waiting for PopupNotifications.panel to show can accumulate, causing a test to report that windows were being leaked.

Changed patch to accommodate changes to PopupNotifications.jsm in popuppositioned.patch. Also fixed the nits mentioned by :MattN. browser_bug553455.js totally rewritten because it was permafailing when other patches in this bug are applied. This patch should be applied after popuppositioned.patch because they both modify PopupNotifications.jsm.

Comment on attachment 8788618 [details] [diff] [review] 553455_fix.patch Review of attachment 8788618 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay, I've been traveling this week. This lgtm, thanks for refactoring the tests to the newer style. Just a note for next time, it would be easier to read a patch of this size if the refactoring/renaming bits were in a separate commit (can be squashed into one before landing if desired), and also if mozreview was used.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0c9c5b508ccd Add popuppositioning state and popuppositioned event to improve arrow panel position handling. r=Enn, r=adw https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0a8096c6e0 Fix for Jetpack bugs caused by the popuppositioned patch. r=gabor https://hg.mozilla.org/integration/mozilla-inbound/rev/b99b5f178405 Fix browser chrome mochitests broken by the popuppositioned patch. r=Enn https://hg.mozilla.org/integration/mozilla-inbound/rev/baaf5f5624d3 Fix browser_bug553455.js such that it does not make invalid assumptions about how panels work and refactor from callbacks to Tasks and Promises. r=rhelmer

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=baaf5f5624d32358ba3a1464dc5c5b3333103116 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=35653851&repo=mozilla-inbound 21:17:52 INFO - 124 INFO TEST-PASS | browser/base/content/test/popupNotifications/browser_popupNotification_checkbox.js | MainAction should be disabled - 21:17:52 INFO - 125 INFO TEST-PASS | browser/base/content/test/popupNotifications/browser_popupNotification_checkbox.js | Checkbox should have the correct label - 21:17:52 INFO - 126 INFO TEST-PASS | browser/base/content/test/popupNotifications/browser_popupNotification_checkbox.js | Checkbox should be shown - 21:17:52 INFO - 127 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/popupNotifications/browser_popupNotification_checkbox.js | Checkbox should be checked by default - Got true, expected false 21:17:52 INFO - Stack trace: 21:17:52 INFO - chrome://mochikit/content/browser-test.js:test_is:960 21:17:52 INFO - chrome://mochitests/content/browser/browser/base/content/test/popupNotifications/browser_popupNotification_checkbox.js:checkCheckbox:18 21:17:52 INFO - chrome://mochitests/content/browser/browser/base/content/test/popupNotifications/browser_popupNotification_checkbox.js:.onShown:180 21:17:52 INFO - chrome://mochitests/content/browser/browser/base/content/test/popupNotifications/head.js:runNextTest/</<:100 21:17:52 INFO - resource://gre/modules/Task.jsm:createAsyncFunction/asyncFunction:243 21:17:52 INFO - resource://gre/modules/Task.jsm:Task_spawn:168 21:17:52 INFO - chrome://mochitests/content/browser/browser/base/content/test/popupNotifications/head.js:runNextTest/<:100 21:17:52 INFO - chrome://mochitests/content/browser/browser/base/content/test/popupNotifications/head.js:onPopupEvent/listener/<:255 21:17:52 INFO - chrome://mochikit/content/browser-test.js:testScope/test_executeSoon/<.run:986 21:17:52 INFO - Not taking screenshot here: see the one that was previously logged

Above comment was for the backout for failing browser-chrome test browser_popupNotification_checkbox.js: https://hg.mozilla.org/integration/mozilla-inbound/rev/43725e6c61b396f0ae965f060f6ccc02a1d6a382 https://hg.mozilla.org/integration/mozilla-inbound/rev/d2deda115569aa52d6fe70042ced70fee27cdbb4 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d37bc3c376e4f05944ae92f1cd0ab886d0b9895 https://hg.mozilla.org/integration/mozilla-inbound/rev/0f0d055ed09a8c234ae192a860c222be430878fd

I am currently working on something else, but I will fix this as soon as I can.

Removed changes to browser_filldoorhanger.js since that test was removed in Bug 1286718. Carrying forward r+.

I suggest to include devtools tests in some of your try runs, in case we added new tests relying on the old behavior during the last 2 months.

It seems that the popuppositioned patch causes the checkbox to not always be visible right away when the panel opens. This patch should fix the browser_popupNotification_checkbox.js test such that it waits for the checkbox to be visible when necessary.

Comment on attachment 8794963 [details] [diff] [review] browser_popupNotification_checkbox.patch Review of attachment 8794963 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine but I'm a bit confused why it's not necessary to add the waiting code in all test cases. Can you please clarify? :)

The checkbox state can be read before it is visible with no problems. The problem arises when we try to click on the checkbox before it is visible (which does not work);

But there are more occasions where we try to click the checkbox (http://searchfox.org/mozilla-central/search?q=synthesizeMouseAtCenter&case=false&regexp=false&path=browser_popupNotification_checkbox)

Good point. Honestly, I couldn't tell you why the test is failing at that point and not at the others. I have been unable to reproduce the test failure locally or find the underlying cause of the failure. The best I could do was to determine that the checkbox was not visible when it should be. I am hoping to get Neil Deakin's input when he returns from PTO and improve this fix. At the moment, however, the test is passing with this patch applied and we would really like to close this bug because it has been open for a long time and it is blocking other work.

(In reply to Kirk Steuber [:bytesized] from comment #161) > Good point. Honestly, I couldn't tell you why the test is failing at that > point and not at the others. I have been unable to reproduce the test > failure locally or find the underlying cause of the failure. The best I > could do was to determine that the checkbox was not visible when it should > be. I am hoping to get Neil Deakin's input when he returns from PTO and > improve this fix. At the moment, however, the test is passing with this > patch applied and we would really like to close this bug because it has been > open for a long time and it is blocking other work. I can totally understand that. I'd just like to avoid intermittent failures and if the checkbox is really sometimes not there yet it might be safer to always wait for it to appear. Can you turn this into a function and call it whenever we try to click the checkbox, please? Thanks!

Comment on attachment 8795458 [details] [diff] [review] browser_popupNotification_checkbox.patch Review of attachment 8795458 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/popupNotifications/browser_popupNotification_checkbox.js @@ +24,5 @@ > is(warningLabel.hidden, !disabled, "Warning label should be shown"); > is(mainAction.disabled, disabled, "MainAction should be disabled"); > } > > +function elementVisiblePromise(element) { Nit: I think the more common style would be to name this promiseElementVisible @@ +28,5 @@ > +function elementVisiblePromise(element) { > + // HTMLElement.offsetParent is null when the element is not visisble > + // (or if the element has |position: fixed|). See: > + // https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/offsetParent > + yield BrowserTestUtils.waitForCondition(() => element.offsetParent !== null, you need to either replace `yield` with `return` or make the function a generator for this to work properly @@ +29,5 @@ > + // HTMLElement.offsetParent is null when the element is not visisble > + // (or if the element has |position: fixed|). See: > + // https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/offsetParent > + yield BrowserTestUtils.waitForCondition(() => element.offsetParent !== null, > + "Waiting for checkbox to be visible"); Nit: since your function is now generic you could replace "checkbox" with e.g. "element" here

Comment on attachment 8795513 [details] [diff] [review] browser_popupNotification_checkbox.patch Review of attachment 8795513 [details] [diff] [review]: ----------------------------------------------------------------- Looks good now, thanks!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bca4534616ad Add popuppositioning state and popuppositioned event to improve arrow panel position handling. r=enndeakin https://hg.mozilla.org/integration/mozilla-inbound/rev/9401602eabf3 Fix for Jetpack bugs caused by the popuppositioned patch. r=gabor https://hg.mozilla.org/integration/mozilla-inbound/rev/06f3f82059c8 Fix browser chrome mochitests broken by the popuppositioned patch. r=enndeakin https://hg.mozilla.org/integration/mozilla-inbound/rev/b0e0213a3b2b Fix browser_bug553455.js such that it does not make invalid assumptions about how panels work and refactored from callbacks to Tasks and Promises. r=rhelmer https://hg.mozilla.org/integration/mozilla-inbound/rev/4a6f6c64ae89 Fix browser_popupNotification_checkbox.js, which was broken by the popuppositioned patch. r=jhofmann

https://hg.mozilla.org/mozilla-central/rev/bca4534616ad https://hg.mozilla.org/mozilla-central/rev/9401602eabf3 https://hg.mozilla.org/mozilla-central/rev/06f3f82059c8 https://hg.mozilla.org/mozilla-central/rev/b0e0213a3b2b https://hg.mozilla.org/mozilla-central/rev/4a6f6c64ae89

(In reply to :Gijs from comment #39) > AIUI, unless 'noautohide' is set on any of the panels (which doesn't look like it's the case from a > quick glance at the panel.js code, but maybe I'm missing something?) I would expect any user > interaction which triggers another panel to also close the previous panel... No, several panels are often opened at the same time, see long-standing ignored by Firefox team bugs bug 1171394, bug 1187647