Last Comment Bug 587134 - Context menu item for Highlight Element (highlighter)
: Context menu item for Highlight Element (highlighter)
Status: VERIFIED FIXED
[minotaur][fixed-in-fx-team]
: verified-aurora
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 9
Assigned To: Panos Astithas [:past]
:
Mentors:
Depends on: 642471
Blocks: 696167 712871
  Show dependency treegraph
 
Reported: 2010-08-13 12:45 PDT by Mihai Sucan [:msucan]
Modified: 2011-12-23 08:31 PST (History)
9 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Inspect Element context menu item (9.08 KB, patch)
2010-08-21 06:51 PDT, Rob Campbell [:rc] (:robcee)
limi: ui‑review+
Details | Diff | Review
Proposed patch, version 1.1. (7.63 KB, patch)
2010-09-01 12:14 PDT, Patrick Walton (:pcwalton)
no flags Details | Diff | Review
Inspector context menu (14.22 KB, patch)
2010-09-22 13:49 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Review
context menu (14.63 KB, patch)
2011-05-27 13:52 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Review
Highlighter context menu (15.82 KB, patch)
2011-08-19 14:09 PDT, Panos Astithas [:past]
gavin.sharp: review-
Details | Diff | Review
Highlighter context menu v2 (16.12 KB, patch)
2011-09-01 12:36 PDT, Panos Astithas [:past]
gavin.sharp: review-
Details | Diff | Review
Highlighter context menu v3 (15.63 KB, patch)
2011-09-15 21:54 PDT, Panos Astithas [:past]
gavin.sharp: review+
Details | Diff | Review
Diff between v2 and v3 (6.84 KB, patch)
2011-09-15 21:56 PDT, Panos Astithas [:past]
no flags Details | Diff | Review
[backed-out] Highlighter context menu v4 (15.53 KB, patch)
2011-09-20 01:12 PDT, Panos Astithas [:past]
no flags Details | Diff | Review
[in-fx-team] Highlighter context menu v5 (29.27 KB, patch)
2011-09-21 04:15 PDT, Panos Astithas [:past]
no flags Details | Diff | Review

Description Mihai Sucan [:msucan] 2010-08-13 12:45:00 PDT
When one right-clicks inside a web page, he/she should have the option to inspect the target element, using the integrated Inspector tool. This context menu option is available in the other browsers as well.
Comment 1 Kevin Dangoor 2010-08-16 06:47:31 PDT
As part of this, we need to figure out the interplay between this Inspect Element and the one added to the context menu by Firebug.
Comment 2 Rob Campbell [:rc] (:robcee) 2010-08-20 09:00:31 PDT
taking this to go with the other menu bug.
Comment 3 Rob Campbell [:rc] (:robcee) 2010-08-21 06:51:21 PDT
Created attachment 468036 [details] [diff] [review]
Inspect Element context menu item

first patch, setting shouldShow to true for now because this should appear when hovering over just about anything in a webpage.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-08-24 12:26:16 PDT
Should probably get some kind of UX guidance on whether showing this menu item on all context menus regardless of the target makes sense. It seems like it would break muscle memory in at least some cases ("copy link location"? probably not commonly used...).
Comment 5 Rob Campbell [:rc] (:robcee) 2010-08-24 12:44:19 PDT
I did add it to the bottom of the menu, so hopefully, that shouldn't break anybody's muscle memory. I'm just trying to think of any web-page content that you might never want to inspect and can't think of any.

CC'ing limi for some UI-feedback.
Comment 6 Alex Limi (:limi) — Firefox UX Team 2010-08-24 15:58:39 PDT
Comment on attachment 468036 [details] [diff] [review]
Inspect Element context menu item

Adding it at the end of the menu (as long as it's not grouped with anything else) seems sane to me — I can't read from the diff whether this is the case, but from the comments, it seems to be the case? :)
Comment 7 Rob Campbell [:rc] (:robcee) 2010-08-24 16:09:51 PDT
yeah, it will be living alone down there under a separator. That is, until you install an add-on that modifies the context menu.

