Closed Bug 814889 Opened 10 years ago Closed 10 years ago

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

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 21

People

(Reporter: vporof, Assigned: paul)

References

Details

Attachments

(4 files)

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.
Blocks: 788977
Blocks: 816946
No longer blocks: 788977
Priority: -- → P1
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 ?
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.
Is this a toolbox regression? Did it use to work in the previous version of the inspector?
This is not a toolbox regression, I confirmed it by testing it on 17.0.1 just now.
Maybe we should un-pause the debugger when another tool is selected. Panos, what do you think?
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.
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.
Any reply to my previous comment ?
Flags: needinfo?(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?
Flags: needinfo?(paul) → needinfo?(past)
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.
Flags: needinfo?(past)
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.
Since this isn't a toolbox regression does it still need to block bug 816946 and be a P1?
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?
(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.
Assignee: nobody → paul
Attached patch patch v1Splinter Review
Attachment #698312 - Flags: review?(vporof)
Attachment #698312 - Attachment is patch: true
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!
Attachment #698312 - Flags: review?(vporof) → review+
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."
Attached patch patch v1.1Splinter Review
Addressed Victor's comments.
Blocks: 816953
Attached patch patch v1.2Splinter Review
Whiteboard: to land once try is green
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.
Attached patch v1.3Splinter Review
Addressed Panos' comments.

https://tbpl.mozilla.org/?tree=Try&rev=af0bc47592fd
sadly, this can't get into Aurora (string change).
Whiteboard: to land once try is green → [land-in-fx-team]
Duplicate of this bug: 827990
https://hg.mozilla.org/integration/fx-team/rev/abb9c29159f0
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/abb9c29159f0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.