The default bug view has changed. See this FAQ.

[highlighter] Nodes should be selectable from the HTML tree

RESOLVED FIXED in Firefox 8

Status

()

Firefox
Developer Tools
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

unspecified
Firefox 8
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [minotaur])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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).
Attachment #546032 - Flags: review?(rcampbell)
(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.
Assignee: nobody → paul
Status: NEW → ASSIGNED
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.
(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 on attachment 546032 [details] [diff] [review]
patch v1

r- because of lack of unittest and above behavior.
Attachment #546032 - Flags: review?(rcampbell) → review-

Updated

6 years ago
Whiteboard: [minotaur]
(Assignee)

Comment 5

6 years ago
Created attachment 547707 [details] [diff] [review]
patch v1.1

Addressed Rob's comments (test and no toggle-highlight).
Attachment #546032 - Attachment is obsolete: true
Attachment #547707 - Flags: review?(rcampbell)
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.
Attachment #547707 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 7

6 years ago
Created attachment 548112 [details] [diff] [review]
[in-fx-team] patch v1.2

Thanks for the r+ Rob.
(Assignee)

Updated

6 years ago
Attachment #548112 - Flags: review?(gavin.sharp)
Priority: -- → P1
Whiteboard: [minotaur] → [minotaur][has-patch]
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.
Attachment #548112 - Flags: review?(gavin.sharp) → review+
(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.
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);
        }
      }
    }
  },

?
(Assignee)

Comment 11

6 years ago
(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?
only difference is it eliminates the redundant calls to highlighter.highlightNode and select which are called from stopInspecting.
Whiteboard: [minotaur][has-patch] → [minotaur][fixed-in-fx-team]
Target Milestone: --- → Firefox 8
Comment on attachment 548112 [details] [diff] [review]
[in-fx-team] patch v1.2

http://hg.mozilla.org/integration/fx-team/rev/6eee63caf1c6
Attachment #548112 - Attachment description: patch v1.2 → [in-devtools] patch v1.2
Comment on attachment 548112 [details] [diff] [review]
[in-fx-team] patch v1.2

not in-devtools haha!lol
Attachment #548112 - Attachment description: [in-devtools] patch v1.2 → [in-fx-team] patch v1.2
http://hg.mozilla.org/mozilla-central/rev/6eee63caf1c6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [minotaur][fixed-in-fx-team] → [minotaur]
You need to log in before you can comment on or make changes to this bug.