Thanks!
Comment 8 Rob Campbell [:rc] (:robcee) 2010-08-27 15:53:39 PDT
moving to beta6. Requesting blocking.
Comment 9 Patrick Walton (:pcwalton) 2010-09-01 12:14:06 PDT
Created attachment 471215 [details] [diff] [review]
Proposed patch, version 1.1.

Patch rebased to trunk.
Comment 10 Kevin Dangoor 2010-09-03 19:57:48 PDT
Inspector feature postponed. Removing blocking flags from Inspector bugs.
Comment 11 Kevin Dangoor 2010-09-03 20:34:45 PDT
Removing items from kd4b6.
Comment 12 Kevin Dangoor 2010-09-04 19:00:55 PDT
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Comment 13 Rob Campbell [:rc] (:robcee) 2010-09-22 13:49:15 PDT
Created attachment 477639 [details] [diff] [review]
Inspector context menu

Revised patch, unbitrotted as of 2010-09-22.

Creates a single "Inspect" menu item on the context menu if the Inspector is enabled (via devtools.inspector.enabled).

Added a unittest to browser_inspector_initialization.js.
Comment 14 Rob Campbell [:rc] (:robcee) 2011-05-27 13:52:50 PDT
Created attachment 535741 [details] [diff] [review]
context menu

updated and unbitrotted. Initialization test seems to be failing.
Comment 15 Panos Astithas [:past] 2011-08-19 14:09:36 PDT
Created attachment 554545 [details] [diff] [review]
Highlighter context menu

Rebased and fixed failing test.
Comment 16 Rob Campbell [:rc] (:robcee) 2011-08-24 07:26:30 PDT
Comment on attachment 554545 [details] [diff] [review]
Highlighter context menu

asking mossop for a little review.
Comment 17 Dave Townsend [:mossop] 2011-08-25 15:22:07 PDT
Comment on attachment 554545 [details] [diff] [review]
Highlighter context menu

