Last Comment Bug 1206133 - Improve arrow panel position handling
: Improve arrow panel position handling
Status: RESOLVED FIXED
[fce-active]
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: mozilla52
Assigned To: Kirk Steuber [:bytesized]
:
: Neil Deakin
Mentors:
Depends on: 1287388 1293211 1306786
Blocks: 1188400 1197621 1203904 1207419 932317 1207536 1210328
  Show dependency treegraph
 
Reported: 2015-09-18 09:18 PDT by Neil Deakin
Modified: 2017-01-03 10:45 PST (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: 3
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Add popuppositioned event (19.01 KB, patch)
2015-09-18 09:18 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Add popuppositioned event, v2 (20.26 KB, patch)
2015-10-02 10:34 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Add popuppositioned event, v3 (26.69 KB, patch)
2015-12-02 04:45 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Add popuppositioned event, v4 (31.70 KB, patch)
2015-12-04 09:58 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Patch: Rebase of Neil's patch to add popuppositioned event (31.89 KB, patch)
2016-05-06 09:39 PDT, Kirk Steuber [:bytesized]
no flags Details | Diff | Splinter Review
Patch: Rebase of Neil's patch to add popuppositioned event (31.66 KB, patch)
2016-05-06 10:12 PDT, Kirk Steuber [:bytesized]
no flags Details | Diff | Splinter Review
Some possible test fixes (12.13 KB, patch)
2016-05-09 06:26 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Patch: Rebase of Neil's patch to add popuppositioned event (31.62 KB, patch)
2016-05-11 14:04 PDT, Kirk Steuber [:bytesized]
no flags Details | Diff | Splinter Review
jetpack_test_fix.diff (5.98 KB, patch)
2016-06-09 07:49 PDT, Kirk Steuber [:bytesized]
no flags Details | Diff | Splinter Review
jetpack_test_fix.patch (8.83 KB, patch)
2016-06-21 13:03 PDT, Kirk Steuber [:bytesized]
gkrizsanits: review+
Details | Diff | Splinter Review
jetpack_test_fix.patch (8.79 KB, patch)
2016-06-27 12:26 PDT, Kirk Steuber [:bytesized]
gkrizsanits: review+
Details | Diff | Splinter Review
Patch: browser-chrome-testfixes (12.89 KB, patch)
2016-07-21 12:17 PDT, Kirk Steuber [:bytesized]
no flags Details | Diff | Splinter Review
popuppositioned.patch (36.97 KB, patch)
2016-07-21 13:54 PDT, Kirk Steuber [:bytesized]
enndeakin: review+
adw: review+
Details | Diff | Splinter Review
browser-chrome-testfixes.patch (10.23 KB, patch)
2016-07-27 14:16 PDT, Kirk Steuber [:bytesized]
enndeakin: review+
Details | Diff | Splinter Review
browser-chrome-testfixes.patch (10.16 KB, patch)
2016-08-03 09:46 PDT, Kirk Steuber [:bytesized]
ksteuber: review+
Details | Diff | Splinter Review
Bug 1206133 - wait for xul wrapper popupshown event in HTMLTooltip; (58 bytes, text/x-review-board-request)
2016-08-08 06:02 PDT, Julian Descottes [:jdescottes]
no flags Details | Review
Bug 1206133 - fire ready event in SwatchBasedEditorTooltips after widget initialization; (58 bytes, text/x-review-board-request)
2016-08-08 06:02 PDT, Julian Descottes [:jdescottes]
no flags Details | Review
Bug 1206133 - wait for EditorTooltip ready event in tests; (58 bytes, text/x-review-board-request)
2016-08-08 06:02 PDT, Julian Descottes [:jdescottes]
no flags Details | Review
Bug 1206133 - hide all tooltips before tests ending; (58 bytes, text/x-review-board-request)
2016-08-08 06:02 PDT, Julian Descottes [:jdescottes]
no flags Details | Review
popuppositioned.patch (36.96 KB, patch)
2016-08-16 15:49 PDT, Kirk Steuber [:bytesized]
no flags Details | Diff | Splinter Review
popuppositioned.patch (36.96 KB, patch)
2016-08-16 16:20 PDT, Kirk Steuber [:bytesized]
enndeakin: review+
Details | Diff | Splinter Review
popuppositioned.patch (40.92 KB, patch)
2016-08-19 15:56 PDT, Kirk Steuber [:bytesized]
no flags Details | Diff | Splinter Review
browser-chrome-testfixes.patch (18.99 KB, patch)
2016-08-19 15:58 PDT, Kirk Steuber [:bytesized]
enndeakin: review+
Details | Diff | Splinter Review
popuppositioned.patch (40.86 KB, patch)
2016-08-19 16:26 PDT, Kirk Steuber [:bytesized]
enndeakin: review+
Details | Diff | Splinter Review
popuppositioned.patch (41.20 KB, patch)
2016-08-24 14:41 PDT, Kirk Steuber [:bytesized]
enndeakin: review+
Details | Diff | Splinter Review
popuppositioned.patch (39.24 KB, patch)
2016-08-25 09:12 PDT, Kirk Steuber [:bytesized]
ksteuber: review+
Details | Diff | Splinter Review
553455_fix.patch (83.33 KB, patch)
2016-09-01 09:52 PDT, Kirk Steuber [:bytesized]
no flags Details | Diff | Splinter Review
553455_fix.patch (83.36 KB, patch)
2016-09-01 11:02 PDT, Kirk Steuber [:bytesized]
no flags Details | Diff | Splinter Review
553455_fix.patch (83.33 KB, patch)
2016-09-01 11:35 PDT, Kirk Steuber [:bytesized]
no flags Details | Diff | Splinter Review
popuppositioned.patch (39.48 KB, patch)
2016-09-06 13:33 PDT, Kirk Steuber [:bytesized]
enndeakin: review+
Details | Diff | Splinter Review
553455_fix.patch (83.39 KB, patch)
2016-09-06 13:36 PDT, Kirk Steuber [:bytesized]
rhelmer: review+
Details | Diff | Splinter Review
browser-chrome-testfixes.patch (16.88 KB, patch)
2016-09-14 12:29 PDT, Kirk Steuber [:bytesized]
ksteuber: review+
Details | Diff | Splinter Review
browser_popupNotification_checkbox.patch (2.27 KB, patch)
2016-09-26 12:20 PDT, Kirk Steuber [:bytesized]
no flags Details | Diff | Splinter Review
browser_popupNotification_checkbox.patch (5.76 KB, patch)
2016-09-27 13:43 PDT, Kirk Steuber [:bytesized]
jhofmann: review-
Details | Diff | Splinter Review
browser_popupNotification_checkbox.patch (5.76 KB, patch)
2016-09-27 15:01 PDT, Kirk Steuber [:bytesized]
jhofmann: review+
Details | Diff | Splinter Review

Description User image Neil Deakin 2015-09-18 09:18:26 PDT
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 1 User image Neil Deakin 2015-09-18 09:23:36 PDT
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.
Comment 2 User image Matthew N. [:MattN] (PM if requests are blocking you) 2015-09-18 17:01:31 PDT
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.
Comment 3 User image Alex Gibson [:agibson] 2015-09-21 02:28:51 PDT
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!
Comment 4 User image Alex Gibson [:agibson] 2015-09-21 02:32:02 PDT
(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.
Comment 5 User image Neil Deakin 2015-09-21 05:35:34 PDT
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 6 User image Matthew N. [:MattN] (PM if requests are blocking you) 2015-09-25 00:03:48 PDT
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?
Comment 7 User image Alex Gibson [:agibson] 2015-09-25 01:27:21 PDT
(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)
Comment 8 User image Neil Deakin 2015-09-25 05:04:47 PDT
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.
Comment 9 User image Neil Deakin 2015-10-02 10:34:11 PDT
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
Comment 10 User image Neil Deakin 2015-12-02 04:45:02 PST
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.
Comment 11 User image Neil Deakin 2015-12-04 09:58:58 PST
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.
Comment 12 User image Jens 2016-04-08 05:34:06 PDT
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 ;)
Comment 13 User image Neil Deakin 2016-04-08 06:24:22 PDT
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 User image Jens 2016-04-08 06:53:11 PDT
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)
Comment 15 User image Neil Deakin 2016-04-08 07:03:40 PDT
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
Comment 16 User image Neil Deakin 2016-04-08 07:05:41 PDT
I set up a try build to get a more complete list:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73095c8fa7e1
Comment 19 User image Kirk Steuber [:bytesized] 2016-05-06 09:39:19 PDT
Created attachment 8749705 [details] [diff] [review]
Patch: Rebase of Neil's patch to add popuppositioned event
Comment 20 User image Kirk Steuber [:bytesized] 2016-05-06 09:49:17 PDT
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
Comment 21 User image Neil Deakin 2016-05-06 09:52:31 PDT
No, it's left over from an earlier version of the patch.
Comment 22 User image Kirk Steuber [:bytesized] 2016-05-06 10:12:53 PDT
Created attachment 8749715 [details] [diff] [review]
Patch: Rebase of Neil's patch to add popuppositioned event

