Last Comment Bug 671689 - [highlighter] Nodes should be selectable from the HTML tree
: [highlighter] Nodes should be selectable from the HTML tree
Status: RESOLVED FIXED
[minotaur]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 Linux
: P1 normal (vote)
: Firefox 8
Assigned To: Paul Rouget [:paul]
:
:
Mentors:
Depends on: 669656
Blocks: 663830
  Show dependency treegraph
 
Reported: 2011-07-14 16:05 PDT by Paul Rouget [:paul]
Modified: 2011-08-03 07:56 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (1.23 KB, patch)
2011-07-14 16:05 PDT, Paul Rouget [:paul]
rcampbell: review-
Details | Diff | Splinter Review
patch v1.1 (3.71 KB, patch)
2011-07-22 08:53 PDT, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Splinter Review
[in-fx-team] patch v1.2 (4.61 KB, patch)
2011-07-25 00:44 PDT, Paul Rouget [:paul]
gavin.sharp: review+
Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2011-07-14 16:05:24 PDT
Created attachment 546032 [details] [diff] [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).
Comment 1 Rob Campbell [:rc] (:robcee) 2011-07-18 12:40:45 PDT
(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) 2011-07-18 14:36:59 PDT
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) 2011-07-19 03:07:35 PDT
(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) 2011-07-19 08:16:08 PDT
Comment on attachment 546032 [details] [diff] [review]
patch v1

r- because of lack of unittest and above behavior.
Comment 5 Paul Rouget [:paul] 2011-07-22 08:53:37 PDT
Created attachment 547707 [details] [diff] [review]
patch v1.1

Addressed Rob's comments (test and no toggle-highlight).
Comment 6 Rob Campbell [:rc] (:robcee) 2011-07-22 12:34:19 PDT
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] 2011-07-25 00:44:40 PDT
Created attachment 548112 [details] [diff] [review]
[in-fx-team] patch v1.2

Thanks for the r+ Rob.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-02 13:49:18 PDT
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) 2011-08-02 14:03:13 PDT
(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) 2011-08-02 14:06:58 PDT
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] 2011-08-02 14:12:05 PDT
(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) 2011-08-02 14:13:19 PDT
only difference is it eliminates the redundant calls to highlighter.highlightNode and select which are called from stopInspecting.
Comment 13 Rob Campbell [:rc] (:robcee) 2011-08-02 14:33:02 PDT
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) 2011-08-02 14:33:34 PDT
Comment on attachment 548112 [details] [diff] [review]
[in-fx-team] patch v1.2

not in-devtools haha!lol
Comment 15 Tim Taubert [:ttaubert] 2011-08-03 07:56:41 PDT
http://hg.mozilla.org/mozilla-central/rev/6eee63caf1c6

Note You need to log in before you can comment on or make changes to this bug.