1251658 - Turn off the noautohide mode when the browser toolbox is closed, or when the browser is closed

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Patrick Brosset <:pbro>]

I have been using the noautohide feature in the browser toolbox recently, and it's very useful. But I've been bitten by the fact that it persists its state. And Gijs was talking about this on IRC today too.

Say you turn it ON, debug a thing, then forget about it, close the browser, come back the next day and open a popup. 
The popup will stay and you might not realize that that's because the noautohide feature remained ON.

The feature should be considered as a debugging helper that should have its lifetime synchronized to that of the debugger you're using when turning it ON (if that makes any sense).

- either it should turn OFF when you close the browser toolbox where you started it and remain OFF
- or it should turn OFF when you close the browser where you started it and remain OFF
- or it should turn OFF when close the browser toolbox but come back ON when that browser toolbox is opened again

Gijs said he just lost 5 minutes trying to understand why a popup wouldn't close. And I've seen this a few days ago too. I didn't loose time because I was lucky to understand quickly the issue, but I can certainly see people being confused by it especially if they toggled it ON by mistake.

Comment 1

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #0)
> The feature should be considered as a debugging helper that should have its
> lifetime synchronized to that of the debugger you're using when turning it
> ON (if that makes any sense).
> 
> - either it should turn OFF when you close the browser toolbox where you
> started it and remain OFF

I agree with this one - if it was enabled from the browser toolbox it should get reset when the browser toolbox is closed.  No need to keep state across browser toolbox loads to attempt to re-enable later

Comment 2

[Jarda Snajdr [:jsnajdr]]

I usually restart the browser many many times a day when working on something, and it's nice when the environment is stable across restarts and I don't waste time by setting things up on every restart.

Resetting a pref on toolbox close or on browser close breaks this workflow. I can't just call ./mach run --devtools --jsdebugger -P dev in a loop any more.

I know, hot reloading should help a lot, but it doesn't work for everything and also the browser toolbox is not always 100% stable and requires a restart once in a while.

Comment 3

[:Gijs (he/him)]

(In reply to Jarda Snajdr [:jsnajdr] from comment #2)
> I usually restart the browser many many times a day when working on
> something, and it's nice when the environment is stable across restarts and
> I don't waste time by setting things up on every restart.
> 
> Resetting a pref on toolbox close or on browser close breaks this workflow.
> I can't just call ./mach run --devtools --jsdebugger -P dev in a loop any
> more.
> 
> I know, hot reloading should help a lot, but it doesn't work for everything
> and also the browser toolbox is not always 100% stable and requires a
> restart once in a while.

The fact that context menus, panels, main menus and autocomplete boxes stick around permanently with the switch on doesn't bother you when debugging something completely unrelated? :-\

IME I need this switch very rarely - much more rarely than I need to run the browser, and having the switch persist *with no way to turn it off* when the browser toolbox is not helpful behaviour.

Perhaps the behaviour should be different depending on whether the browser is started with the browser toolbox enabled from the commandline ? That feels hacky, but it would make some amount of sense in that it controls whether or not you can switch this behaviour of...

Comment 4

[Jarda Snajdr [:jsnajdr]]

(In reply to :Gijs Kruitbosch from comment #3)
> The fact that context menus, panels, main menus and autocomplete boxes stick
> around permanently with the switch on doesn't bother you when debugging
> something completely unrelated? :-\

When I'm finished debugging the popup and want to move on to something else, I turn the switch off, of course. My point is that the settings should be persisted across restarts whenever possible.

> IME I need this switch very rarely - much more rarely than I need to run the
> browser, and having the switch persist *with no way to turn it off* when the
> browser toolbox is not helpful behaviour.

The switch can be disabled by setting a ui.popup.disable_autohide pref. The Browser Toolbox button is nothing but a shortcut for setting an about:config pref.

> Perhaps the behaviour should be different depending on whether the browser
> is started with the browser toolbox enabled from the commandline ? That
> feels hacky, but it would make some amount of sense in that it controls
> whether or not you can switch this behaviour of...

I found out there is already a "devtools.cache.disabled" pref that has a very nice behavior and I think it should be used for disabling popup autohide, too. Its label is "Disable Cache (when toolbox is open)". It doesn't disable the cache directly an unconditionally, like "ui.popup.disable_autohide" does for popups, but only disables it when the toolbox for the page is opened, and enables it back when it's closed.

Similarly, the "ui.popup.disable_autohide" should be activated only when the Browser Toolbox is open. You don't ever need it when the toolbox is closed, anyway - its only purpose is to make popups accessible to inspector, right?

Renaming the "ui.popup.disable_autohide" to something with a "devtools.*" prefix would be good, too - the option would be now directly related to devtools.

This achieves both goals:
- doesn't cause mysterious behavior when you're not actually debugging the browser
- persists across restarts and doesn't need to be enabled over and over

Comment 5

[:Gijs (he/him)]

(In reply to Jarda Snajdr [:jsnajdr] from comment #4)
> I found out there is already a "devtools.cache.disabled" pref that has a
> very nice behavior and I think it should be used for disabling popup
> autohide, too. Its label is "Disable Cache (when toolbox is open)". It
> doesn't disable the cache directly an unconditionally, like
> "ui.popup.disable_autohide" does for popups, but only disables it when the
> toolbox for the page is opened, and enables it back when it's closed.
> 
> Similarly, the "ui.popup.disable_autohide" should be activated only when the
> Browser Toolbox is open. You don't ever need it when the toolbox is closed,
> anyway - its only purpose is to make popups accessible to inspector, right?
> 
> Renaming the "ui.popup.disable_autohide" to something with a "devtools.*"
> prefix would be good, too - the option would be now directly related to
> devtools.
> 
> This achieves both goals:
> - doesn't cause mysterious behavior when you're not actually debugging the
> browser
> - persists across restarts and doesn't need to be enabled over and over

This would theoretically work, but I think the way that the noautohide stuff is implemented (directly in toolkit) this is actually really hard to do. The guts of toolkit/xul shouldn't change behaviour depending on whether or not devtools are attached - it shouldn't know anything about devtools.

Comment 7

[Patrick Brosset <:pbro>]

This feature design has been the cause of time lost for a bunch of people. Recently in bug 1393091. We should really change it.
Brian, would you, by any chance, have some time to look into this?

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: Bug 1251658 - Reset the ui.popup.disable_autohide pref when closing the Browser Toolbox;

But only reset if the pref has been toggled from the Browser Toolbox UI, to prevent
unexpected resetting if the pref was changed from about:config before opening the
Browser Toolbox.

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

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8904641 [details]
Bug 1251658 - Reset the ui.popup.disable_autohide pref when closing the Browser Toolbox;

https://reviewboard.mozilla.org/r/176454/#review181492

Thanks for looking into that!

Comment 10

[Brian Grinstead [:bgrins]]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5016d847744
Reset the ui.popup.disable_autohide pref when closing the Browser Toolbox;r=ochameau

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b5016d847744