814889 - [toolbox] Highlighter can't inspect while the debugger is paused

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Victor Porof [:vporof][:vp]]

STR:

1. Go to http://htmlpad.org/debugger/
2. Open debugger
3. Switch to inspector (and lock onto any node, doesn't matter)
4. Click me
5. Try to highlight another node

Locking onto a different node doesn't work, highlighter is stuck. However, the markup panel works properly.

Comment 1

[Girish Sharma [:Optimizer]]

I think this is because in paused state, debugger stops any mouse event. Inspector listens to mouse click to select a new target. This is by design, but annoying. Should there be a white listing kind of thing such that debugger allows inspector to act properly ?

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

That's just not possible. The debugger calls nsIDOMWindowUtils.suppressEventHandling which ignores all content events. If the Inspector depends on those events, it won't be able to function properly. One thing we could do is emit a toolbox notification that other tools can listen for and display a "execution is paused" notification or similar.

Comment 3

[Paul Rouget [:paul]]

Is this a toolbox regression? Did it use to work in the previous version of the inspector?

Comment 4

[Girish Sharma [:Optimizer]]

This is not a toolbox regression, I confirmed it by testing it on 17.0.1 just now.

Comment 5

[Paul Rouget [:paul]]

Maybe we should un-pause the debugger when another tool is selected. Panos, what do you think?

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

We can't do that for the web console for instance, since people have already requested the ability to use the console when the debugger is paused. If we do it just for the other tools, what if the user clicks on the inspector tab by mistake? No other browser does this AFAIK and it seems like a pretty confusing behavior.

Comment 7

[Girish Sharma [:Optimizer]]

Chrome also does not allow this, i.e. to inspect while debugger is paused. What should be done to be consistent would be to display a warning that "Debugger is paused" as suggested by Panos in comment #2. Chrome also does that and dims the page, indicating that you cannot inspect.

Comment 8

[Girish Sharma [:Optimizer]]

Any reply to my previous comment ?

Comment 9

[Paul Rouget [:paul]]

(In reply to Girish Sharma [:Optimizer] from comment #8)
> Any reply to my previous comment ?

We could indeed do like Chrome: when the debugger is paused and notices a new tool is selected (there's an event for that), it could show the notification ("warning, debugger is paused, tool might miss-behave, do you want to resume?") or something like that. Maybe the debugger can prevent the other tool to be selected as well (we will need a "will-select" cancellable event then).

Panos, what do you think? We do that?

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Prompting the user to resume could work for the scenario when the user switches from the debugger to the inspector, but it would also unnecessarily annoy the users who would switch from the debugger to the web console, or any other tool that has no dependency on content events.

Preventing any other tools to be selected when execution is paused would be akin to throwing out the baby with the bath water: AFAIK all tools can function even when the page is in a paused state, including the inspector if one only selects nodes through the breadcrumbs or the markup view.

Note that what Chrome does is display a veil on top of the content with a notification message and just disable the highlighter. Since it looks like the highlighter is the only component that is affected, we could either do what chrome does, or just display a warning when the user switches to the inspector that highlighting would require a resumption of execution.

Comment 11

[Girish Sharma [:Optimizer]]

A Notification from the top (just like the one for "changes will be lost") saying something on the terms of "Execution is paused in debugger, thus disabling Node selection via Highlighter" along with 2 buttons , one to resume execution, other to dismiss the alert.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Since this isn't a toolbox regression does it still need to block bug 816946 and be a P1?

Comment 13

[Rob Campbell [:rc] (:robcee)]

I believe the only thing that won't work properly in the inspector is the highlighter because it needs to handle mouseover events from content to get the currently hovered node. Otherwise, selection in the markup panel and changes to the style panel should work.

Maybe just disable entering live inspection mode with the notification that we're paused?

Comment 14

[Paul Rouget [:paul]]

(In reply to Panos Astithas [:past] from comment #12)
> Since this isn't a toolbox regression does it still need to block bug 816946
> and be a P1?

Yes. The toolbox makes this scenario much more probable to happen than before.

(In reply to Rob Campbell [:rc] (:robcee) from comment #13)
> Maybe just disable entering live inspection mode with the notification that
> we're paused?

I'll try that.

Comment 15

[Paul Rouget [:paul]]

Attachment: patch v1

Comment 16

[Victor Porof [:vporof][:vp]]

Comment on attachment 698312 [details] [diff] [review]
patch v1

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

This is pretty cool. I really like how the notification works, it helps tremendously here.

I would like the test to also verify that the notification goes away when switching away from the inspector, not just when resuming. r+ with that and tweaking the warning message a bit.

::: browser/devtools/debugger/debugger-controller.js
@@ +397,5 @@
>    _update: function TS__update(aEvent) {
>      DebuggerView.Toolbar.toggleResumeButtonState(this.activeThread.state);
> +
> +    if (aEvent == "paused" || aEvent == "resumed") {
> +      DebuggerController._target.emit("script-" + aEvent);

_target doesn't always exist, in a chrome debugger for example.

Nit: instead of "script-" (and isScript* in Target.jsm), maybe "thread-" and isThread* is more accurate.

::: browser/devtools/debugger/test/browser_dbg_pause-warning.js
@@ +67,5 @@
> +  });
> +
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> +    gDebugger.document.getElementById("resume"),
> +    gDebugger);

Clicking a button that's not currently visible, since we're currently in the inspector :) Funny, but it does the job!

::: browser/devtools/framework/Target.jsm
@@ +250,5 @@
> +        break;
> +      case "script-paused":
> +        this._isScriptPaused = true;
> +        break;
> +      default:

Nit: useless default (for now).

@@ +384,5 @@
> +        break;
> +      case "script-paused":
> +        this._isScriptPaused = true;
> +        break;
> +      default:

Nit: useless default here as well.
I wonder if it'd be a good idea to reuse some of these methods across different types of Targets, with a generic prototype, some Object.create() magic etc. Maybe later.

@@ +449,5 @@
> +  _setupListeners: function() {
> +    this.destroy = this.destroy.bind(this);
> +    this.client.addListener("tabDetached", this.destroy);
> +
> +    this._onTabNavigated = function onRemoteTabNavigated(aType, aPacket) {

Moving this inside _setupListeners is gorgeous. However, why not go the extra mile and have the onRemoteTabNavigated function as a property on the prototype, since it's private anyway. This will avoid creating 2 functions each time when binding |this|, and it'll look a bit cleaner.

@@ +474,5 @@
> +        break;
> +      case "script-paused":
> +        this._isScriptPaused = true;
> +        break;
> +      default:

Here we go again!

::: browser/devtools/framework/Toolbox.jsm
@@ +423,5 @@
>      deck.selectedIndex = index;
>  
>      let definition = gDevTools.getToolDefinitions().get(id);
>  
> +    this._currentToolId = id;

Why move this assignment here? To make sure the id is available before a tool's iframe is loaded, so that, for example, the notification in InspectorPanel.jsm will be shown reliably? Or to avoid breaking the early return in TBOX_selectTool?

Either way, a short comment seems in order.

::: browser/devtools/inspector/InspectorPanel.jsm
@@ -40,5 @@
>    this.panelWin.inspector = this;
>  
> -  this.tabTarget = (this.target.tab != null);
> -  this.winTarget = (this.target.window != null);
> -

Yay!

@@ +101,5 @@
> +            "inspector-script-paused", "", notificationBox.PRIORITY_WARNING_HIGH);
> +        }
> +
> +        if (notification && this._toolbox.currentToolId != "inspector") {
> +          notificationBox.removeNotification(notification);

I know this is unlikely to break, but this needs to be tested.

::: browser/locales/en-US/chrome/browser/devtools/inspector.properties
@@ +23,5 @@
>  breadcrumbs.siblings=Siblings
>  
> +# LOCALIZATION NOTE (debuggerPausedWarning): Used in the Inspector tool, when
> +# the user switch to the inspector when the debugger is paused.
> +debuggerPausedWarning.message=Debugger is paused. Some feature like mouse selection might not work.

Couple of comments here:

"Debugger is paused on a breakpoint" sounds less scary tbh :)

Some feature*s*.

"Mouse selection" sounds confusing to me, maybe it'd be better to respect the inspect button tooltip text and say "selecting elements with the mouse". But that probably sounds even more confusing... What do you think? You can leave "mouse selection" if you think it sounds ok.

Also, "might" is enthusiastically over-selling it. Like 9.99 price tags. Let's be honest, it won't work!

Comment 17

[Paul Rouget [:paul]]

I addressed your comment (patch comming).

> ::: browser/devtools/framework/Toolbox.jsm
> @@ +423,5 @@
> >      deck.selectedIndex = index;
> >  
> >      let definition = gDevTools.getToolDefinitions().get(id);
> >  
> > +    this._currentToolId = id;
> 
> Why move this assignment here? To make sure the id is available before a
> tool's iframe is loaded, so that, for example, the notification in
> InspectorPanel.jsm will be shown reliably? Or to avoid breaking the early
> return in TBOX_selectTool?

That was a mistake in the first place. When we fire the "select" event, in the callbacks, this._currentToolId didn't have the right value.

> @@ +101,5 @@
> > +            "inspector-script-paused", "", notificationBox.PRIORITY_WARNING_HIGH);
> > +        }
> > +
> > +        if (notification && this._toolbox.currentToolId != "inspector") {
> > +          notificationBox.removeNotification(notification);
> 
> I know this is unlikely to break, but this needs to be tested.

It is now tested.

> ::: browser/locales/en-US/chrome/browser/devtools/inspector.properties
> @@ +23,5 @@
> >  breadcrumbs.siblings=Siblings
> >  
> > +# LOCALIZATION NOTE (debuggerPausedWarning): Used in the Inspector tool, when
> > +# the user switch to the inspector when the debugger is paused.
> > +debuggerPausedWarning.message=Debugger is paused. Some feature like mouse selection might not work.
> 
> Couple of comments here:
> 
> "Debugger is paused on a breakpoint" sounds less scary tbh :)
> 
> Some feature*s*.
> 
> "Mouse selection" sounds confusing to me, maybe it'd be better to respect
> the inspect button tooltip text and say "selecting elements with the mouse".
> But that probably sounds even more confusing... What do you think? You can
> leave "mouse selection" if you think it sounds ok.
> 
> Also, "might" is enthusiastically over-selling it. Like 9.99 price tags.
> Let's be honest, it won't work!

I went for: "Debugger is paused on a breakpoint. Some features like mouse selection will not work."

Comment 18

[Paul Rouget [:paul]]

Attachment: patch v1.1

Addressed Victor's comments.

Comment 19

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=4d24a91827fb

Comment 20

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=382e49c9911f

Comment 21

[Paul Rouget [:paul]]

Attachment: patch v1.2

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 698655 [details] [diff] [review]
patch v1.2

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

::: browser/devtools/markupview/test/browser_inspector_markup_edit.js
@@ +39,5 @@
>      aField.focus();
>      EventUtils.sendKey("return");
>      let input = inplaceEditor(aField).input;
>      input.value = aValue;
> +    EventUtils.sendKey("return");

In bug 817562 I discovered that I had to use EventUtils.sendKey(..., panelWin) to avoid failures when running tests with a detached toolbox.

::: browser/locales/en-US/chrome/browser/devtools/inspector.properties
@@ +23,5 @@
>  breadcrumbs.siblings=Siblings
>  
> +# LOCALIZATION NOTE (debuggerPausedWarning): Used in the Inspector tool, when
> +# the user switch to the inspector when the debugger is paused.
> +debuggerPausedWarning.message=Debugger is paused on a breakpoint. Some features like mouse selection will not work.

Note that it can also be paused due to a debugger statement, or, in the future, due to a watchpoint or after a Break On Next (bug 789430). Might be better to make the message more generic.

Comment 23

[Paul Rouget [:paul]]

Attachment: v1.3

Addressed Panos' comments.

https://tbpl.mozilla.org/?tree=Try&rev=af0bc47592fd

Comment 24

[Paul Rouget [:paul]]

sadly, this can't get into Aurora (string change).

Comment 26

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/abb9c29159f0

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

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