Closed Bug 587134 Opened 14 years ago Closed 13 years ago

Context menu item for Highlight Element (highlighter)

Categories

(DevTools :: General, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 9

People

(Reporter: msucan, Assigned: past)

References

Details

(Keywords: verified-aurora, Whiteboard: [minotaur][fixed-in-fx-team])

Attachments

(1 file, 9 obsolete files)

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.
blocking2.0: --- → ?
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.
Whiteboard: [kd4b5]
taking this to go with the other menu bug.
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
first patch, setting shouldShow to true for now because this should appear when hovering over just about anything in a webpage.
Attachment #468036 - Flags: review?
Attachment #468036 - Flags: review? → review?(gavin.sharp)
blocking2.0: ? → beta5+
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...).
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.
Attachment #468036 - Flags: ui-review?(limi)
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? :)
Attachment #468036 - Flags: ui-review?(limi) → ui-review+
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!
moving to beta6. Requesting blocking.
blocking2.0: beta5+ → ?
Whiteboard: [kd4b5] → [kd4b6]
blocking2.0: ? → beta6+
Whiteboard: [kd4b6] → [kd4b6] [strings]
Whiteboard: [kd4b6] [strings] → [kd4b6] [strings] [patchbitrot]
Attached patch Proposed patch, version 1.1. (obsolete) — Splinter Review
Patch rebased to trunk.
Attachment #471215 - Flags: review?(gavin.sharp)
Attachment #468036 - Flags: review?(gavin.sharp)
Whiteboard: [kd4b6] [strings] [patchbitrot] → [kd4b6] [strings] [patchbitrot] [parity-chrome]
Severity: normal → blocker
Inspector feature postponed. Removing blocking flags from Inspector bugs.
blocking2.0: beta6+ → ---
Removing items from kd4b6.
Whiteboard: [kd4b6] [strings] [patchbitrot] [parity-chrome] → [strings] [patchbitrot] [parity-chrome]
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
Attached patch Inspector context menu (obsolete) — Splinter Review
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.
Attachment #468036 - Attachment is obsolete: true
Attachment #471215 - Attachment is obsolete: true
Attachment #477639 - Flags: review?(gavin.sharp)
Attachment #471215 - Flags: review?(gavin.sharp)
Summary: Context menu item for Inspect this element (reticle) → Context menu item for Highlight Element (highlighter)
Version: Trunk → unspecified
Attachment #477639 - Flags: review?(gavin.sharp)
Attached patch context menu (obsolete) — Splinter Review
updated and unbitrotted. Initialization test seems to be failing.
Attachment #477639 - Attachment is obsolete: true
Whiteboard: [strings] [patchbitrot] [parity-chrome] → [strings] [rebased-05-27] [parity-chrome][failed-test]
Depends on: 642471
Whiteboard: [strings] [rebased-05-27] [parity-chrome][failed-test] → [minotaur]
Attached patch Highlighter context menu (obsolete) — Splinter Review
Rebased and fixed failing test.
Attachment #535741 - Attachment is obsolete: true
Priority: -- → P1
Comment on attachment 554545 [details] [diff] [review]
Highlighter context menu

asking mossop for a little review.
Attachment #554545 - Flags: review?(dtownsend)
Whiteboard: [minotaur] → [minotaur][has-patch][needs-review]
Comment on attachment 554545 [details] [diff] [review]
Highlighter context menu

Alas, I am not a browser peer
Attachment #554545 - Flags: review?(dtownsend)
(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 on attachment 554545 [details] [diff] [review]
Highlighter context menu

shifting to gavin for reviewage.
Attachment #554545 - Flags: review?(gavin.sharp)
I'll take some load off Rob's shoulders now that I'm back.
Assignee: rcampbell → past
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.
Attachment #554545 - Flags: review?(gavin.sharp) → review-
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.
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.
(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".
Attached patch Highlighter context menu v2 (obsolete) — Splinter Review
(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.
Attachment #554545 - Attachment is obsolete: true
Attachment #557610 - Flags: review?(gavin.sharp)
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.
Attachment #557610 - Flags: review?(gavin.sharp) → review-
Attached patch Highlighter context menu v3 (obsolete) — Splinter Review
(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.
Attachment #557610 - Attachment is obsolete: true
Attachment #560510 - Flags: review?(gavin.sharp)
Attached patch Diff between v2 and v3 (obsolete) — Splinter Review
Here are the changes since the previous patch in order to help you with the review.
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).
Attachment #560510 - Flags: review?(gavin.sharp) → review+
(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!
Attachment #560510 - Attachment is obsolete: true
Attachment #560511 - Attachment is obsolete: true
Whiteboard: [minotaur][has-patch][needs-review] → [minotaur][land-in-fx-team]
Whiteboard: [minotaur][land-in-fx-team] → [minotaur][fixed-in-fx-team]
Comment on attachment 561138 [details] [diff] [review]
[backed-out] Highlighter context menu v4

https://hg.mozilla.org/integration/fx-team/rev/304f93baaa1b
Attachment #561138 - Attachment description: Highlighter context menu v4 → [in-fx-team] Highlighter context menu v4
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
backed out in https://hg.mozilla.org/integration/fx-team/rev/931650933b13
Whiteboard: [minotaur][fixed-in-fx-team] → [minotaur]
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.
Attachment #561138 - Attachment description: [in-fx-team] Highlighter context menu v4 → [backed-out] Highlighter context menu v4
(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.
Attachment #561138 - Attachment is obsolete: true
Try results are green, this is ready to land.
Whiteboard: [minotaur] → [minotaur][land-in-fx-team]
Whiteboard: [minotaur][land-in-fx-team] → [minotaur][fixed-in-fx-team]
Comment on attachment 561432 [details] [diff] [review]
[in-fx-team] Highlighter context menu v5

https://hg.mozilla.org/integration/fx-team/rev/f9d487c2b076
Attachment #561432 - Attachment description: Highlighter context menu v5 → [in-fx-team] Highlighter context menu v5
https://hg.mozilla.org/mozilla-central/rev/f9d487c2b076
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Verified using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20110928 Firefox/10.0a1
Status: RESOLVED → VERIFIED
Keywords: verified-aurora
Blocks: 712871
Flags: in-testsuite+
Version: unspecified → Trunk
why are you making a mess in a bug that's been closed for 3 months? Please stop.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.