950936 - Debugging auto-close XUL panels extremely painful

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

The basic problem is that as soon as someone clicks away from the panel (and into a debugging tool, like, say Firebug (or, once bug 777674 gets fixed, Firefox's own DOM inspector), the panel disappears (i.e. is no longer laid out).  So if one needs to work on cleaning up look-and-feel or debugging CSS rules, it's substantially harder than with (e.g.) the sidebar, which provides much worse user experience.

As a proof-of-concept, I've implemented a hacky add-on that, when installed, causes the panel to be pinned open until the developer clicks on the toolbar button that opened the panel.  It's working great for the moment, but has fragile dependencies on Firefox frontend details that are expected to break it soon. A real solution in the tree (even as simple as a hidden pref) would be wonderful.

Here's an email from :mixedpuppy (quoted with permission):

I wanted to open a short conversation about possible approaches to improving the debuggability of remote content inside panels.  Dan has brought up the issue they have with development on Talkilla using the panels in social api.  I believe jetpack panels have similar issues, and in my own experience I've often hacked panels to keep them open so I could more easily inspect into them.  Since, with Australis, I want to migrate the social panels fully over to the widget system, and IIUC jetpack has done that already, I'm including those I know who are familiar with the widget system as well.

The proposals I've heard from Dan and Florian is 1) using a pref that would set noautohide on the panel or 2) allow for a meta tag in the content to turn on noautohide.

I'd like to see if these approaches are acceptable or if anyone has a better approach.  Dan and Florian can probably fill in better on some of the development issues that Talkilla has run into with panels.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Code for the proof-of-concept debugging add-on is here:

https://github.com/mozilla/talkilla/tree/master/bin/PinPanel

If you want to try it out, save the following to disk and install from the gear menu in about:addons.

https://github.com/mozilla/talkilla/blob/master/bin/PinPanel.xpi

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I'm always a bit squeamish about content altering stuff in the chrome. I'm not really a security guy, but that sounds like an attack vector.

So to me, the best option proposed here is a pref - something like social.debug.pinPanels, and just have a pref watcher on that, and perform essentially what dmose's add-on is doing.

Just my 2c,

-Mike

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Sure, pref sounds fine to me.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Mike Conley (:mconley) from comment #2)
> I'm always a bit squeamish about content altering stuff in the chrome. I'm
> not really a security guy, but that sounds like an attack vector.
> 
> So to me, the best option proposed here is a pref - something like
> social.debug.pinPanels, and just have a pref watcher on that, and perform
> essentially what dmose's add-on is doing.
> 
> Just my 2c,
> 
> -Mike

@mconley, for australis the buttons are moving to using the widgets, which means that the australis widget code will need to support this as well as the social code.  Given that jetpack may also benefit from something like this, I think the pref should be more generic.  What are your thoughts on this from an australis perspective?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

TBH, the pinned-ness of the panel was never the hard part about debugging its layout. The layout part was. :)

I'd normally just set noautohide="true" with DOMi, or do it in the code if I had to restart a bunch of times. For widget panels that are generated on-the-fly, I just temporarily modified the code to make the panels noautohide.

The pref I suggested is, I guess, a shortcut for doing the same sort of thing.

I'm not really familiar with how the Social panels are being implemented - are they using PanelUI subviews? If so, we could have PanelUI read some pref and set the noautohide attribute...

I'd need to know more about Social API panels though before I can be confident that what I'm saying isn't nonsense.

-Mike

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to Mike Conley (:mconley) from comment #5)

> I'm not really familiar with how the Social panels are being implemented -
> are they using PanelUI subviews? If so, we could have PanelUI read some pref
> and set the noautohide attribute...

Right now they are a hybrid, using PanelUI when in the menu panel, but still using our own panel when in the toolbar.  This is because we use the same iframe across windows so moving 100% to PanelUI will take a little more work (but is something I want to get done in the near future).  When I do move to only using PanelUI, then we'll need to support this in the widget code.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

So I think this was resolved yesterday into the following plan:

a) The SocialAPI panel-handling code will observe a preference that will allow a developer to lock any SocialAPI notification widget panels open when that widget is in a toolbar. This will put current developers miles ahead.
b) We should see if perhaps there's something we can do in toolkit or Gecko to support such a preference for all panels. We would, however, need to solve the problem of finding a way to _close_ those panels in the event that they rely on outside clicks to close.

Comment 8

[:Gijs (he/him; back Aug 12)]

(In reply to Mike Conley (:mconley) from comment #7)
> So I think this was resolved yesterday into the following plan:
> 
> a) The SocialAPI panel-handling code will observe a preference that will
> allow a developer to lock any SocialAPI notification widget panels open when
> that widget is in a toolbar. This will put current developers miles ahead.
> b) We should see if perhaps there's something we can do in toolkit or Gecko
> to support such a preference for all panels. We would, however, need to
> solve the problem of finding a way to _close_ those panels in the event that
> they rely on outside clicks to close.

I wonder if we could add a pane to the browser toolbox for debugging options that browser/add-on/social provider devs want. That could also have the "close all those panels" button.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Mark, is this something you could take a look at relatively soon?

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

(I'm assuming this is shared code with the LoopAPI stuff, as that's where we really need it).

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Another very low-cost option is to execute in a "browser" scratchpad:

  document.getElementById("loop-panel").setAttribute("noautohide", "true")

which persists for the entire session.  Is there some reason this isn't enough?

Comment 12

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Mark Hammond [:markh] from comment #11)
> Another very low-cost option is to execute in a "browser" scratchpad:
> 
>   document.getElementById("loop-panel").setAttribute("noautohide", "true")
> 
> which persists for the entire session.  Is there some reason this isn't
> enough?