Removed unused variable
Comment 24 User image Neil Deakin 2016-05-09 06:26:41 PDT
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.
Comment 25 User image Kirk Steuber [:bytesized] 2016-05-11 14:04:22 PDT
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
Comment 26 User image Kris Maglione [:kmag] 2016-05-18 10:40:21 PDT
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
Comment 27 User image Kirk Steuber [:bytesized] 2016-05-18 12:35:37 PDT
(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?
Comment 28 User image Kirk Steuber [:bytesized] 2016-05-18 12:45:04 PDT
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
Comment 29 User image Neil Deakin 2016-05-18 14:07:19 PDT
> I think so, but I am not sure. Neil?

I would think so, but you'll just have to try it to verify.
Comment 30 User image Kirk Steuber [:bytesized] 2016-06-09 07:49:05 PDT
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?
Comment 31 User image Kirk Steuber [:bytesized] 2016-06-10 01:39:30 PDT
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!
Comment 32 User image Kirk Steuber [:bytesized] 2016-06-10 01:51:02 PDT
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 34 User image Kris Maglione [:kmag] 2016-06-10 10:29:54 PDT
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.
Comment 35 User image :Gijs 2016-06-10 10:41:15 PDT
(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.)
Comment 36 User image Kirk Steuber [:bytesized] 2016-06-11 07:54:41 PDT
(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.
Comment 37 User image Kirk Steuber [:bytesized] 2016-06-11 07:59:36 PDT
(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 User image Luca Greco [:rpl] 2016-06-12 08:16:34 PDT
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 User image :Gijs 2016-06-13 02:10:35 PDT
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?
Comment 40 User image Neil Deakin 2016-06-13 02:47:59 PDT
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.
Comment 41 User image Kris Maglione [:kmag] 2016-06-14 02:17:27 PDT
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.
Comment 42 User image Luca Greco [:rpl] 2016-06-14 04:56:01 PDT
 (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.
Comment 46 User image Kirk Steuber [:bytesized] 2016-06-21 11:00:14 PDT
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?
Comment 47 User image Luca Greco [:rpl] 2016-06-21 11:09:55 PDT
Yes, Gabor is reviewing most of my SDK patches and I think he could be a good reviewer for this one too.
Comment 48 User image Kirk Steuber [:bytesized] 2016-06-21 13:03:36 PDT
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
Comment 49 User image Gabor Krizsanits [:krizsa :gabor] 2016-06-23 02:04:17 PDT
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.
Comment 50 User image Neil Deakin 2016-06-23 03:02:41 PDT
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.
Comment 51 User image Gabor Krizsanits [:krizsa :gabor] 2016-06-23 04:15:55 PDT
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?
Comment 52 User image Kirk Steuber [:bytesized] 2016-06-23 12:58:03 PDT
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 53 User image Gabor Krizsanits [:krizsa :gabor] 2016-06-24 02:27:55 PDT
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.
Comment 54 User image Kirk Steuber [:bytesized] 2016-06-24 08:40:03 PDT
(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.
Comment 56 User image Kirk Steuber [:bytesized] 2016-06-27 12:26:06 PDT
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
Comment 57 User image Gabor Krizsanits [:krizsa :gabor] 2016-06-28 03:18:01 PDT
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!
Comment 63 User image Julian Descottes [:jdescottes] 2016-07-11 10:53:14 PDT
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.
Comment 68 User image Kirk Steuber [:bytesized] 2016-07-21 12:12:13 PDT
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
Comment 69 User image Kirk Steuber [:bytesized] 2016-07-21 12:17:07 PDT
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
Comment 70 User image Julian Descottes [:jdescottes] 2016-07-21 12:31:09 PDT
(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.
Comment 72 User image Kirk Steuber [:bytesized] 2016-07-21 13:54:27 PDT
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
Comment 74 User image Julian Descottes [:jdescottes] 2016-07-22 15:05:30 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75a7f70feaa39d058f883b193b83a1b6a7bbb5c7
Should be green for devtools tests.
Comment 78 User image Kirk Steuber [:bytesized] 2016-07-27 10:05:50 PDT
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?
Comment 79 User image Kirk Steuber [:bytesized] 2016-07-27 14:16:38 PDT
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.
Comment 80 User image Neil Deakin 2016-07-28 18:00:06 PDT
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?
Comment 81 User image Kirk Steuber [:bytesized] 2016-08-02 11:18:34 PDT
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 82 User image Neil Deakin 2016-08-03 08:09:17 PDT
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.
Comment 83 User image Kirk Steuber [:bytesized] 2016-08-03 09:46:40 PDT
Created attachment 8777416 [details] [diff] [review]
browser-chrome-testfixes.patch

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

Carrying forward r+
Comment 84 User image Kirk Steuber [:bytesized] 2016-08-03 09:55:55 PDT
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 85 User image Neil Deakin 2016-08-03 10:20:30 PDT
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.
Comment 86 User image Kirk Steuber [:bytesized] 2016-08-03 10:25:37 PDT
I tried requesting review from him, but apparently he is not accepting review requests.
Comment 87 User image Panos Astithas [:past] 2016-08-05 04:45:42 PDT
Marco is on PTO for a couple of weeks, you could send them to Drew (:adw) instead.
Comment 88 User image Julian Descottes [:jdescottes] 2016-08-06 07:11:09 PDT
new patches for devtools tests : https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e1a350a84aac7e4ac4ada24e0374dfb492ebc73
Comment 89 User image Julian Descottes [:jdescottes] 2016-08-08 06:02:22 PDT Comment hidden (mozreview-request)
Comment 90 User image Julian Descottes [:jdescottes] 2016-08-08 06:02:22 PDT Comment hidden (mozreview-request)
Comment 91 User image Julian Descottes [:jdescottes] 2016-08-08 06:02:22 PDT Comment hidden (mozreview-request)
Comment 92 User image Julian Descottes [:jdescottes] 2016-08-08 06:02:22 PDT Comment hidden (mozreview-request)
Comment 93 User image Panos Astithas [:past] 2016-08-16 08:54:18 PDT
I see every patch here has an r+ and all the dependencies are resolved. Are these changes ready to land?
Comment 94 User image Kirk Steuber [:bytesized] 2016-08-16 11:11:35 PDT
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.
Comment 95 User image Matthew N. [:MattN] (PM if requests are blocking you) 2016-08-16 11:29:48 PDT
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.
Comment 96 User image Kirk Steuber [:bytesized] 2016-08-16 11:37:22 PDT
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?
Comment 97 User image Matthew N. [:MattN] (PM if requests are blocking you) 2016-08-16 11:43:47 PDT
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.
Comment 98 User image Kirk Steuber [:bytesized] 2016-08-16 12:44:09 PDT
Well that sounds great. How do I push to Elm?
Comment 99 User image Matthew N. [:MattN] (PM if requests are blocking you) 2016-08-16 13:56:56 PDT
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
Comment 100 User image Kirk Steuber [:bytesized] 2016-08-16 15:49:22 PDT
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
Comment 102 User image Kirk Steuber [:bytesized] 2016-08-16 16:20:07 PDT
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
Comment 104 User image Panos Astithas [:past] 2016-08-17 05:12:06 PDT
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.
Comment 105 User image Pulsebot 2016-08-17 12:45:07 PDT
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
Comment 106 User image Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2016-08-18 11:05:06 PDT
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.
Comment 107 User image Kirk Steuber [:bytesized] 2016-08-18 12:51:19 PDT
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?
Comment 108 User image Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2016-08-18 13:09:46 PDT
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
Comment 109 User image Kirk Steuber [:bytesized] 2016-08-19 15:56:00 PDT
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.
Comment 110 User image Kirk Steuber [:bytesized] 2016-08-19 15:58:16 PDT
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
Comment 113 User image Kirk Steuber [:bytesized] 2016-08-19 16:26:34 PDT
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.
Comment 114 User image Neil Deakin 2016-08-22 10:27:15 PDT
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.
Comment 115 User image Kirk Steuber [:bytesized] 2016-08-24 14:41:34 PDT
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
Comment 116 User image Neil Deakin 2016-08-25 07:40:56 PDT
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.
Comment 117 User image Kirk Steuber [:bytesized] 2016-08-25 09:12:44 PDT
Created attachment 8784875 [details] [diff] [review]
popuppositioned.patch

Reverted unnecessary Task.jsm changes as Neil suggested. Carrying forward r+.
Comment 118 User image Panos Astithas [:past] 2016-08-31 05:50:37 PDT
Is this now ready to land?
Comment 119 User image Kirk Steuber [:bytesized] 2016-08-31 09:44:19 PDT
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
Comment 121 User image Kirk Steuber [:bytesized] 2016-09-01 09:52:14 PDT
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.
Comment 122 User image Kirk Steuber [:bytesized] 2016-09-01 09:53:25 PDT
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.
Comment 123 User image Kirk Steuber [:bytesized] 2016-09-01 11:02:48 PDT
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.
Comment 124 User image Kirk Steuber [:bytesized] 2016-09-01 11:35:27 PDT
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.
Comment 125 User image Matthew N. [:MattN] (PM if requests are blocking you) 2016-09-01 15:15:44 PDT
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
Comment 126 User image Kirk Steuber [:bytesized] 2016-09-06 13:33:20 PDT
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.
Comment 127 User image Kirk Steuber [:bytesized] 2016-09-06 13:36:17 PDT
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.
Comment 129 User image Robert Helmer [:rhelmer] 2016-09-09 09:40:11 PDT
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.
Comment 130 User image Pulsebot 2016-09-10 12:55:35 PDT
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
Comment 131 User image Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2016-09-10 14:44:02 PDT
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
Comment 133 User image Kirk Steuber [:bytesized] 2016-09-13 09:28:51 PDT
I am currently working on something else, but I will fix this as soon as I can.
Comment 134 User image Kirk Steuber [:bytesized] 2016-09-14 12:29:35 PDT
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+.
Comment 151 User image Julian Descottes [:jdescottes] 2016-09-21 23:47:36 PDT
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.
Comment 157 User image Kirk Steuber [:bytesized] 2016-09-26 12:20:44 PDT
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.
Comment 158 User image Johann Hofmann [:johannh] 2016-09-27 04:40:09 PDT
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? :)
Comment 159 User image Kirk Steuber [:bytesized] 2016-09-27 10:24:09 PDT
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);
Comment 160 User image Johann Hofmann [:johannh] 2016-09-27 12:13:59 PDT
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)
Comment 161 User image Kirk Steuber [:bytesized] 2016-09-27 12:49:23 PDT
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.
Comment 162 User image Johann Hofmann [:johannh] 2016-09-27 13:28:05 PDT
(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 164 User image Kirk Steuber [:bytesized] 2016-09-27 13:43:45 PDT
Created attachment 8795458 [details] [diff] [review]
browser_popupNotification_checkbox.patch
Comment 165 User image Johann Hofmann [:johannh] 2016-09-27 14:13:54 PDT
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 167 User image Kirk Steuber [:bytesized] 2016-09-27 15:01:26 PDT
Created attachment 8795513 [details] [diff] [review]
browser_popupNotification_checkbox.patch
Comment 168 User image Johann Hofmann [:johannh] 2016-09-27 23:23:46 PDT
Comment on attachment 8795513 [details] [diff] [review]
browser_popupNotification_checkbox.patch

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

Looks good now, thanks!
Comment 170 User image Pulsebot 2016-09-29 16:59:41 PDT
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
Comment 172 User image arni2033 2017-01-03 10:45:02 PST
(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

Note You need to log in before you can comment on or make changes to this bug.