Alas, I am not a browser peer
Comment 18 Rob Campbell [:rc] (:robcee) 2011-08-26 08:23:32 PDT
(In reply to Dave Townsend (:Mossop) from comment #17)
> Comment on attachment 554545 [details] [diff] [review]
> Highlighter context menu
> 
> Alas, I am not a browser peer

oops!
Comment 19 Rob Campbell [:rc] (:robcee) 2011-08-26 08:25:52 PDT
Comment on attachment 554545 [details] [diff] [review]
Highlighter context menu

shifting to gavin for reviewage.
Comment 20 Panos Astithas [:past] 2011-08-29 08:25:19 PDT
I'll take some load off Rob's shoulders now that I'm back.
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-31 15:47:03 PDT
Comment on attachment 554545 [details] [diff] [review]
Highlighter context menu

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>   // Enable Inspector?
>   let enabled = gPrefService.getBoolPref(InspectorUI.prefEnabledName);
>+  InspectorUI.enabled = enabled; // Convenience for context menu item.

Seems like you should make this a true property of InspectorUI (and have it retrieve the pref, etc.)

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

>   initializeStore: function IUI_initializeStore()

>     // Has this windowID been inspected before?
>-    if (InspectorStore.hasID(this.winID)) {
>+    if (this.openedFromContextMenu)
>+      this.openedFromContextMenu = null;
>+    else if (InspectorStore.hasID(this.winID)) {

Can you explain what this is for, and why it's done in "initializeStore" (which seems unrelated?)

>+  inspectNodeFromContext: function IUI_inspectNodeFromContext(aNode)

>+      this.contextNode = aNode;

>+      Services.obs.addObserver(this.inspectObserver,
>+                               INSPECTOR_NOTIFICATIONS.OPENED,
>+                               false);

Could you use e.g.:

function inspectObserver() {
  ...
}
Services.obs.addObserver(inspectObserver.bind(this, aNode),
                         INSPECTOR_NOTIFICATIONS.OPENED, false);

>diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd

>+<!ENTITY inspectContextMenu.label     "Highlight Element">
>+<!ENTITY inspectContextMenu.accesskey "H">

This seems like an odd string to use, seems like it would be confusing in this context (the tool isn't exposed as the "highlighter" anywhere, is it?). The accesskey seems to conflict with thisFrameMenu.accesskey, which can be displayed at the same time as this item.

This doesn't seem to actually add the item to the end of the list, it's still above the bidi switcher and spell check stuff.
Comment 22 Dão Gottwald [:dao] 2011-09-01 00:34:42 PDT
I feel strongly that the menu item should not be labeled "Highlight Element". Whatever you do in the web developer menu is your choice, but you're exposing the menu item in a neutral context here and "Highlight Element" lacks the devtool connotation.
Comment 23 Panos Astithas [:past] 2011-09-01 04:32:56 PDT
The plan is to call the tool Highlighter, hence the "Highlight Element" label. There is a relevant thread in d.a.f: http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/1090f9ed965aca2b

Would you guys prefer to use "Inspect Element" in this patch and do the renaming in a followup patch (assuming the Highlighter name sticks, of course)? There might be a conflict with Firebug's menu item for a short time period, though.
Comment 24 Dão Gottwald [:dao] 2011-09-01 04:35:42 PDT
(In reply to Panos Astithas [:past] from comment #23)
> The plan is to call the tool Highlighter, hence the "Highlight Element"
> label. There is a relevant thread in d.a.f:
> http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/
> 1090f9ed965aca2b

I know, I replied there yesterday.

> Would you guys prefer to use "Inspect Element" in this patch

Yes.

> and do the
> renaming in a followup patch (assuming the Highlighter name sticks, of
> course)?

No.

> There might be a conflict with Firebug's menu item for a short time
> period, though.

As I said in the newsgroups, Firebug should either hijack the stock "Inspect Element" item or call its item "Inspect Element with Firebug".
Comment 25 Panos Astithas [:past] 2011-09-01 12:36:44 PDT
Created attachment 557610 [details] [diff] [review]
Highlighter context menu v2

(In reply to Gavin Sharp from comment #21)
> Comment on attachment 554545 [details] [diff] [review]
> Highlighter context menu
> 
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >   // Enable Inspector?
> >   let enabled = gPrefService.getBoolPref(InspectorUI.prefEnabledName);
> >+  InspectorUI.enabled = enabled; // Convenience for context menu item.
> 
> Seems like you should make this a true property of InspectorUI (and have it
> retrieve the pref, etc.)

Done.

> >diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js
> 
> >   initializeStore: function IUI_initializeStore()
> 
> >     // Has this windowID been inspected before?
> >-    if (InspectorStore.hasID(this.winID)) {
> >+    if (this.openedFromContextMenu)
> >+      this.openedFromContextMenu = null;
> >+    else if (InspectorStore.hasID(this.winID)) {
> 
> Can you explain what this is for, and why it's done in "initializeStore"
> (which seems unrelated?)

It's a flag for avoiding to reinitialize the InspectorStore after coming from a context menu invocation, since in that case the store is already initialized. Removing this flag causes context menu invocations to not work properly, when the highlighter is not already open.

> >+  inspectNodeFromContext: function IUI_inspectNodeFromContext(aNode)
> 
> >+      this.contextNode = aNode;
> 
> >+      Services.obs.addObserver(this.inspectObserver,
> >+                               INSPECTOR_NOTIFICATIONS.OPENED,
> >+                               false);
> 
> Could you use e.g.:
> 
> function inspectObserver() {
>   ...
> }
> Services.obs.addObserver(inspectObserver.bind(this, aNode),
>                          INSPECTOR_NOTIFICATIONS.OPENED, false);

I tried, but I can't see how I could remove the observer function inside inspectObserver, since it doesn't have access to the function created by the bind call.

> >diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd
> 
> >+<!ENTITY inspectContextMenu.label     "Highlight Element">
> >+<!ENTITY inspectContextMenu.accesskey "H">
> 
> This seems like an odd string to use, seems like it would be confusing in
> this context (the tool isn't exposed as the "highlighter" anywhere, is it?).
> The accesskey seems to conflict with thisFrameMenu.accesskey, which can be
> displayed at the same time as this item.
> 
> This doesn't seem to actually add the item to the end of the list, it's
> still above the bidi switcher and spell check stuff.

All fixed: renamed to "Inspect Element", moved to the end of the list and used N as the access key.
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-09-13 14:33:27 PDT
Comment on attachment 557610 [details] [diff] [review]
Highlighter context menu v2

(In reply to Panos Astithas [:past] from comment #25)
> It's a flag for avoiding to reinitialize the InspectorStore after coming
> from a context menu invocation, since in that case the store is already
> initialized. Removing this flag causes context menu invocations to not work
> properly, when the highlighter is not already open.

I don't understand why the context menu invocation of openInspectorUI() is special in this regard. If it's related to the fact that it uses a INSPECTOR_NOTIFICATIONS.OPENED observer to then trigger the inspection, it seems like this would be better addressed by adding an inspectNode parameter to openInspectorUI() and handling things internally.

"inspectNodeFromContext" seems far too specific - there's no need to mention the context menu at all. There should be a clean way to open the inspector for a particular node, and ideally that could be openInspectorUI directly.

We need to start paying more attention to InspectorUI's "exposed API" - things like highlighterReady and handleEvent should be "_"-prefixed to indicate that they're not useful externally. It would be good to get a bug on file for this.

> I tried, but I can't see how I could remove the observer function inside
> inspectObserver, since it doesn't have access to the function created by the
> bind call.

You can assign the bound function to a local variable and refer to it via a closure.
Comment 27 Panos Astithas [:past] 2011-09-15 21:54:51 PDT
Created attachment 560510 [details] [diff] [review]
Highlighter context menu v3

(In reply to Gavin Sharp from comment #26)
> Comment on attachment 557610 [details] [diff] [review]
> Highlighter context menu v2
> 
> (In reply to Panos Astithas [:past] from comment #25)
> > It's a flag for avoiding to reinitialize the InspectorStore after coming
> > from a context menu invocation, since in that case the store is already
> > initialized. Removing this flag causes context menu invocations to not work
> > properly, when the highlighter is not already open.
> 
> I don't understand why the context menu invocation of openInspectorUI() is
> special in this regard. If it's related to the fact that it uses a
> INSPECTOR_NOTIFICATIONS.OPENED observer to then trigger the inspection, it
> seems like this would be better addressed by adding an inspectNode parameter
> to openInspectorUI() and handling things internally.
> 
> "inspectNodeFromContext" seems far too specific - there's no need to mention
> the context menu at all. There should be a clean way to open the inspector
> for a particular node, and ideally that could be openInspectorUI directly.

OK, I like your ideal, so I removed the openedFromContextMenu flag and inspectNodeFromContext altogether, and put its logic into openInspectorUI. This path is followed when a node is provided as a parameter.

> We need to start paying more attention to InspectorUI's "exposed API" -
> things like highlighterReady and handleEvent should be "_"-prefixed to
> indicate that they're not useful externally. It would be good to get a bug
> on file for this.

Filed bug 686934.

> > I tried, but I can't see how I could remove the observer function inside
> > inspectObserver, since it doesn't have access to the function created by the
> > bind call.
> 
> You can assign the bound function to a local variable and refer to it via a
> closure.

OK, done.
Comment 28 Panos Astithas [:past] 2011-09-15 21:56:41 PDT
Created attachment 560511 [details] [diff] [review]
Diff between v2 and v3

Here are the changes since the previous patch in order to help you with the review.
Comment 29 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-09-19 18:51:20 PDT
Comment on attachment 560510 [details] [diff] [review]
Highlighter context menu v3

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

>+  openInspectorUI: function IUI_openInspectorUI(aNode)

The comment should probably explicitly mention that aNode is optional.

>+    // If the Inspector is initialized and is opened from the context menu, just
>+    // inspect the node.
>+    if (aNode && this.browser) {

Hmm, this logic looks a bit odd here (it's not obvious that openInspectorUI would have this behavior). I think this check would make more sense either in gContextMenu.inspectNode, e.g.:

if (InspectorUI.isTreePanelOpen)
  InspectorUI.inspectNode(this.target);
else
  InspectorUI.openInspectorUI(this.target);

or alternatively, in InspectorUI.inspectNode itself, e.g.:

if (!this.isTreePanelOpen) {
  this.openInspectorUI(aNode);
  return;
}

(in which case gContextMenu.inspectNode would continue to just call InspectorUI.inspectNode directly). I think I prefer the former, but defer to your judgement.

>+    function inspectObserver(aElement) {

>+      var boundInspectObserver = inspectObserver.bind(this, aNode);

These two can be declared next to each other, for clarity.

The rest of the patch looks fine, so r=me with these comments addressed (find me on IRC if you have any questions).
Comment 30 Panos Astithas [:past] 2011-09-20 01:12:01 PDT
Created attachment 561138 [details] [diff] [review]
[backed-out] Highlighter context menu v4

(In reply to Gavin Sharp from comment #29)
> Comment on attachment 560510 [details] [diff] [review]
> Highlighter context menu v3
> 
> >diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js
> 
> >+  openInspectorUI: function IUI_openInspectorUI(aNode)
> 
> The comment should probably explicitly mention that aNode is optional.

Sure.

> >+    // If the Inspector is initialized and is opened from the context menu, just
> >+    // inspect the node.
> >+    if (aNode && this.browser) {
> 
> Hmm, this logic looks a bit odd here (it's not obvious that openInspectorUI
> would have this behavior). I think this check would make more sense either
> in gContextMenu.inspectNode, e.g.:
> 
> if (InspectorUI.isTreePanelOpen)
>   InspectorUI.inspectNode(this.target);
> else
>   InspectorUI.openInspectorUI(this.target);
> 
> or alternatively, in InspectorUI.inspectNode itself, e.g.:
> 
> if (!this.isTreePanelOpen) {
>   this.openInspectorUI(aNode);
>   return;
> }
> 
> (in which case gContextMenu.inspectNode would continue to just call
> InspectorUI.inspectNode directly). I think I prefer the former, but defer to
> your judgement.

Good idea, and I share your preference, thanks.

> >+    function inspectObserver(aElement) {
> 
> >+      var boundInspectObserver = inspectObserver.bind(this, aNode);
> 
> These two can be declared next to each other, for clarity.

I had opted for a tiny optimization (avoiding the binding in the common case), but OK.

> The rest of the patch looks fine, so r=me with these comments addressed
> (find me on IRC if you have any questions).

Thank you very much!
Comment 31 Rob Campbell [:rc] (:robcee) 2011-09-20 08:06:17 PDT
Comment on attachment 561138 [details] [diff] [review]
[backed-out] Highlighter context menu v4

https://hg.mozilla.org/integration/fx-team/rev/304f93baaa1b
Comment 32 Rob Campbell [:rc] (:robcee) 2011-09-20 11:31:37 PDT
this has to be backed out, causing failures in test_contextmenu.html

failed | menuitem context-inspect has same accesskey as context-sendimage
failed | menuitem context-inspect has same accesskey as context-sendvideo
failed | menuitem context-inspect has same accesskey as context-sendvideo
failed | menuitem context-inspect has same accesskey as context-sendvideo
failed | checking item #13 (---) name - got undefined, expected ---
failed | checking item #14 (context-inspect) name - got undefined, expected context-inspect
failed | checking item #14 (context-inspect) enabled state - got undefined, expected true
failed | checking expected number of menu entries - got 26, expected 30
failed | checking expected number of menu entries - got 34, expected 30
failed | checking expected number of menu entries - got 34, expected 30

with the following patch applied:

diff --git a/browser/base/content/test/test_contextmenu.html b/browser/base/content/test/test_contextmenu.html
--- a/browser/base/content/test/test_contextmenu.html
+++ b/browser/base/content/test/test_contextmenu.html
@@ -242,125 +242,143 @@ function runTest(testNum) {
                           "context-bookmarkpage", true,
                           "context-savepage",     true,
                           "context-sendpage",     true,
                           "---",                  null,
                           "context-viewbgimage",  false,
                           "context-selectall",    true,
                           "---",                  null,
                           "context-viewsource",   true,
-                          "context-viewinfo",     true]);
+                          "context-viewinfo",     true,
+                          "---",                  null,
+                          "context-inspect",      true]);
         closeContextMenu();
         openContextMenuFor(link); // Invoke context menu for next test.
         break;
 
     case 3:
         // Context menu for text link
         checkContextMenu(["context-openlinkintab", true,
                           "context-openlink",      true,
                           "---",                   null,
                           "context-bookmarklink",  true,
                           "context-savelink",      true,
                           "context-sendlink",      true,
-                          "context-copylink",      true]);
+                          "context-copylink",      true,
+                          "---",                  null,
+                          "context-inspect",      true]);
         closeContextMenu();
         openContextMenuFor(mailto); // Invoke context menu for next test.
         break;
 
     case 4:
         // Context menu for text mailto-link
-        checkContextMenu(["context-copyemail", true]);
+        checkContextMenu(["context-copyemail", true,
+                          "---",                  null,
+                          "context-inspect",      true]);
         closeContextMenu();
         openContextMenuFor(input); // Invoke context menu for next test.
         break;
 
     case 5:
         // Context menu for text input field
         checkContextMenu(["context-undo",        false,
                           "---",                 null,
                           "context-cut",         false,
                           "context-copy",        false,
                           "context-paste",       null, // ignore clipboard state
                           "context-delete",      false,
                           "---",                 null,
                           "context-selectall",   true,
                           "---",                 null,
-                          "spell-check-enabled", true]);
+                          "spell-check-enabled", true,
+                          "---",                  null,
+                          "context-inspect",      true]);
         closeContextMenu();
         openContextMenuFor(img); // Invoke context menu for next test.
         break;
 
     case 6:
         // Context menu for an image
         checkContextMenu(["context-viewimage",            true,
                           "context-copyimage-contents",   true,
                           "context-copyimage",            true,
                           "---",                          null,
                           "context-saveimage",            true,
                           "context-sendimage",            true,
                           "context-setDesktopBackground", true,
-                          "context-viewimageinfo",        true]);
+                          "context-viewimageinfo",        true,
+                          "---",                          null,
+                          "context-inspect",              true]);
         closeContextMenu();
         openContextMenuFor(canvas); // Invoke context menu for next test.
         break;
 
     case 7:
         // Context menu for a canvas
         checkContextMenu(["context-viewimage",    true,
                           "context-saveimage",    true,
                           "context-bookmarkpage", true,
-                          "context-selectall",    true]);
+                          "context-selectall",    true,
+                          "---",                  null,
+                          "context-inspect",      true]);
         closeContextMenu();
         openContextMenuFor(video_ok); // Invoke context menu for next test.
         break;
 
     case 8:
         // Context menu for a video (with a VALID media source)
         checkContextMenu(["context-media-play",         true,
                           "context-media-mute",         true,
                           "context-media-showcontrols", true,
                           "context-video-fullscreen",   true,
                           "---",                        null,
                           "context-viewvideo",          true,
                           "context-copyvideourl",       true,
                           "---",                        null,
                           "context-savevideo",          true,
-                          "context-sendvideo",          true]);
+                          "context-sendvideo",          true,
+                          "---",                        null,
+                          "context-inspect",            true]);
         closeContextMenu();
         openContextMenuFor(video_bad); // Invoke context menu for next test.
         break;
 
     case 9:
         // Context menu for a video (with an INVALID media source)
         checkContextMenu(["context-media-play",         false,
                           "context-media-mute",         false,
                           "context-media-showcontrols", false,
                           "context-video-fullscreen",   false,
                           "---",                        null,
                           "context-viewvideo",          true,
                           "context-copyvideourl",       true,
                           "---",                        null,
                           "context-savevideo",          true,
-                          "context-sendvideo",          true]);
+                          "context-sendvideo",          true,
+                          "---",                        null,
+                          "context-inspect",            true]);
         closeContextMenu();
         openContextMenuFor(video_bad2); // Invoke context menu for next test.
         break;
 
     case 10:
         // Context menu for a video (with an INVALID media source)
         checkContextMenu(["context-media-play",         false,
                           "context-media-mute",         false,
                           "context-media-showcontrols", false,
                           "context-video-fullscreen",   false,
                           "---",                        null,
                           "context-viewvideo",          false,
                           "context-copyvideourl",       false,
                           "---",                        null,
                           "context-savevideo",          false,
-                          "context-sendvideo",          false]);
+                          "context-sendvideo",          false,
+                          "---",                        null,
+                          "context-inspect",            true]);
         closeContextMenu();
         openContextMenuFor(iframe); // Invoke context menu for next test.
         break;
 
     case 11:
         // Context menu for an iframe
         checkContextMenu(["context-back",         false,
                           "context-forward",      false,
@@ -381,20 +399,24 @@ function runTest(testNum) {
                                "context-reloadframe",       true,
                                "---",                       null,
                                "context-bookmarkframe",     true,
                                "context-saveframe",         true,
                                "---",                       null,
                                "context-printframe",        true,
                                "---",                       null,
                                "context-viewframesource",   true,
-                               "context-viewframeinfo",     true], null,
+                               "context-viewframeinfo",     true,
+                               "---",                  null,
+                               "context-inspect",      true], null,
                           "---",                  null,
                           "context-viewsource",   true,
-                          "context-viewinfo",     true]);
+                          "context-viewinfo",     true,
+                          "---",                  null,
+                          "context-inspect",      true]);
         closeContextMenu();
         openContextMenuFor(textarea); // Invoke context menu for next test.
         break;
 
     case 12:
         // Context menu for textarea
         checkContextMenu(["*chubbiness",         true, // spelling suggestion
                           "spell-add-to-dictionary", true,
@@ -407,17 +429,19 @@ function runTest(testNum) {
                           "context-delete",      false,
                           "---",                 null,
                           "context-selectall",   true,
                           "---",                 null,
                           "spell-check-enabled", true,
                           "spell-dictionaries",  true,
                               ["spell-check-dictionary-en-US", true,
                                "---",                          null,
-                               "spell-add-dictionaries",       true], null]);
+                               "spell-add-dictionaries",       true], null,
+                          "---",                  null,
+                          "context-inspect",      true]);
 
         closeContextMenu();
         openContextMenuFor(contenteditable); // Invoke context menu for next test.
         break;
 
     case 13:
         // Context menu for contenteditable
         checkContextMenu(["spell-no-suggestions", false,
@@ -431,17 +455,19 @@ function runTest(testNum) {
                           "context-delete",      false,
                           "---",                 null,
                           "context-selectall",   true,
                           "---",                 null,
                           "spell-check-enabled", true,
                           "spell-dictionaries",  true,
                               ["spell-check-dictionary-en-US", true,
                                "---",                          null,
-                               "spell-add-dictionaries",       true], null]);
+                               "spell-add-dictionaries",       true], null],
+                          "---",                  null,
+                          "context-inspect",      true);
 
         closeContextMenu();
         openContextMenuFor(inputspell); // Invoke context menu for next test.
         break;
 
     case 14:
         // Context menu for spell-check input
         checkContextMenu(["*prodigality",        true, // spelling suggestion
@@ -455,17 +481,19 @@ function runTest(testNum) {
                           "context-delete",      false,
                           "---",                 null,
                           "context-selectall",   true,
                           "---",                 null,
                           "spell-check-enabled", true,
                           "spell-dictionaries",  true,
                               ["spell-check-dictionary-en-US", true,
                                "---",                          null,
-                               "spell-add-dictionaries",       true], null]);
+                               "spell-add-dictionaries",       true], null],
+                          "---",                  null,
+                          "context-inspect",      true);
 
         closeContextMenu();
         openContextMenuFor(link); // Invoke context menu for next test.
         break;
 
     case 15:
         executeCopyCommand("cmd_copyLink", "http://mozilla.com/");
         closeContextMenu();