That depends - for us it's a perfectly fine hack but it feels a bit sketchy to expect 3rd party developers to run arcane commands like this to get a decent development env going. Do we have or expect to have a lot of 3rd party developers working on this stuff? If so, we should probably make it nicer.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

The issue is that it has to be entered every time the browser is restarted, and is fragile across DOM structure changes. 

So in the sense of "it's a lot better than nothing", it's good enough.

But in terms of offering a tight feedback loop without an unnecessary barrier after each browser restart, it's a real missed opportunity.  This is particularly true since, assuming it's shared across SocialAPI and LoopAPI, it's likely to be valuable to many (if not most) SocialAPI developers.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

I've no objection to any of the above (and see also bug 839959 for an even better solution).  But given the work-around exists and really isn't *too* painful, I don't think I can justify arguing this is a higher priority than the other stuff currently on my plate.

Comment 15

[Andrei Oprea [mention me on [e-mail]]]

FYI the snippet here isn't working for me anymore, I use:

document.getElementById("loop-notification-panel").setAttribute("noautohide", "true")

To stop the panel from hiding.

Comment 16

[Blake Winton (:bwinton) (:☕️)]

I wrote http://people.mozilla.org/~bwinton/temp/noautohide.patch when I was trying to style the bookmarks and history panels, and it might also be helpful here.

(If anyone takes it and adds stuff to it, please send the changes back to me so that we can all benefit!)

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Shell, debugging the panel has been an ongoing source of pain during the visual uplift.  How would we request that this be re-evaluated / re-triaged by the devtools team?

Comment 18

[:shell escalante]

