671689 - [highlighter] Nodes should be selectable from the HTML tree

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Paul Rouget [:paul]]

Attachment: patch v1

The current behavior is:

× If not inspecting (a node is already selected): clicking on a node in the HTML tree will highlight and lock the node in the web page and select the node in the HTML tree.

× If inspecting: clicking on a node in the HTML doesn't highlight the node in the web page, but select the node in the HTML tree.

This behavior is confusing.

I propose this behavior:

Regardless the highlighter status (inspecting or not), a click on a node in the HTML tree will lock and highlight this node. If the locked node is clicked in the HTML tree, the node is unlocked (start inspecting).

Comment 1

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

(In reply to comment #0)
> Created attachment 546032 [details] [diff] [review] [review]
> patch v1
> 
> The current behavior is:
> 
> × If not inspecting (a node is already selected): clicking on a node in the
> HTML tree will highlight and lock the node in the web page and select the
> node in the HTML tree.
> 
> × If inspecting: clicking on a node in the HTML doesn't highlight the node
> in the web page, but select the node in the HTML tree.
> 
> This behavior is confusing.
> 
> I propose this behavior:
> 
> Regardless the highlighter status (inspecting or not), a click on a node in
> the HTML tree will lock and highlight this node. If the locked node is
> clicked in the HTML tree, the node is unlocked (start inspecting).

I'm not sure we should unlock from the html panel as this could also be confusing to be suddenly thrown into dynamic highlighting. A user could be clicking in the panel to focus the html panel. Or they might be clicking to try to edit something.

The rest of the behavior you're proposing sounds fine.

Comment 2

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

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

+      if (!this.inspecting && node === this.selection) {
+        this.startInspecting();
+      } else {

I think we can just take this if/else clause out and do the select, highlightNode and stopInspecting calls directly.

Also, this needs a unittest.

Comment 3

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

(In reply to comment #2)
> Comment on attachment 546032 [details] [diff] [review] [review]
> patch v1
> 
> +      if (!this.inspecting && node === this.selection) {
> +        this.startInspecting();
> +      } else {
> 
> I think we can just take this if/else clause out and do the select,
> highlightNode and stopInspecting calls directly.
> 
> Also, this needs a unittest.

or just check if (this.inspecting) this.stopInspecting() and do the rest in either case.

Comment 4

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

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

r- because of lack of unittest and above behavior.

Comment 5

[Paul Rouget [:paul]]

Attachment: patch v1.1

Addressed Rob's comments (test and no toggle-highlight).

Comment 6

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

Comment on attachment 547707 [details] [diff] [review]
patch v1.1

+  gBrowser.selectedBrowser.addEventListener("load", function() {
+    gBrowser.selectedBrowser.removeEventListener("load",
+                                                 arguments.callee, true);
+    doc = content.document;

r+ with arguments.callee changed to a function name.

Comment 7

[Paul Rouget [:paul]]

Attachment: [in-fx-team] patch v1.2

Thanks for the r+ Rob.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 548112 [details] [diff] [review]
[in-fx-team] patch v1.2

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

>+        this.select(node, true, false);
>+        this.highlighter.highlightNode(node);
>+        this.stopInspecting(true);

Isn't the select() call here redundant, given that stopInspecting also calls select() on the highlighted node?

r=me with that addressed.

Comment 9

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

(In reply to comment #8)
> Comment on attachment 548112 [details] [diff] [review] [diff] [details] [review]
> patch v1.2
> 
> >diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js
> 
> >+        this.select(node, true, false);
> >+        this.highlighter.highlightNode(node);
> >+        this.stopInspecting(true);
> 
> Isn't the select() call here redundant, given that stopInspecting also calls
> select() on the highlighted node?

Not quite. stopInspecting() bails out early if !this.inspecting. In other words, it won't make it to the selection code unless we're highlighting dynamically.

I think I have a fix for this that will kill the redundancy.

> r=me with that addressed.

Comment 10

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

what about:

  onTreeClick: function IUI_onTreeClick(aEvent)
  {
    let node;
    let target = aEvent.target;
    let hitTwisty = false;
    if (this.hasClass(target, "twisty")) {
      node = this.getRepObject(aEvent.target.nextSibling);
      hitTwisty = true;
    } else {
      node = this.getRepObject(aEvent.target);
    }

    if (node) {
      if (hitTwisty) {
        this.ioBox.toggleObject(node);
      } else {
        if (this.inspecting) {
          this.stopInspecting(true);
        } else {
          this.select(node, true, false);
          this.highlighter.highlightNode(node);
        }
      }
    }
  },

?

Comment 11

[Paul Rouget [:paul]]

(In reply to comment #10)
> what about:
> 
>   onTreeClick: function IUI_onTreeClick(aEvent)
>   {
>     let node;
>     let target = aEvent.target;
>     let hitTwisty = false;
>     if (this.hasClass(target, "twisty")) {
>       node = this.getRepObject(aEvent.target.nextSibling);
>       hitTwisty = true;
>     } else {
>       node = this.getRepObject(aEvent.target);
>     }
> 
>     if (node) {
>       if (hitTwisty) {
>         this.ioBox.toggleObject(node);
>       } else {
>         if (this.inspecting) {
>           this.stopInspecting(true);
>         } else {
>           this.select(node, true, false);
>           this.highlighter.highlightNode(node);
>         }
>       }
>     }
>   },
> 
> ?

I'm ok with this, but what is it supposed to change?

Comment 12

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

only difference is it eliminates the redundant calls to highlighter.highlightNode and select which are called from stopInspecting.

Comment 13

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

Comment on attachment 548112 [details] [diff] [review]
[in-fx-team] patch v1.2

http://hg.mozilla.org/integration/fx-team/rev/6eee63caf1c6

Comment 14

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

Comment on attachment 548112 [details] [diff] [review]
[in-fx-team] patch v1.2

not in-devtools haha!lol

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/6eee63caf1c6