The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ianb, Unassigned)

Tracking

unspecified
Firefox 17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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".
Created attachment 647684 [details] [diff] [review]
propose

Like this?
(Reporter)

Comment 2

5 years ago
Comment on attachment 647684 [details] [diff] [review]
propose

>      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?
Created attachment 649298 [details] [diff] [review]
propose rev2

I fixed Ian's commented point.
Attachment #647684 - Attachment is obsolete: true
Attachment #649298 - Flags: review?(ianb)
(Reporter)

Comment 4

5 years ago
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)

Updated

5 years ago
Attachment #649298 - Flags: review?(jwalker)
Hmm.

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+
Created attachment 653298 [details] [diff] [review]
patch rev.3

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]:
-----------------------------------------------------------------

Thanks!
Attachment #653298 - Flags: review?(jwalker) → review+
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Fx-Team&rev=bbd267f16fc7
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
This was failing on Try? Thanks for at least not pushing inbound without Try results...
https://tbpl.mozilla.org/?tree=Try&rev=4c6632e2334a

https://tbpl.mozilla.org/php/getParsedLog.php?id=14693190&tree=Try

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.
Thanks.
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!

Comment 16

5 years ago
https://hg.mozilla.org/mozilla-central/rev/33945cb30eae
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.