Improve arrow panel position handling

RESOLVED FIXED in Firefox 52

Status

()

Core
XUL
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: Neil Deakin (not available until Aug 9), Assigned: bytesized)

Tracking

(Blocks: 4 bugs)

unspecified
mozilla52
Points:
3
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [fce-active])

Attachments

(5 attachments, 30 obsolete attachments)

8.79 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
39.48 KB, patch
Neil Deakin (not available until Aug 9)
: review+
Details | Diff | Splinter Review
83.39 KB, patch
rhelmer
: review+
Details | Diff | Splinter Review
16.88 KB, patch
bytesized
: review+
Details | Diff | Splinter Review
5.76 KB, patch
johannh
: review+
Details | Diff | Splinter Review
Created attachment 8662994 [details] [diff] [review]
Add popuppositioned event

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.
Attachment #8662994 - Flags: feedback?(MattN+bmo)
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.
Flags: needinfo?(agibson)
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!
Flags: needinfo?(agibson)
(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.
Flags: needinfo?(MattN+bmo)
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.
Blocks: 1207419
Flags: needinfo?(MattN+bmo)
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?
Attachment #8662994 - Flags: feedback?(MattN+bmo)
(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.
Created attachment 8669069 [details] [diff] [review]
Add popuppositioned event, v2

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
Attachment #8662994 - Attachment is obsolete: true
Created attachment 8694694 [details] [diff] [review]
Add popuppositioned event, v3

This version makes the event fire synchronously, which fixes all the issues where callers assume that openPopup does that, fixing all tests.
Blocks: 1207536
Blocks: 1203904
Created attachment 8695966 [details] [diff] [review]
Add popuppositioned event, v4

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.
Attachment #8669069 - Attachment is obsolete: true
Attachment #8694694 - Attachment is obsolete: true
Blocks: 1210328
Blocks: 1188400

Updated

a year ago
Blocks: 1197446

Updated

a year ago
Blocks: 1197621

Updated

a year ago
No longer blocks: 1197446

Comment 12

a year ago
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.

Comment 14

a year ago
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
(Assignee)

Updated

a year ago
Assignee: enndeakin → ksteuber
(Assignee)

Comment 17

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b582605fa65
(Assignee)

Comment 18

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27d55e71542d
(Assignee)

Comment 19

a year ago
Created attachment 8749705 [details] [diff] [review]
Patch: Rebase of Neil's patch to add popuppositioned event
Attachment #8695966 - Attachment is obsolete: true
(Assignee)

Comment 20

a year ago
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
Flags: needinfo?(enndeakin)
No, it's left over from an earlier version of the patch.
Flags: needinfo?(enndeakin)
(Assignee)

Comment 22

a year ago
Created attachment 8749715 [details] [diff] [review]
Patch: Rebase of Neil's patch to add popuppositioned event

Removed unused variable
Attachment #8749705 - Attachment is obsolete: true
(Assignee)

Comment 23

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c2fd8faf430
Created attachment 8750293 [details] [diff] [review]
Some possible test fixes

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.
(Assignee)

Comment 25

a year ago
Created attachment 8751470 [details] [diff] [review]
Patch: Rebase of Neil's patch to add popuppositioned event

Minor rebase to get the patch to apply on mozilla-central
Attachment #8749715 - Attachment is obsolete: true
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
(Assignee)

Comment 27

a year ago
(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?
Flags: needinfo?(enndeakin)
(Assignee)

Comment 28

a year ago
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.
Flags: needinfo?(enndeakin)
Whiteboard: [fce-active]
(Assignee)

Comment 30

a year ago
Created attachment 8761605 [details] [diff] [review]
jetpack_test_fix.diff

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?
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 31

a year ago
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!
(Assignee)

Comment 32

a year ago
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.
(Assignee)

Comment 33

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7248e0c7de7
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.
Flags: needinfo?(lgreco)
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(gijskruitbosch+bugs)

Comment 35

a year ago
(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.)
(Assignee)

Comment 36

a year ago
(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.
(Assignee)

Comment 37

a year ago
(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'?

Comment 38

11 months ago
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).

Comment 39

11 months ago
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?
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(lgreco)

Comment 42

11 months ago
 (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.
(Assignee)

Comment 43

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=80cd14bafa38
(Assignee)

Comment 44

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f74d9f5e6c0
(Assignee)

Comment 45

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f767d941405
(Assignee)

Comment 46

11 months ago
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?
Flags: needinfo?(lgreco)

Comment 47

11 months ago
Yes, Gabor is reviewing most of my SDK patches and I think he could be a good reviewer for this one too.
Flags: needinfo?(lgreco)
(Assignee)

Comment 48

11 months ago
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
Attachment #8761605 - Attachment is obsolete: true
Attachment #8764016 - Flags: review?(gkrizsanits)
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.
Attachment #8764016 - Flags: review?(gkrizsanits)
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?
(Assignee)

Comment 52

11 months ago
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
Flags: needinfo?(gkrizsanits)
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.
Attachment #8764016 - Flags: review+
Flags: needinfo?(gkrizsanits)
(Assignee)

Comment 54

11 months ago
(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.
(Assignee)

Comment 55

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=909e3cc73835
(Assignee)

Comment 56

11 months ago
Created attachment 8765572 [details] [diff] [review]
jetpack_test_fix.patch

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
Attachment #8764016 - Attachment is obsolete: true
Attachment #8765572 - Flags: review?(gkrizsanits)
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!
Attachment #8765572 - Flags: review?(gkrizsanits) → review+
(Assignee)

Comment 58

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ddbc087657d
(Assignee)

Comment 59

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d74c393e1b8d
(Assignee)

Comment 60

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb1897bf2996
(Assignee)

Comment 61

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=671d1037cb06
(Assignee)

Comment 62

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be9a1bfb90c2
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.
(Assignee)

Comment 64

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c908653b3299
Depends on: 1287388
(Assignee)

Comment 65

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6e40d9c3161
(Assignee)

Comment 66

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=170242bfa2dd
(Assignee)

Comment 67

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=55737b21bdf9
(Assignee)

Comment 68

10 months ago
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
Flags: needinfo?(jdescottes)
(Assignee)

Comment 69

10 months ago
Created attachment 8773422 [details] [diff] [review]
Patch: browser-chrome-testfixes

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
Attachment #8750293 - Attachment is obsolete: true
(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.
Flags: needinfo?(jdescottes)
(Assignee)

Comment 71

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2db186a57ea6
(Assignee)

Comment 72

10 months ago
Created attachment 8773472 [details] [diff] [review]
popuppositioned.patch

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
Attachment #8751470 - Attachment is obsolete: true
Attachment #8773422 - Attachment is obsolete: true
(Assignee)

Comment 73

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8f4d07a7655
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75a7f70feaa39d058f883b193b83a1b6a7bbb5c7
Should be green for devtools tests.
(Assignee)

Comment 75

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28fb600dd3af
(Assignee)

Comment 76

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=82113b844b84
(Assignee)

Comment 77

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bc8205736cc
(Assignee)

Comment 78

10 months ago
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?
Flags: needinfo?(jdescottes)
(Assignee)

Comment 79

10 months ago
Created attachment 8775318 [details] [diff] [review]
browser-chrome-testfixes.patch

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.
Attachment #8775318 - Flags: review?(enndeakin)
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?
(Assignee)

Comment 81

10 months ago
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.
Attachment #8775318 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 83

10 months ago
Created attachment 8777416 [details] [diff] [review]
browser-chrome-testfixes.patch

|waitForLocationChange| now tests for url equality rather than performing string analysis.

Carrying forward r+
Attachment #8775318 - Attachment is obsolete: true
Attachment #8777416 - Flags: review+
(Assignee)

Comment 84

10 months ago
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
Attachment #8773472 - Flags: review?(enndeakin)
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.
Attachment #8773472 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 86

10 months ago
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.
(Assignee)

Updated

10 months ago
Attachment #8773472 - Flags: review?(adw)

Updated

10 months ago
Attachment #8773472 - Flags: review?(adw) → review+
new patches for devtools tests : https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e1a350a84aac7e4ac4ada24e0374dfb492ebc73
Flags: needinfo?(jdescottes)
Depends on: 1293211
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8778875 - Attachment is obsolete: true
Attachment #8778875 - Flags: review?(bgrinstead)
Attachment #8778876 - Attachment is obsolete: true
Attachment #8778876 - Flags: review?(bgrinstead)
Attachment #8778877 - Attachment is obsolete: true
Attachment #8778877 - Flags: review?(bgrinstead)
Attachment #8778878 - Attachment is obsolete: true
Attachment #8778878 - Flags: review?(bgrinstead)
I see every patch here has an r+ and all the dependencies are resolved. Are these changes ready to land?
(Assignee)

Comment 94

9 months ago
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.
(Assignee)

Comment 96

9 months ago
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?
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 98

9 months ago
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
(Assignee)

Comment 100

9 months ago
Created attachment 8781742 [details] [diff] [review]
popuppositioned.patch

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
Attachment #8773472 - Attachment is obsolete: true
Attachment #8781742 - Flags: review?(enndeakin)
(Assignee)

Comment 101

9 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1da9888ca563
(Assignee)

Comment 102

9 months ago
Created attachment 8781753 [details] [diff] [review]
popuppositioned.patch

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
Attachment #8781742 - Attachment is obsolete: true
Attachment #8781742 - Flags: review?(enndeakin)
Attachment #8781753 - Flags: review?(enndeakin)
(Assignee)

Comment 103

9 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a836883f1f1
(Reporter)

Updated

9 months ago
Attachment #8781753 - Flags: review?(enndeakin) → review+
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.
(Assignee)

Updated

9 months ago
Keywords: checkin-needed

Comment 105

9 months ago
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
Keywords: checkin-needed
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.
Flags: needinfo?(ksteuber)
(Assignee)

Comment 107

9 months ago
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?
Flags: needinfo?(ksteuber)
(Assignee)

Updated

9 months ago
Flags: needinfo?(aryx.bugmail)
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
Flags: needinfo?(aryx.bugmail)
(Assignee)

Comment 109

9 months ago
Created attachment 8783097 [details] [diff] [review]
popuppositioned.patch

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.
Attachment #8781753 - Attachment is obsolete: true
Attachment #8783097 - Flags: review?(enndeakin)
(Assignee)

Comment 110

9 months ago
Created attachment 8783099 [details] [diff] [review]
browser-chrome-testfixes.patch

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
Attachment #8777416 - Attachment is obsolete: true
Attachment #8783099 - Flags: review?(enndeakin)
(Assignee)

Comment 111

9 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39b43f76f5dc
(Assignee)

Comment 112

9 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d66dbd7ba32c
(Assignee)

Comment 113

9 months ago
Created attachment 8783113 [details] [diff] [review]
popuppositioned.patch

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.
Attachment #8783097 - Attachment is obsolete: true
Attachment #8783097 - Flags: review?(enndeakin)
Attachment #8783113 - Flags: review?(enndeakin)
(Reporter)

Updated

9 months ago
Attachment #8783099 - Flags: review?(enndeakin) → review+
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.
Attachment #8783113 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 115

9 months ago
Created attachment 8784571 [details] [diff] [review]
popuppositioned.patch

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
Attachment #8783113 - Attachment is obsolete: true
Attachment #8784571 - Flags: review?(enndeakin)
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.
Attachment #8784571 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 117

9 months ago
Created attachment 8784875 [details] [diff] [review]
popuppositioned.patch

Reverted unnecessary Task.jsm changes as Neil suggested. Carrying forward r+.
Attachment #8784571 - Attachment is obsolete: true
Attachment #8784875 - Flags: review+
Is this now ready to land?
(Assignee)

Comment 119

9 months ago
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
(Assignee)

Comment 120

9 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=22c981a60e94
(Assignee)

Comment 121

9 months ago
Created attachment 8787286 [details] [diff] [review]
553455_fix.patch

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.
Attachment #8787286 - Flags: review?(dtownsend)
(Assignee)

Comment 122

9 months ago
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.
(Assignee)

Comment 123

9 months ago
Created attachment 8787313 [details] [diff] [review]
553455_fix.patch

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.
Attachment #8787286 - Attachment is obsolete: true
Attachment #8787286 - Flags: review?(dtownsend)
Attachment #8787313 - Flags: review?(dtownsend)
(Assignee)

Comment 124

9 months ago
Created attachment 8787326 [details] [diff] [review]
553455_fix.patch

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.
Attachment #8787313 - Attachment is obsolete: true
Attachment #8787313 - Flags: review?(dtownsend)
Attachment #8787326 - Flags: review?(dtownsend)
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
(Assignee)

Comment 126

9 months ago
Created attachment 8788616 [details] [diff] [review]
popuppositioned.patch

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.
Attachment #8784875 - Attachment is obsolete: true
Attachment #8788616 - Flags: review?(enndeakin)
(Assignee)

Comment 127

9 months ago
Created attachment 8788618 [details] [diff] [review]
553455_fix.patch

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.
Attachment #8787326 - Attachment is obsolete: true
Attachment #8787326 - Flags: review?(dtownsend)
Attachment #8788618 - Flags: review?(dtownsend)
(Assignee)

Comment 128

9 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=923cce62b42b
(Reporter)

Updated

9 months ago
Attachment #8788616 - Flags: review?(enndeakin) → review+

Updated

9 months ago
Attachment #8788618 - Flags: review?(dtownsend) → review?(rhelmer)
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.
Attachment #8788618 - Flags: review?(rhelmer) → review+
(Assignee)

Updated

9 months ago
Keywords: checkin-needed

Comment 130

8 months ago
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
Keywords: checkin-needed
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
Flags: needinfo?(ksteuber)
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
(Assignee)

Comment 133

8 months ago
I am currently working on something else, but I will fix this as soon as I can.
Flags: needinfo?(ksteuber)
(Assignee)

Comment 134

8 months ago
Created attachment 8791353 [details] [diff] [review]
browser-chrome-testfixes.patch

Removed changes to browser_filldoorhanger.js since that test was removed in Bug 1286718.

Carrying forward r+.
Attachment #8783099 - Attachment is obsolete: true
Attachment #8791353 - Flags: review+
(Assignee)

Comment 135

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4e120abdd36
(Assignee)

Comment 136

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63564d68d1d0
(Assignee)

Comment 137

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfc904512b92
(Assignee)

Comment 138

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=692d9345f826
(Assignee)

Comment 139

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cb0bb7740f4
(Assignee)

Comment 140

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f9dda502cb9
(Assignee)

Comment 141

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb649648a5d9
(Assignee)

Comment 142

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b17acfbd36fb
(Assignee)

Comment 143

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1642ef221268
(Assignee)

Comment 144

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db50429ee0b9
(Assignee)

Comment 145

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2421e90b432
(Assignee)

Comment 146

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a49ea2437a1f
(Assignee)

Comment 147

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61d335fc52a0
(Assignee)

Comment 148

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65c774660213
(Assignee)

Comment 149

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdacaeb107e1
(Assignee)

Comment 150

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c929fa4afe8
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.
(Assignee)

Comment 152

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eeb6031a4b86
(Assignee)

Comment 153

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4897c4f5f4a
(Assignee)

Comment 154

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e91657c39f97
(Assignee)

Comment 155

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=34f66beabc4a
(Assignee)

Comment 156

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffb442cdc2cd
(Assignee)

Comment 157

8 months ago
Created attachment 8794963 [details] [diff] [review]
browser_popupNotification_checkbox.patch

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.
Attachment #8794963 - Flags: review?(jhofmann)
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? :)
Flags: needinfo?(ksteuber)
(Assignee)

Comment 159

8 months ago
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);
Flags: needinfo?(ksteuber)
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)
(Assignee)

Comment 161

8 months ago
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!
(Assignee)

Comment 163

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2ca4da22439
(Assignee)

Comment 164

8 months ago
Created attachment 8795458 [details] [diff] [review]
browser_popupNotification_checkbox.patch
Attachment #8794963 - Attachment is obsolete: true
Attachment #8794963 - Flags: review?(jhofmann)
Attachment #8795458 - Flags: review?(jhofmann)
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
Attachment #8795458 - Flags: review?(jhofmann) → review-
(Assignee)

Comment 166

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cda9680887f3
(Assignee)

Comment 167

8 months ago
Created attachment 8795513 [details] [diff] [review]
browser_popupNotification_checkbox.patch
Attachment #8795458 - Attachment is obsolete: true
Attachment #8795513 - Flags: review?(jhofmann)
Comment on attachment 8795513 [details] [diff] [review]
browser_popupNotification_checkbox.patch

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

Looks good now, thanks!
Attachment #8795513 - Flags: review?(jhofmann) → review+
(Assignee)

Comment 169

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=adff557d8898
(Assignee)

Updated

8 months ago
Keywords: checkin-needed

Comment 170

8 months ago
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
Keywords: checkin-needed

Comment 171

8 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1306786

Updated

5 months ago
See Also: → bug 1171394

Comment 172

5 months ago
(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
See Also: → bug 1187647
Duplicate of this bug: 1290011

Updated

2 months ago
Depends on: 1349823
You need to log in before you can comment on or make changes to this bug.