Closed Bug 777001 Opened 10 years ago Closed 10 years ago

[Developer Toolbar] "inspect" command refers to "node", should refer to "selector"


(DevTools :: General, defect)

Not set


(Not tracked)

Firefox 17


(Reporter: ianbicking, Unassigned)


(Whiteboard: [fixed-in-fx-team])


(1 file, 2 obsolete files)

In the completion suggestion and in help the "inspect" command refers to invoking "inspect node" but the second argument is a selector, not some node object.  It would be easier to immediately understand the second argument if it was consistently named "selector".
Attached patch propose (obsolete) — Splinter Review
Like this?
Comment on attachment 647684 [details] [diff] [review]

>      document.defaultView.InspectorUI.openInspectorUI(args.node);

I'm guessing that line has to be changed too?  And perhaps the help documents (inspectNodeDesc, inspectNodeManual) also need to be updated?
Attached patch propose rev2 (obsolete) — Splinter Review
I fixed Ian's commented point.
Attachment #647684 - Attachment is obsolete: true
Attachment #649298 - Flags: review?(ianb)
Comment on attachment 649298 [details] [diff] [review]
propose rev2

I'm not really familiar enough with this code to review.
Attachment #649298 - Flags: review?(ianb)
Attachment #649298 - Flags: review?(jwalker)

So technically, the thing that we're inspecting is a node, and not (the result of looking up) a selector, in that we demand a single node. We could argue that 'node' is a subset of 'selector', I guess.

In addition we have discussed (and I've pushed back on) allowing use of xpath to specify a node.

However, I think that the user can imply a singular result from the context, but they can't imply the syntax, so probably 'selector' is more helpful.

So I'm generally favourable to this, but with caveats, that I thought should be explained.

More in review...
Comment on attachment 649298 [details] [diff] [review]
propose rev2

Review of attachment 649298 [details] [diff] [review]:

r+ with the changes below, assuming my notes on 'node vs selector' don't change anyone's mind.
Thanks for the patch.

::: browser/devtools/commandline/GcliCommands.jsm
@@ +484,5 @@
>    manual: gcli.lookup("inspectManual"),
>    params: [
>      {
> +      name: "selector",
> +      type: "selector",

Changing the *type* to selector is wrong - the type really is "node" (unless you want to go into GCLI and change how the NodeType is registered).

@@ +486,5 @@
>      {
> +      name: "selector",
> +      type: "selector",
> +      description: gcli.lookup("inspectSelectorDesc"),
> +      manual: gcli.lookup("inspectSelectorManual")

Much as I think we should keep the naming consistent, I think the localizers will be cross with us if we needlessly change the lookup strings, so could we leave all of these as 'Node'.
Attachment #649298 - Flags: review?(jwalker) → review+
Attached patch patch rev.3Splinter Review
Fixed Joe's point.

I think that "node" signage mislead the user like this parameter accept JS expression, |document.getElementById()|.
Attachment #649298 - Attachment is obsolete: true
Attachment #653298 - Flags: review?(jwalker)
Comment on attachment 653298 [details] [diff] [review]
patch rev.3

Review of attachment 653298 [details] [diff] [review]:

Attachment #653298 - Flags: review?(jwalker) → review+
This was failing on Try? Thanks for at least not pushing inbound without Try results...

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_gcli_inspect.js | emptyParameters[0] - Got  <selector>, expected  <node>
My test script wasn't working properly, so I didn't catch this before I pushed, but I fixed the patch, and pushed it again.
Sorry. I had missed it.
And Thank you, Joe.

By the way, Joe, I detect that we need to change the argument of "screenshot" command "node",  to "selector", if we change "inspect" command.
(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #12)
> Sorry. I had missed it.
> And Thank you, Joe.
> By the way, Joe, I detect that we need to change the argument of
> "screenshot" command "node",  to "selector", if we change "inspect" command.

Is 'yes please' a valid answer?
(In reply to Joe Walker [:jwalker] from comment #13)
> Is 'yes please' a valid answer?

Yes. I would like to work it.
Needless to say, Should I file the new bug?
Yes please!
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.