looking at User Story comments from Dan and mixedpuppy - do you want me to set up a meeting with Florian, mixedpuppy, Dan and _________ (who is the engineering manager for the panel that we'd be requesting the work be prioritized), and anyone else... just so we can reach a resolution.

Comment 19

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I'll talk to Joe about this and see what's possible.

Comment 20

[Joe Walker [:jwalker]]

Patrick, there is some conceptual overlap with pseudo-class locks, although the implementation would be different.
What do you think?

Comment 21

[Joe Walker [:jwalker]]

I've been thinking and talking to Jeff, and I take back the pseudo-class lock idea. Things like :hover work for arbitrary elements, this doesn't.
How about a build flag?

Comment 22

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Dan: given that we should not ship a browser where it is possible to set these panels into 'noautohide' mode, what about this workflow:

* we create a patch similar to Blake's that is included when a build flag is enabled
* that patch implements the 'noautohide' attribute behaviour you want
* as you work, you add the 'noautohide' attribute to the panel you're working on, and remove it laer when you're done.

This approach mainly adds friction in two places: you need to set the build flag and you need to set the attribute on the panel you're working on. I suppose an additional component could be an add-on that just keeps a list of panel ids and sets noautohide to true on them dynamically.

(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #13)
> The issue is that it has to be entered every time the browser is restarted,
> and is fragile across DOM structure changes. 
> 
> So in the sense of "it's a lot better than nothing", it's good enough.
> 
> But in terms of offering a tight feedback loop without an unnecessary
> barrier after each browser restart, it's a real missed opportunity.  This is
> particularly true since, assuming it's shared across SocialAPI and LoopAPI,
> it's likely to be valuable to many (if not most) SocialAPI developers.

Again, how many developers are we talking about? Are they mostly in Mozilla, or will this be an increasing set of people outside of MoCo working on partner integrations like Pocket? Is it a key requirement that these developers not have to need to build Firefox from source?

Comment 23

[Blake Winton (:bwinton) (:☕️)]

The new WebExtensions API makes it really easy to open a panel from a toolbar button (it's a line in the manifest.json!), and doesn't expose the XUL necessary to add 'noautohide' to the panel, so I suspect we'll see more add-on authors wanting to debug panel layouts without knowing about the panel.  (This could be worked around with a checkbox in the add-on debugger version of the devtools, if that would be any easier…)

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Jeff and Joe, thanks for looking into this quickly!

(In reply to Jeff Griffiths (:canuckistani) from comment #22)
> Dan: given that we should not ship a browser where it is possible to set
> these panels into 'noautohide' mode, what about this workflow:
> 
> * we create a patch similar to Blake's that is included when a build flag is
> enabled
> * that patch implements the 'noautohide' attribute behaviour you want
> * as you work, you add the 'noautohide' attribute to the panel you're
> working on, and remove it laer when you're done.
> 
> This approach mainly adds friction in two places: you need to set the build
> flag and you need to set the attribute on the panel you're working on. I

What do you think of the possibility of having a (exposed via UI || exposed via about:config || hidden) pref that could be toggled dynamically at runtime?  That seems like would have both less friction than a build-flag, and would be useful to a larger group of people (including add-on and socialAPI provider authors).

> suppose an additional component could be an add-on that just keeps a list of
> panel ids and sets noautohide to true on them dynamically.

Oooh, good idea!

> (In reply to Dan Mosedale (:dmose) - use needinfo flag for response from
> comment #13)
> > The issue is that it has to be entered every time the browser is restarted,
> > and is fragile across DOM structure changes. 
> > 
> > So in the sense of "it's a lot better than nothing", it's good enough.
> > 
> > But in terms of offering a tight feedback loop without an unnecessary
> > barrier after each browser restart, it's a real missed opportunity.  This is
> > particularly true since, assuming it's shared across SocialAPI and LoopAPI,
> > it's likely to be valuable to many (if not most) SocialAPI developers.
> 
> Again, how many developers are we talking about? 

I don't know, but see below...

> Are they mostly in Mozilla, or will this be an increasing set of people outside of MoCo working on
> partner integrations like Pocket? 

As Blake points out, panels are likely to be used by both current and future add-on devs, many of whom (afaik) don't build the tree.  :mixedpuppy might know more about partner integrations, giving him a needinfo...

> Is it a key requirement that these developers not have to need to build Firefox from source?

I believe it is, particularly in light of the fact that it seems high value to remove barriers to development productivity for both add-ons and partner integrations.  Somewhat separably from that, one of our goals in moving Hello (but presumably eventually more bits of Firefox functionality) to a system add-on (a.k.a. Go-Faster) is that we'd like to lower the barrier to contribution as well, and one way that we want to that is making by making it easy to develop key pieces of functionality without ever having to build all of Firefox.

That said, any improvement to the current situation would be welcomed.

Comment 25

[Shane Caraveo (:mixedpuppy)]

-1 on build flags, too high a burden for anyone doing any kind of panel development.

lots of addons use panels, IMHO it would help those developers to have this functionality.

high likelihood of panels being used in various partner projects, whether through addons, builtin, etc., having the dev tools work better with panels would be a benefit to working with those.  cant put any numbers on that.

Comment 26

[:shell escalante]

Hi Jeff, based on the replies from Blake, Dan, Shane, and the go-faster work could this be prioritized and kindof highly in devtools?

They provided info to your questions in comment 22:
Again, how many developers are we talking about? Are they mostly in Mozilla, or will this be an increasing set of people outside of MoCo working on partner integrations like Pocket? Is it a key requirement that these developers not have to need to build Firefox from source?

*partners, community, and Mozilla should be an increasing set of people (comment 23, comment 24, comment 25)
*comment 25 on why not wanting a build flag solution

Comment 27

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Thanks - we're doing q4 planning right now and will put it on the list.

Comment 28

[:Gijs (he/him; back Aug 12)]

(In reply to Jeff Griffiths (:canuckistani) from comment #22)
> Dan: given that we should not ship a browser where it is possible to set
> these panels into 'noautohide' mode

I don't understand this. Why not? What's wrong with a preference that the user can toggle? The impact will be felt pretty instantaneously. I'm also happy to hide this pref in devtools until people tick "enable chrome and add-on debugging".

We let people do a lot more scary stuff after they tick that box by just typing/pasting random JS in the browser console.

Adding a pref and making the XUL panel code obey that pref and have it override the non-presence (ie imply the presence) of noautohide seems like the simplest solution to me.

Comment 29

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to :Gijs Kruitbosch from comment #28)
> (In reply to Jeff Griffiths (:canuckistani) from comment #22)
> > Dan: given that we should not ship a browser where it is possible to set
> > these panels into 'noautohide' mode
> 
> I don't understand this. Why not? What's wrong with a preference that the
> user can toggle? The impact will be felt pretty instantaneously. I'm also
> happy to hide this pref in devtools until people tick "enable chrome and
> add-on debugging".

The problem is that it becomes an annoyance to users - an addon can create panels that the user cannot dismiss. Requiring the user to toggle a devtools pref doesn't help here because the add-on can also toggle the pref without user interaction.

Time was, UX was opposed to this. Is that not the case still?

> We let people do a lot more scary stuff after they tick that box by just
> typing/pasting random JS in the browser console.
> 
> Adding a pref and making the XUL panel code obey that pref and have it
> override the non-presence (ie imply the presence) of noautohide seems like
> the simplest solution to me.

Well, okay - patches welcome, happy to have someone from devtools review devtools-related code. I'll stop trying to speak for the UX team, you should get UX review though.

Comment 30

[Mark Hammond [:markh] [:mhammond]]

(In reply to Jeff Griffiths (:canuckistani) from comment #29)
> The problem is that it becomes an annoyance to users - an addon can create
> panels that the user cannot dismiss.

As many comments in this bug have already said, you can currently execute, eg,

> document.getElementById("downloadsPanel").setAttribute("noautohide", "true");

And the specified panel doesn't auto hide. Unless we are discussing removing the noautohide attribute (which I haven't seen mentioned), then addons can already trivially cause panels to not auto-hide.  IIUC, a preference is being suggested as "sugar" around this capability and something that addons probably wouldn't bother using given the noautohide attribute exists - so I don't quite understand the pushback against a preference.

(TBH, given this is talking about using devtools to debug, and given devtools already has a console where that 1-liner can trivially be executed, my pushback against a preference is simply that it IMO it doesn't offer enough value compared to the additional code complexity in keeping the preference and the attribute value in-sync, but I already made that point in comment 11 ;)

Comment 31

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Mark Hammond [:markh] from comment #30)
...
> As many comments in this bug have already said, you can currently execute,
> eg,
> 
> > document.getElementById("downloadsPanel").setAttribute("noautohide", "true");

Sorry, I thought that didn't work, see: https://bugzilla.mozilla.org/show_bug.cgi?id=950936#c15

Blake linked to a patch that seems to implement notautohide in c++ code:

http://people.mozilla.org/~bwinton/temp/noautohide.patch

What's that for?

If all you need to do is enable 'noautohide' behaviour via the devtools chrome debugging pref as Gijs suggest, that sounds fine. Blake's patch implies a platform patch needs to be applied as well?

Can someone (Dan?) clarify what this proposal currently changes? I admit, I'm confused.

Comment 32

[:Gijs (he/him; back Aug 12)]

(In reply to Mark Hammond [:markh] from comment #30)
> (TBH, given this is talking about using devtools to debug, and given
> devtools already has a console where that 1-liner can trivially be executed,
> my pushback against a preference is simply that it IMO it doesn't offer
> enough value compared to the additional code complexity in keeping the
> preference and the attribute value in-sync, but I already made that point in
> comment 11 ;)

The problem is that in some cases the panels get created on the fly (e.g. if you move the devtools button to the navigation toolbar, that uses a temp panel created by PanelUI) and so a one-liner isn't possible in the same way.

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Sorry, today got crazy.  I'll try to write a coherent version of the current proposal shortly.  Leaving the needinfo set as a reminder.

Comment 34

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Jeff Griffiths (:canuckistani) from comment #31)
> 
> Blake linked to a patch that seems to implement notautohide in c++ code:
> 
> http://people.mozilla.org/~bwinton/temp/noautohide.patch
> 
> What's that for?

The first hunk of the patch isn't necessary.

The second hunk of the patch makes all menus and panels behave as though they had
noautohide set for easy debuggability, regardless of whether they actually do.

> If all you need to do is enable 'noautohide' behaviour via the devtools
> chrome debugging pref

I suspect we don't want to enable this mode just because chrome debugging is on, because that would make all the menus and panels actually behave differently from production for add-on, partner, and front-end developers pretty much all the time, which seems likely to foster confusion about how the interactions the devs are working on would feel to end users.

In other words, I think we would want a specific preference for this that could be toggled just when someone was trying to do this specific sort of debugging.

> as Gijs suggest, that sounds fine. 

What I think Gijs was pointing at in comment 28 (Gijs, correct me if I'm wrong), was that this preference is relevant if one is doing chrome/addon/social development, but not otherwise.  Exposing the preference in the UI all the time would lead to unnecessary cognitive load and confusion in the default case.  So it would make sense to display this preference in the UI (and, for that matter, honor it) only when the "enable chrome and addon" debugging preference was also set.
 
> implies a platform patch needs to be applied as well?

Well, setting the prefs would cause the bits of code in hunk 2 of that patch to be activated (i.e. the platform would need to check and observe the prefs and use them to decide when to do the forcing).

> Can someone (Dan?) clarify what this proposal currently changes? I admit,
> I'm confused.

If I'm understanding everything correctly, the proposal looks like this:

a) if/when the "enable chrome and addon debugging pref" is checked in the devtools, reveal ui to "force browser/chrome menus and panels to autohide"
b) the layout code would be modified to do that forcing if and only if both prefs are set to true

Comment 35

[:Gijs (he/him; back Aug 12)]

(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #34)
> > as Gijs suggest, that sounds fine. 
> 
> What I think Gijs was pointing at in comment 28 (Gijs, correct me if I'm
> wrong), was that this preference is relevant if one is doing
> chrome/addon/social development, but not otherwise.  Exposing the preference
> in the UI all the time would lead to unnecessary cognitive load and
> confusion in the default case.  So it would make sense to display this
> preference in the UI (and, for that matter, honor it) only when the "enable
> chrome and addon" debugging preference was also set.
>  
> > implies a platform patch needs to be applied as well?
> 
> Well, setting the prefs would cause the bits of code in hunk 2 of that patch
> to be activated (i.e. the platform would need to check and observe the prefs
> and use them to decide when to do the forcing).
> 
> > Can someone (Dan?) clarify what this proposal currently changes? I admit,
> > I'm confused.
> 
> If I'm understanding everything correctly, the proposal looks like this:
> 
> a) if/when the "enable chrome and addon debugging pref" is checked in the
> devtools, reveal ui to "force browser/chrome menus and panels to autohide"
> b) the layout code would be modified to do that forcing if and only if both
> prefs are set to true

Yes, this was what I meant. Sorry if I added to the confusion rather than reducing it. :-\

Comment 36

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #34)
> (In reply to Jeff Griffiths (:canuckistani) from comment #31)
...
> > If all you need to do is enable 'noautohide' behaviour via the devtools
> > chrome debugging pref
> 
> I suspect we don't want to enable this mode just because chrome debugging is
> on, because that would make all the menus and panels actually behave
> differently from production for add-on, partner, and front-end developers
> pretty much all the time, which seems likely to foster confusion about how
> the interactions the devs are working on would feel to end users.
> 
> In other words, I think we would want a specific preference for this that
> could be toggled just when someone was trying to do this specific sort of
> debugging.
...
> If I'm understanding everything correctly, the proposal looks like this:
> 
> a) if/when the "enable chrome and addon debugging pref" is checked in the
> devtools, reveal ui to "force browser/chrome menus and panels to autohide"
> b) the layout code would be modified to do that forcing if and only if both
> prefs are set to true

Thanks for the summary / clarification - this looks fine to me. What I'm unsure about is resourcing it from devtools - if you can get this done with review support from us that would be ideal.

Comment 38

[nico.schloemer]

This problem also crops up for extensions that have a popup window.

Comment 39

[Alexandre Poirot [:ochameau]]

This drives me nuts of not having that while working on bug 1221238, involving panels.
I'll try to do something.

Comment 40

[Alexandre Poirot [:ochameau]]

Attachment: platform patch v1

Introduce ui.menu.disable_autohide pref to ease debugging popups.
nsXULPopupManager is watching this pref.
If the pref is true when a popup opens, it automatically consider
the popup as having a noautohide attribute.
Then, when the pref toggles back to false,
I'm trying to closes all the noautohide popups.
It works, but I can see the state of some toolbarbutton stuck to enabled.
I have absolutely no idea what is going on?
May be it isn't not worth of watching the pref and hidding the popups?

Otherwise, I tried to write a test against noautohide attribute (there isn't any yet),
but I wasn't able to make a regular <panel>, without noautohide to close
without calling hidePopup. I tried:
 - calling panel.blur()
 -         anotherElement.focus()
 -         top level window's element.focus()
 - nsIDOMWindowUtils various mouse helpers
And nothing makes the panel disappear.

Comment 41

[Alexandre Poirot [:ochameau]]

Attachment: devtools patch v1

The associated devtools patch that adds a button in the browser toolbox
to automatically control this pref remotely.
(the browser toolbox lives in another process)
I stole the icon of the eyedropper, I need to craft a new icon for this...
I wish we had a menu with textual items in the toolbox instead of all these cryptic icons.

Otherwise I opened bug 1222047 in order to ease retrieving the Preference Actor.
It is quite silly to have to hack toolbox.js for that.

Comment 42

[Neil Deakin]

Comment on attachment 8683759 [details] [diff] [review]
platform patch v1

I don't think you want to abuse the noautohide is this way. This causes subtle behaviour changes to the popup (such as keyboard handling being different).

Better would be to return early in nsXULPopupManager::Rollup while the preference is set. I'm not sure you need to explicitly close the popups when the preference is cleared either. You could just leave them open and let the user close them.

I assume you want the popup to still close if closed in the normal way (pressing escape, press a close button, etc.) It would be good if you could list which situations you expect the popup to not close, and I could provide a more detailed answer.

Comment 43

[Alexandre Poirot [:ochameau]]

(In reply to Neil Deakin from comment #42)
> Better would be to return early in nsXULPopupManager::Rollup while the
> preference is set.

I gave that a try, with various variation:
- consuming / not consuming
- very early / just dismiss the HidePopup

And no matter what I do, clicks seems to all be captured by the popup.
So that I can't click on any other window other than the one that opened the popup.

> I'm not sure you need to explicitly close the popups when
> the preference is cleared either. You could just leave them open and let the
> user close them.

Yes, that would be better. In my original patch the issue is that the popup don't necessarely dismiss on blur as they do have noautohide and they may not be designed to be closed in any other way.
That's why I added this code. But if we can prevent blur-hiding without forcing the noautohide flag, that would be better.

> I assume you want the popup to still close if closed in the normal way
> (pressing escape, press a close button, etc.) It would be good if you could
> list which situations you expect the popup to not close, and I could provide
> a more detailed answer.

I think the most important (and only one?) hide call to block is the hide-on-blur.
The usecase is that you can't debug popups as you have to focus the debugger/inspector/console UI,
which closes the popup.
All other automatic closing aren't an issue. The developer can easily avoid pressing escape or clicking a close button.
The only other thing I can think of is if popup would also close if we do ALT+TAB, to switch to the debugger window? But I imagine that's just because of the blur, not because of the keystroke or window switching.

Comment 44

[Alexandre Poirot [:ochameau]]

Comment on attachment 8683759 [details] [diff] [review]
platform patch v1

See comment 43.

Comment 45

[Neil Deakin]

Sorry I was on vacation.

It works for me on Mac when I return false at the beginning of nsXULPopupManager:Rollup.

Are you on Linux? We may also need to call:
  mWidget->CaptureRollupEvents(nullptr, false);

to clear the mouse/key capture state as well.

Comment 46

[Alexandre Poirot [:ochameau]]

Attachment: platform patch v2

With the suggested call to CaptureRollupEvents.
Thanks Neil, it works great with that!

I think this patch is ready for review,
but I would really like to write a test for this.
This patch includes one, but doesn't work.
I wasn't able to close it without calling hidePopup,
which breaks the value of such test.

Comment 47

[Neil Deakin]

Comment on attachment 8693743 [details] [diff] [review]
platform patch v2

The code looks ok. You won't be able to test this that way so no point including the test. Either have no test or use nsIDOMWindowUtils::sendNativeMouseEvent to simulate native events on Windows and Mac (this isn't implemented on Linux so you won't be able to test at all there) 

One issue is that once the preference has been set and then unset, you will only be able to close menus via clicking outside them; the escape key will no longer work.

Comment 48

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5313de78205b

Comment 49

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19664fa7c2da

Comment 50

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1edd597c78e

Comment 51

[Alexandre Poirot [:ochameau]]

Helen, I'm about to introduce yet another button in the Browser toolbox toolbar.
In the mix of the existing:
- frame selection button,
- split console toggle
- settings button
- close button

In my current patch I'm just stealing an existing color picker icon, which is very sad :p

It would help me a ton if you could shed some light on this.
Is a new button the right choice?
What icon should I use?

This button is going to freeze all popups in Firefox once you click on it.
For example, if you open Firefox Menus like Tools menu, or if you open a popup from the toolbar like Hello, or Downloads... they will all stay alive, visible *and debuggable!*.
It allows to go back to the Browser toolbox and inspect and debug them.
Then, once you are done with your debugging session, you toggle this button again and get back to a regular popup behavior.

Comment 52

[Alexandre Poirot [:ochameau]]

Attachment: platform patch v3

I didn't managed to get the test to work on Windows,
but it was working fine on Mac.
I'm not sure there is enough value to keep the test.
So I pulled it out.

Comment 53

[Alexandre Poirot [:ochameau]]

Attachment: test for noautohide pref

The test, for posterity...

Comment 54

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Alexandre Poirot [:ochameau] from comment #51)
> Helen, I'm about to introduce yet another button in the Browser toolbox
> toolbar.
> In the mix of the existing:
> - frame selection button,
> - split console toggle
> - settings button
> - close button
> 
> In my current patch I'm just stealing an existing color picker icon, which
> is very sad :p
> 
> It would help me a ton if you could shed some light on this.
> Is a new button the right choice?
> What icon should I use?
> 
> This button is going to freeze all popups in Firefox once you click on it.
> For example, if you open Firefox Menus like Tools menu, or if you open a
> popup from the toolbar like Hello, or Downloads... they will all stay alive,
> visible *and debuggable!*.
> It allows to go back to the Browser toolbox and inspect and debug them.
> Then, once you are done with your debugging session, you toggle this button
> again and get back to a regular popup behavior.

bgrins proposed a design that I mocked up that will hopefully make our toolbar a little less insane, haha: https://invis.io/8U52CZVXY

I can design an icon for you. Of course, "they stay alive!" just reminds me of zombies... haha, I'll think of something!

Comment 55

[Alexandre Poirot [:ochameau]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #54)
> bgrins proposed a design that I mocked up that will hopefully make our
> toolbar a little less insane, haha: https://invis.io/8U52CZVXY

f+!
Yep, looks like it would help, especially that even if we come up with nice icon, it is often easier to understand via a label. It's even more true for the browser console, where options like this one can be very cryptic.

Comment 56

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: xul-panels.svg

I was very tempted to go with something ridiculous, but did not... :(

Since this feature is only really related to us working on the devtools themselves (no one should be debugging XUL panels in regular webdev, at least, I sincerely hope) I will request that turning this button on/off be a flag in about:config instead of in the regular settings, where it might be confusing ("What is this button? Why does it not do anything??")

Does that sound good to you Alex?

Comment 57

[Alexandre Poirot [:ochameau]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #56)
> Created attachment 8700606 [details]
> xul-panels.svg

Thanks!

> Since this feature is only really related to us working on the devtools
> themselves (no one should be debugging XUL panels in regular webdev, at
> least, I sincerely hope) I will request that turning this button on/off be a
> flag in about:config instead of in the regular settings, where it might be
> confusing ("What is this button? Why does it not do anything??")
> 
> Does that sound good to you Alex?

Actually, this button would only be displayed in the Browser Toolbox which is used by Firefox frontend developers (including us) as well as some add-ons developers.
I think we should always enable this feature there, otherwise I'm not sure anyone is going to discover it.

Comment 58

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #57)
> (In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #56)
> > Since this feature is only really related to us working on the devtools
> > themselves (no one should be debugging XUL panels in regular webdev, at
> > least, I sincerely hope) I will request that turning this button on/off be a
> > flag in about:config instead of in the regular settings, where it might be
> > confusing ("What is this button? Why does it not do anything??")
> > 
> > Does that sound good to you Alex?
> 
> Actually, this button would only be displayed in the Browser Toolbox which
> is used by Firefox frontend developers (including us) as well as some
> add-ons developers.
> I think we should always enable this feature there, otherwise I'm not sure
> anyone is going to discover it.

It's very hard to set prefs from the browser toolbox (since it uses a different profile and we don't ever open a normal window where you could navigate to about:config).  I've thought about this before for similar things - maybe we could have an option in the options panel but only have it be visible in the BT.  For your normal profile the only way to toggle it would be via a pref, but in the BT profile there'd be a checkbox in the options UI.  So basically two prefs - one to indicate whether the checkbox is visible and another to indicate whether the button is visible.

Comment 59

[Alexandre Poirot [:ochameau]]

Is there really a value in making this optional in the browser toolbox??

Comment 60

[Alexandre Poirot [:ochameau]]

I see much more value in making these toolbox actions scale and have text than making any of them optional (see comment 54).

Comment 61

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #59)
> Is there really a value in making this optional in the browser toolbox??

Being able to have less icons in the tab bar.  I don't see why this should be different from every other command button (all the others can be disabled from the options menu).

(In reply to Alexandre Poirot [:ochameau] from comment #60)
> I see much more value in making these toolbox actions scale and have text
> than making any of them optional (see comment 54).

The new UI for choosing them looks great.  Still, in that case we'd need to make this icon visible in that UI for the Browser Toolbox and not visible for the webconsole.  In that case, replace 'options panel' with 'the new command button editing UI' in Comment 58.

Comment 62

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Alexandre Poirot [:ochameau] from comment #59)
> Is there really a value in making this optional in the browser toolbox??

So, I think the value is that the browser toolbox will follow the same patterns/look the same as the regular toolbox (good UX). Also, Brian brought up some good points above. But I definitely take back the stuff about about:config, since having it scoped to the browser toolbox makes sense to me.

Comment 63

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9c73fd46c4e

Comment 64

[Alexandre Poirot [:ochameau]]

Attachment: devtools patch v2

Add what is required to make this button optional.
Fixed various broken tests due to this new special button.
Waiting for green try to proceed with review.

Comment 65

[Alexandre Poirot [:ochameau]]

I also integrated the new svg icon ;)

Comment 66

[Brian Grinstead [:bgrins]]

Comment on attachment 8703830 [details] [diff] [review]
devtools patch v2

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

Looks good, thanks.  I'd like to take one more look at the code and test using it before landing.

::: devtools/client/framework/toolbox-options.js
@@ +183,5 @@
>      for (let tool of toggleableButtons) {
>        if (this.toolbox.target.isMultiProcess && tool.id === "command-button-tilt") {
>          continue;
>        }
> +      if (tool.hideIfNotSupported &&

I can't think of a time when it makes sense to show a checkbox for a button that doesn't do anything on the current target.  We do that for scratchpad and it seems weird.  Not saying we need to remove that ability for Panels here, but we shouldn't allow it for command buttons.  So I'd suggest we get rid of hideIfNotSupported completely in this patch and just replace the condition with:

if (!tool.isTargetSupported(this.toolbox.target)) {
  continue;
}

::: devtools/client/framework/toolbox.js
@@ +1042,5 @@
>      });
>  
> +    let noautohideMenu = this.doc.getElementById("command-button-noautohide");
> +    if (!noautohideMenu.hasAttribute("hidden")) {
> +      this._updateNoautohideButton();

We already bail out early in _updateNoautohideButton if !this.target.root and then get a reference to command-button-noautohide later on in that function.  Could you also early return if the button is hidden there and then unconditionally call _updateNoautohideButton here?

@@ +1547,5 @@
>      this._host.setTitle(title);
>    },
>  
> +  // Returns a memoized instance of the preference actor
> +  get _preferenceFront() {

Nit - this is easier to follow for me as:

  get _preferenceFront() {
    return this.target.root.then(rootForm => {
     return new getPreferenceFront(this.target.client, rootForm);
    });
  },

or

  getPreferenceFront: Task.async(function*() {
    let rootForm = yield this.target.root;
    return new getPreferenceFront(this.target.client, rootForm);
  }),

I don't see much reason to cache the target.root promise.

@@ +1558,5 @@
> +      });
> +  },
> +
> +  _toggleAutohide: Task.async(function*() {
> +    let prefName = "ui.menu.disable_autohide";

Should this pref be named ui.popup.disable_autohide?  Looks like it affects more than just menus

::: devtools/client/locales/en-US/toolbox.dtd
@@ +41,5 @@
>    -  It allows you to switch the context of the whole toolbox. -->
>  <!ENTITY toolboxFramesTooltip          "Select an iframe as the currently targeted document">
>  
> +<!-- LOCALIZATION NOTE (toolboxNoAutoHideButton): This is the label for
> +  -  the button to force the popups/panels to stay visible on blue.

'on blue' -> 'when selected'?

Comment 67

[Brian Grinstead [:bgrins]]

(In reply to Neil Deakin from comment #47)
> One issue is that once the preference has been set and then unset, you will
> only be able to close menus via clicking outside them; the escape key will
> no longer work.

So this is an alright behavior change for us to introduce?  It would be confusing if the escape key didn't close a popup, but it's sort of edge-casey if it requires both setting and unsetting the pref.

Comment 68

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79f685338cae

Comment 69

[Alexandre Poirot [:ochameau]]

Attachment: platform patch v4

Neil, Are you ok naming the pref ui.popup.disable_autohide?

Comment 70

[Alexandre Poirot [:ochameau]]

Attachment: devtools patch interdiff

Comment 71

[Alexandre Poirot [:ochameau]]

Attachment: devtools patch v3

(In reply to Brian Grinstead [:bgrins] from comment #66)

See the interdiff in attachment 8707000 [details] [diff] [review].

Addressed all the comments but this one:

> ::: devtools/client/locales/en-US/toolbox.dtd
> @@ +41,5 @@
> >    -  It allows you to switch the context of the whole toolbox. -->
> >  <!ENTITY toolboxFramesTooltip          "Select an iframe as the currently targeted document">
> >  
> > +<!-- LOCALIZATION NOTE (toolboxNoAutoHideButton): This is the label for
> > +  -  the button to force the popups/panels to stay visible on blue.
> 
> 'on blue' -> 'when selected'?

I meant 'on blur'. Is that clear? Or do you prefer 'when selected'?

Comment 72

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #71)
> Created attachment 8707005 [details] [diff] [review]
> devtools patch v3
> 
> (In reply to Brian Grinstead [:bgrins] from comment #66)
> 
> See the interdiff in attachment 8707000 [details] [diff] [review].
> 
> Addressed all the comments but this one:
> 
> > ::: devtools/client/locales/en-US/toolbox.dtd
> > @@ +41,5 @@
> > >    -  It allows you to switch the context of the whole toolbox. -->
> > >  <!ENTITY toolboxFramesTooltip          "Select an iframe as the currently targeted document">
> > >  
> > > +<!-- LOCALIZATION NOTE (toolboxNoAutoHideButton): This is the label for
> > > +  -  the button to force the popups/panels to stay visible on blue.
> > 
> > 'on blue' -> 'when selected'?
> 
> I meant 'on blur'. Is that clear? Or do you prefer 'when selected'?

Ah, 'on blur' makes more sense

Comment 73

[Brian Grinstead [:bgrins]]

Comment on attachment 8707005 [details] [diff] [review]
devtools patch v3

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

::: devtools/client/framework/toolbox.js
@@ +123,5 @@
>    this._toolRegistered = this._toolRegistered.bind(this);
>    this._toolUnregistered = this._toolUnregistered.bind(this);
>    this._refreshHostTitle = this._refreshHostTitle.bind(this);
> +  this._toggleAutohide = this._toggleAutohide.bind(this);
> +  this._updateNoautohideButton = this._updateNoautohideButton.bind(this);

This function doesn't need to be bound to `this` since it's never attached as an event handler

@@ +1049,5 @@
>          }
>        }
>      });
>  
> +    let noautohideMenu = this.doc.getElementById("command-button-noautohide");

This variable is unused, can be removed

Comment 74

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c62f90cd39c

Comment 75

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/7c8fa9fad7b53245ec3703b96a46e3626d3d3cdb
Bug 950936 - Introduce ui.popup.disable_autohide pref to ease debugging popups. r=neil

https://hg.mozilla.org/integration/fx-team/rev/e78d14bd29b79f4059e893b3f215630df33aa7f8
Bug 950936 - Add disable autohide button in browser toolbox to help debug panel/popups. r=bgrins

Comment 76

[Carsten Book [:Tomcat]]

sorry had to back this out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=6664410&repo=fx-team

Comment 77

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/7567c6dfdee2
https://hg.mozilla.org/integration/fx-team/rev/a425df1225dc

Comment 78

[Alexandre Poirot [:ochameau]]

Weird. There is an exception during this test:

19:41:27     INFO -  console.error:
19:41:27     INFO -    Message: Error: Connection closed, pending request to server1.conn15.child1/domstylerule41, type setRuleText failed

But it also happens without this patch.
Could it be related to toolbox.js modification??

Comment 79

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75ddfc67c716

Comment 80

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd958b2b9b37

Comment 81

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26e0a19fca21

Comment 82

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecbbf4aad2d3

Comment 83

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1f69ec85478

Comment 84

[Alexandre Poirot [:ochameau]]

Attachment: fix the leak

Moving the if(this.target.root) allows to not do the request which appear
to be stalling when we open manyyyy toolbox, like the webconsole test.
With this patch we just don't do this request at all and everything is better :)
(See target's root getter in previous patch, it do some rdp requests)

With this patch I have a green try and should be able to finally close this bug!!

Comment 85

[Brian Grinstead [:bgrins]]

Comment on attachment 8712809 [details] [diff] [review]
fix the leak

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

Looks fine to me, please fold into the original patch before landing

::: devtools/client/framework/toolbox.js
@@ +1571,5 @@
>      let menu = this.doc.getElementById("command-button-noautohide");
>      if (menu.getAttribute("hidden") === "true") {
>        return;
>      }
> +    if (!this.target.root) {

Is this change necessary?  Both are early returning, seems like this shouldn't make a difference.

Comment 86

[Alexandre Poirot [:ochameau]]

Comment on attachment 8712809 [details] [diff] [review]
fix the leak

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

::: devtools/client/framework/toolbox.js
@@ +1571,5 @@
>      let menu = this.doc.getElementById("command-button-noautohide");
>      if (menu.getAttribute("hidden") === "true") {
>        return;
>      }
> +    if (!this.target.root) {

Yes. Actually, that's the main fix. It allows to not query `root` at all in common toolbox usage. And prevent any related request.
We will only do some request when the button is visible, i.e. when using the browser toolbox.

Comment 87

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/f5bd302550b101d5beba853e596c4d7604d4aa48
Bug 950936 - Introduce ui.popup.disable_autohide pref to ease debugging popups. r=neil

https://hg.mozilla.org/integration/fx-team/rev/7fde194b2683352adb50f09859b2c7d1fb98d05b
Bug 950936 - Add disable autohide button in browser toolbox to help debug panel/popups. r=bgrins

Comment 88

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f5bd302550b1
https://hg.mozilla.org/mozilla-central/rev/7fde194b2683

Comment 89

[Alexandre Poirot [:ochameau]]

I think addons developers are going to like this features when using SDK panels, CustomizableUI.jsm or WebExtensions popups.

Comment 90

[Dan Mosedale (:dmosedale, :dmose)]

Totally; thanks for doing this, Alexendre!

Comment 91

[Will Bamberg [:wbamberg]]

Docs for the Browser Toolbox:
https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox#Debugging_popups

Docs for add-ons:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Debugging#Debugging_popups

Please let me know if this covers it.

Comment 92

[Brian Grinstead [:bgrins]]

(In reply to Will Bamberg [:wbamberg] from comment #91)
> Docs for the Browser Toolbox:
> https://developer.mozilla.org/en-US/docs/Tools/
> Browser_Toolbox#Debugging_popups

Please include a small note about remembering to disable the button when closing the browser toolbox, at least until we resolve Bug 1251658, just in case someone else bumps into that.  Also might be worth mentioning that the pref 'ui.popup.disable_autohide' can be set (or unset) manually, which is basically what the button is doing.

> Docs for add-ons:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/
> Debugging#Debugging_popups
> 
> Please let me know if this covers it.

Great, thanks!  Should we also link back to the first doc page from the second? i.e. 'You can use the same technique to debug popups [in browser chrome](URL).'

Comment 93

[Will Bamberg [:wbamberg]]

Thanks Brian.

(In reply to Brian Grinstead [:bgrins] from comment #92)
> (In reply to Will Bamberg [:wbamberg] from comment #91)
> > Docs for the Browser Toolbox:
> > https://developer.mozilla.org/en-US/docs/Tools/
> > Browser_Toolbox#Debugging_popups
> 
> Please include a small note about remembering to disable the button when
> closing the browser toolbox, at least until we resolve Bug 1251658, just in
> case someone else bumps into that.  Also might be worth mentioning that the
> pref 'ui.popup.disable_autohide' can be set (or unset) manually, which is
> basically what the button is doing.

I've added this.

> 
> > Docs for add-ons:
> > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/
> > Debugging#Debugging_popups
> > 
> > Please let me know if this covers it.
> 
> Great, thanks!  Should we also link back to the first doc page from the
> second? i.e. 'You can use the same technique to debug popups [in browser
> chrome](URL).'

I didn't add this, just because I don't expect many add-on developers to be debugging the browser, and most of those who are would probably know this anyway. It's also easy to get to the Browser Toolbox page from this page already.

(Similarly, we don't say that you can debug chrome JS using the same techniques as https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Debugging#Debugging_JavaScript).

So it seems like adding this would introduce noise for most people, and benefit very few.

But please let me know if you feel strongly about it.