@@ -502,17 +530,19 @@ function runTest(testNum) {
                           "context-bookmarkpage", true,
                           "context-savepage",     true,
                           "context-sendpage",     true,
                           "---",                  null,
                           "context-viewbgimage",  false,
                           "context-selectall",    true,
                           "---",                  null,
                           "context-viewsource",   true,
-                          "context-viewinfo",     true]);
+                          "context-viewinfo",     true,
+                          "---",                  null,
+                          "context-inspect",      true]);
 
         invokeItemAction("0");
         closeContextMenu();
         openContextMenuFor(pagemenu, true); // Invoke context menu for next test.
         break;
 
     case 17:
         // Context menu for element with assigned content context menu
@@ -525,17 +555,19 @@ function runTest(testNum) {
                           "context-bookmarkpage", true,
                           "context-savepage",     true,
                           "context-sendpage",     true,
                           "---",                  null,
                           "context-viewbgimage",  false,
                           "context-selectall",    true,
                           "---",                  null,
                           "context-viewsource",   true,
-                          "context-viewinfo",     true]);
+                          "context-viewinfo",     true,
+                          "---",                  null,
+                          "context-inspect",      true]);
 
         subwindow.close();
         SimpleTest.finish();
         return;
 
     /*
      * Other things that would be nice to test:
      *  - selected text
Comment 33 Rob Campbell [:rc] (:robcee) 2011-09-20 11:34:30 PDT
backed out in https://hg.mozilla.org/integration/fx-team/rev/931650933b13
Comment 34 Rob Campbell [:rc] (:robcee) 2011-09-20 11:36:09 PDT
Comment on attachment 561138 [details] [diff] [review]
[backed-out] Highlighter context menu v4

when testing this patch, please make sure you do a full mochitest run. That includes content.
Comment 35 Panos Astithas [:past] 2011-09-21 04:15:43 PDT
Created attachment 561432 [details] [diff] [review]
[in-fx-team] Highlighter context menu v5

(In reply to Rob Campbell [:rc] (robcee) from comment #34)
> Comment on attachment 561138 [details] [diff] [review]
> [backed-out] Highlighter context menu v4
> 
> when testing this patch, please make sure you do a full mochitest run. That
> includes content.

Sorry about that, I wasn't aware of the other context menu tests. Thanks for the test patch, it was already half way there. I fixed the rest of the breakage and pushed to try:

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=197468723984

One side-effect was that the new context menu item access key is 'Q', since every letter in 'Inspect Element' was already used.
Comment 36 Panos Astithas [:past] 2011-09-22 03:02:21 PDT
Try results are green, this is ready to land.
Comment 37 Rob Campbell [:rc] (:robcee) 2011-09-22 04:50:19 PDT
Comment on attachment 561432 [details] [diff] [review]
[in-fx-team] Highlighter context menu v5

https://hg.mozilla.org/integration/fx-team/rev/f9d487c2b076
Comment 38 Rob Campbell [:rc] (:robcee) 2011-09-23 15:44:29 PDT
https://hg.mozilla.org/mozilla-central/rev/f9d487c2b076
Comment 39 Teodosia Pop 2011-09-29 05:34:12 PDT
Verified using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20110928 Firefox/10.0a1
Comment 40 Rob Campbell [:rc] (:robcee) 2011-12-23 08:31:48 PST
why are you making a mess in a bug that's been closed for 3 months? Please stop.

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