Closed Bug 1211613 Opened 9 years ago Closed 8 years ago

Reorganize the inspector context menu

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: ntim, Assigned: moaaz.omer, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [devtools-ux][qx:spec:link])

Attachments

(2 files, 3 obsolete files)

It's starting to get a bit crowded. Moving items into submenus is something we should consider doing.
Attached image current-menu-items.png
Here is what it looks like with the new menu items from bug 994555 added.
Then there'll be one more with bug 1208864 (duplicate node).
And on top of this, there are 2 hidden ones: "open link in a new window" and "copy URL" which are shown when you right-click on a URL.

Helen: One question I have is: do you think it's better to enable/disable menu items or hide/show them? Looks like we have a mix of the 2 and I think we shouldn't.
Flags: needinfo?(hholmes)
I think hiding/showing might be better simply because we have so much we can hide/show. I'm fine with the enabling/disabling as well as a rule, so if anyone's feeling strongly about it one way or another for a good reason let's go with that—I will say that I agree with pbro that we shouldn't be mixing styles.
Flags: needinfo?(hholmes)
All right, what pbro and I worked on (ntim, I know you had a mockup but I couldn't find it again 
All right, what pbro and I worked on (ntim, I know you had a mockup but I couldn't find it again):

Edit as HTML
Delete node
Attributes >>
   Add attribute
   Edit attribute
   Remove attribute
———————
:hover
:active
:focus
:visited
———————
Copy >>
   Inner HTML
   Outer HTML
   Image data-url
   Copy unique selector
Paste >>
   Inner HTML
   Outer HTML
   Before node
   After node
———————
Scroll into view
Screenshot node
We can't add the :visited lock until Bug 729310 is fixed
See Also: → 729310
We also still need Show DOM Properties (I propose putting it at the very end). However I'd like to come up with a better name for it.

"Show DOM Properties" --> I think, attributes, classes, ids, not
"Show DOM Properties" --> javascript object.
final_version_v5.psd

Edit as HTML
Delete node
Attributes >>
   Add attribute
   Edit attribute
   Remove attribute
———————
:hover
:active
:focus
:visited
———————
Copy >>
   Inner HTML
   Outer HTML
   Image data-url
   Copy unique selector
Paste >>
   Inner HTML
   Outer HTML
   Before node
   After node
———————
Scroll into view
Screenshot node
Use in console
Show DOM properties
So here's my version:
Copy >>
   Inner HTML
   Outer HTML
   Image data-url
   Copy unique selector
Paste >>
   Inner HTML
   Outer HTML
   Before node
   After node
———————
Edit as HTML
Scroll into view
Screenshot node
Duplicate node
Delete node
———————
Attributes >>
   Add attribute
   Edit attribute
   Remove attribute
Force element state >>
   :hover
   :active
   :focus
   :visited
Use in console
Show DOM properties

I don't think it's very important to put the pseudo class locks top-level, since they take up space, and the functionality is already available in the rules view.
There's also a command that isn't surfaced yet in the UI that can be accessed via 'h' which is "Hide Element".  Maybe that could be planned for / added in a follow up if we have some room after reorganizing and think it's good to surface.
nothing_is_ever_final_v6.psd

Edit as HTML
Delete node
Attributes >>
   Add attribute
   Edit attribute
   Remove attribute
———————
:hover
:active
:focus
:visited
———————
Copy >>
   Inner HTML
   Outer HTML
   Image data-url
   Copy unique selector
Paste >>
   Inner HTML
   Outer HTML
   Before node
   After node
———————
Scroll into view
Screenshot node
Copy node to clipboard
Use in console
Show DOM properties
(In reply to Brian Grinstead [:bgrins] from comment #9)
> There's also a command that isn't surfaced yet in the UI that can be
> accessed via 'h' which is "Hide Element".  Maybe that could be planned for /
> added in a follow up if we have some room after reorganizing and think it's
> good to surface.

I think that this is fine to have hidden for now. Let's just address the current reorganizing and file this as a backlog bug to discuss in the future if we think it should be surfaced. (I don't think we need to surface all functionality that's been coded into the Inspector; that would be a lot of things.)
Mentor: pbrosset
Whiteboard: [devtools-ux] → [devtools-ux][qx:spec]
The relevant code for this bug is in devtools/client/inspector/inspector.xul: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector.xul

The mentor for this bug is Patrick, you can contact him with :pbro in the #devtools channel. :bgrins can also answer questions about this bug as well.
Whiteboard: [devtools-ux][qx:spec] → [devtools-ux][qx:spec:link]
Would love to take this bug on and work on it.
(In reply to Moaaz Sidat from comment #13)
> Would love to take this bug on and work on it.

Awesome! I'll assign the bug to you. Let me know (or Patrick or Brian) if you have questions!
Assignee: nobody → moaaz.omer
(In reply to Moaaz Sidat from comment #13)
> Would love to take this bug on and work on it.
Thanks for wanting to work on this.

Looks like you're new to bugzilla, so I'm assuming you haven't yet installed the dev environment. Please go through the docs here: https://wiki.mozilla.org/DevTools/Hacking
And don't hesitate to ask questions on IRC as Helen said (see https://wiki.mozilla.org/DevTools/GetInvolved#Communication).

In terms of code, I think you'll mostly be interested in looking at these 2 files:
- devtools/client/inspect/inspector.xul (in particular the element with id inspector-node-popup)
- and devtools/client/inspect/inspector-panel.js (in particular the function called _setupNodeMenu)
(In reply to Patrick Brosset [:pbro] from comment #15)
> (In reply to Moaaz Sidat from comment #13)
> > Would love to take this bug on and work on it.
> Thanks for wanting to work on this.
> 
> Looks like you're new to bugzilla, so I'm assuming you haven't yet installed
> the dev environment. Please go through the docs here:
> https://wiki.mozilla.org/DevTools/Hacking
> And don't hesitate to ask questions on IRC as Helen said (see
> https://wiki.mozilla.org/DevTools/GetInvolved#Communication).
> 
> In terms of code, I think you'll mostly be interested in looking at these 2
> files:
> - devtools/client/inspect/inspector.xul (in particular the element with id
> inspector-node-popup)
> - and devtools/client/inspect/inspector-panel.js (in particular the function
> called _setupNodeMenu)

Yup, I am. Thanks for pointing me in the correct direction. I'll get the dev environment setup today, and will reach out with questions on IRC if I have any. Once I have the setup completed, I'll take a look at the files you've highlighted. 

Cheers!
Hey folks, had a question (tried to reach out to :pbro via IRC, but I'm not sure if I got through, first time with IRC).

Anyway, for each element in the popup menu, the tag looks somewhat like:

<menuitem id="node-menu-edithtml"
	label="&inspectorHTMLEdit.label;"
	accesskey="&inspectorHTMLEdit.accesskey;">

I was wondering where the inspectorHTMLEdit.label and .accesskey are defined in the codebase?

The reason I ask is that I'd be introducing some new elements (such as the submenu for copy), as I understand, and would like to know how to go about giving it the appropriate label. 

Thanks and cheers!
Flags: needinfo?(pbrosset)
(In reply to Moaaz Sidat from comment #17)
> Hey folks, had a question (tried to reach out to :pbro via IRC, but I'm not
> sure if I got through, first time with IRC).
I don't think I've seen your ping on IRC. Worth noting: I'm in Europe, so depending on where you are in the world, timezones might make it hard to discuss live on IRC. :bgrins is Pacific-time and can also help you on IRC if needed.
Make sure to connect to the irc.mozilla.org server and the #devtools channel.
> Anyway, for each element in the popup menu, the tag looks somewhat like:
> 
> <menuitem id="node-menu-edithtml"
> 	label="&inspectorHTMLEdit.label;"
> 	accesskey="&inspectorHTMLEdit.accesskey;">
> 
> I was wondering where the inspectorHTMLEdit.label and .accesskey are defined
> in the codebase?
I highly suggest using http://dxr.mozilla.org/, it allows to search really quickly the whole mozilla source code. Entering these 2 string names in dxr, you'll find where they are defined and used in the code, that's very useful when discovering a codebase for the first time.
So, for this string in particular, you'll see that it's defined here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/locales/en-US/inspector.dtd#1
So, in your local checkout, in the file /devtools/client/locales/en-US/inspector.dtd
Flags: needinfo?(pbrosset)
Thanks for the info, :pbro

Yup, found this file but wasn't really sure if this was a generated file or one where definitions lived, thanks for the pointing me in the correct direction!
See Also: → 1257613
Hey folks, here are the screenshots of the implemented inspector popup  (Dropbox links):

Screenshot 1: https://www.dropbox.com/s/k18erl26yot7lbc/Screenshot%202016-04-07%2018.39.53.png?dl=0
Screenshot 2: https://www.dropbox.com/s/yaddo7p64ebn7ow/Screenshot%202016-04-07%2018.40.03.png?dl=0

:helenvholmes :pbro :bgrins

3 notes: 
- I believe Paste has two more options than outlined in the 'nothing_is_ever_final_v6.psd' attached above in this issue. The items missing from the mockup are 'As First Child' and 'As Last Child' options. Let me know if they should live inside the Paste submenu (see Screenshot 2) or outside of it.
- The mockup is also missing 'Duplicate Node', right now I have that as the second item in the submenu, let me know if that needs to be moved around.
- The mockup also has one additional item, 'Copy node to clipboard'. Let me know if that's something that's part of another issue, or is something else that I may have missed. 

Thanks!
(In reply to Moaaz Sidat from comment #20)
> 3 notes: 
> - I believe Paste has two more options than outlined in the
> 'nothing_is_ever_final_v6.psd' attached above in this issue. The items
> missing from the mockup are 'As First Child' and 'As Last Child' options.
> Let me know if they should live inside the Paste submenu (see Screenshot 2)
> or outside of it.
> - The mockup is also missing 'Duplicate Node', right now I have that as the
> second item in the submenu, let me know if that needs to be moved around.
> - The mockup also has one additional item, 'Copy node to clipboard'. Let me
> know if that's something that's part of another issue, or is something else
> that I may have missed. 
> 
> Thanks!

When Brian and I originally discussed this feature we decided to eliminate those options to streamline the menu. I'm going to needinfo him one last time to make sure he still feels good about the decision.
Flags: needinfo?(bgrinstead)
I don't remember exactly.  We definitely should not *add* any new items in this bug like 'copy node to clipboard' (maybe that's a rename from copy outer html)?

I'd be OK with removing Duplicate Node, although I would also be fine with landing it as-is and doing the removal in a follow up since it will require test changes, but let's forward that decision to Patrick.
Flags: needinfo?(bgrinstead) → needinfo?(pbrosset)
I'm ok with removing the "duplicate node" item. This is one of the lesser used ones I believe. But as Brian said, let's do this in a separate bug, so we don't make this one too complex.
Flags: needinfo?(pbrosset)
See Also: → 1095532
Comment on attachment 8740538 [details] [diff] [review]
Reordering the items in context menu and moving copy and paste items in submenus.

Review of attachment 8740538 [details] [diff] [review]:
-----------------------------------------------------------------

This is a great start, makes the menu a lot easier to navigate. Thanks.
I think we should land this and then perhaps do another iteration in another bug to discuss items such as "duplicate node" or "collapse" and "expand" which aren't in comment 10 but are still in the menu.

I'm not R+'ing this just now though because I think we need some new labels:

- The group menu item "Attribute" should be plural: "Attributes"
- All sub-items inside the Copy and the Paste groups shouldn't repeat the copy and paste prefixes. So inside Copy for instance, we should "Inner HTML" instead of "Copy Inner HTML"

Now, in order to do this though, you can't just change those string values in inspector.dtd, whenever a string value needs to change, we need to make a new string for it, so that people in charge of translating these into other languages are aware of the change.
See the doc here: 
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings
Attachment #8740538 - Flags: review?(pbrosset)
(In reply to Patrick Brosset <:pbro> (off April 18 to 22) from comment #25)
> I'm not R+'ing this just now though because I think we need some new labels:

Should Bug 1106437 be addressed with this bug instead? (Should just be a simple string change.)
Flags: needinfo?(pbrosset)
Updated Attribute label to Attributes and removed Copy and Paste prefixes from the submenu items labels.
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #26)
> (In reply to Patrick Brosset <:pbro> (off April 18 to 22) from comment #25)
> > I'm not R+'ing this just now though because I think we need some new labels:
> 
> Should Bug 1106437 be addressed with this bug instead? (Should just be a
> simple string change.)
You're right, it would be a very simple string change to do here since there are other strings being changed. Someone was working on this a few days ago though (see bug 1106437 comment 9).

Moaaz: thanks for your latest patch, as per the above, could you please update
<!ENTITY inspectorUniqueSelector.label "Unique Selector">
<!ENTITY inspectorUniqueSelector.accesskey "U">

to this instead:
<!ENTITY inspectorCopyCSSSelector.label "CSS Selector">
<!ENTITY inspectorCopyCSSSelector.accesskey "S">

I'll close the other bug as a duplicate and offer the person to work on another bug. There's no sense working on the same thing in 2 separate bugs and risking merge issues.
Flags: needinfo?(pbrosset) → needinfo?(moaaz.omer)
See Also: → 1175629
Made the relevant changes to the 'Unique Selector' entity, changing it to 'CSS Selector', with a new label and accesskey, to address Bug 1106437.
Attachment #8745025 - Flags: review?(pbrosset)
Attachment #8740538 - Attachment is obsolete: true
Attachment #8743988 - Attachment is obsolete: true
Comment on attachment 8745025 [details] [diff] [review]
Updated Unique Selector to CSS Selector

Review of attachment 8745025 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for these changes.
I'm gong to R+ this now, but I do have one last comment (see below).
What this means is that you can upload a new patch with the change and mark it as R+ yourself (no need to ask for another review from me).

I have 2 other questions:
- Could you please change the commit message to match our guidelines: Bug <number> - <description of the fix>; r=<reviewer>
So in your case: Bug 1211613 - Re-organized the inspector context menu; r=pbro
- Did you run the existing inspector tests locally to check if some need to be updated now? You can run all mochitests with `mach mochitest devtools/client/inspector/test` although some might also be in `mach mochitest devtools/client/inspector/markup/test`.

::: devtools/client/locales/en-US/inspector.dtd
@@ +26,4 @@
>       in the inspector contextual-menu for the item that lets users paste outer
>       HTML in the current node -->
> +<!ENTITY inspectorPasteOuterHTML.label      "Outer HTML">
> +<!ENTITY inspectorPasteOuterHTML.accesskey  "P">

Sorry for asking for changes again :( but now that these are in separate sub-menus, we can use the O and I accesskeys for pasting Outer HTML and Inner HTML.
We use them in the Copy sub-menu, so it would make sense to use them also here.
Also, P isn't a letter in the string "Outer HTML", so having it as an access key looks a bit weird.
Attachment #8745025 - Flags: review?(pbrosset) → review+
Hey Patrick,
Thanks for the review! 
I'll update the commit message and make the relevant changes to the accesskeys shortly. 
I do believe I ran the tests earlier, but I'll run 'em again to be safe.
Priority: -- → P2
Updated the commit message, the access keys for paste inner and outer html, and ran the test outlined in the above comment. Should be good to go now. Let me know in case I'm missing something.

Thanks for the feedback, input and help along the way of my first contribution to Mozilla. I look forward to contributing more. Cheers!
Attachment #8747568 - Flags: review+
Attachment #8745025 - Attachment is obsolete: true
Thanks Moaaz! I have pushed this patch to the TRY server to run all tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b71ce28878dd&group_state=expanded

If everything passes, I'll land the patch.
Flags: needinfo?(moaaz.omer)
https://hg.mozilla.org/mozilla-central/rev/5e9136916f72
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Reproduced it on Firefox nightly according to(2015-10-05)

Fixing bug is verified on Latest Developer Edition-- Build ID:( 20160703004019 ),User Agent: Mozilla/5.0 (Windows NT 10.0; rv:49.0) Gecko/20100101 Firefox/49.0

Following that request,It already updated on Latest version.

Tested OS-- Windows10 32bit
QA Whiteboard: [bugday-20160629]
I've updated the following page to show the reorganized content menu:

https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#Element_popup_context_menu

I've also made sure that this change is mentioned in the Fx 49 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/49#Developer_Tools

Let me know if you think it needs anything else; thanks!
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: