Closed
Bug 587134
Opened 14 years ago
Closed 13 years ago
Context menu item for Highlight Element (highlighter)
Categories
(DevTools :: General, defect, P1)
DevTools
General
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)
29.27 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Comment 1•14 years ago
|
||
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]
Comment 2•14 years ago
|
||
taking this to go with the other menu bug.
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Comment 3•14 years ago
|
||
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?
Updated•14 years ago
|
Attachment #468036 -
Flags: review? → review?(gavin.sharp)
Updated•14 years ago
|
blocking2.0: ? → beta5+
Comment 4•14 years ago
|
||
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•14 years ago
|
||
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.
Updated•14 years ago
|
Attachment #468036 -
Flags: ui-review?(limi)
Comment 6•14 years ago
|
||
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+
Comment 7•14 years ago
|
||
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!
Updated•14 years ago
|
Whiteboard: [kd4b5] → [kd4b6]
Updated•14 years ago
|
blocking2.0: ? → beta6+
Updated•14 years ago
|
Whiteboard: [kd4b6] → [kd4b6] [strings]
Updated•14 years ago
|
Whiteboard: [kd4b6] [strings] → [kd4b6] [strings] [patchbitrot]
Updated•14 years ago
|
Attachment #468036 -
Flags: review?(gavin.sharp)
Updated•14 years ago
|
Whiteboard: [kd4b6] [strings] [patchbitrot] → [kd4b6] [strings] [patchbitrot] [parity-chrome]
Updated•14 years ago
|
Severity: normal → blocker
Comment 10•14 years ago
|
||
Inspector feature postponed. Removing blocking flags from Inspector bugs.
blocking2.0: beta6+ → ---
Comment 11•14 years ago
|
||
Removing items from kd4b6.
Whiteboard: [kd4b6] [strings] [patchbitrot] [parity-chrome] → [strings] [patchbitrot] [parity-chrome]
Comment 12•14 years ago
|
||
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
Comment 13•14 years ago
|
||
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)
Updated•13 years ago
|
Summary: Context menu item for Inspect this element (reticle) → Context menu item for Highlight Element (highlighter)
Version: Trunk → unspecified
Updated•13 years ago
|
Attachment #477639 -
Flags: review?(gavin.sharp)
Comment 14•13 years ago
|
||
updated and unbitrotted. Initialization test seems to be failing.
Attachment #477639 -
Attachment is obsolete: true
Updated•13 years ago
|
Whiteboard: [strings] [patchbitrot] [parity-chrome] → [strings] [rebased-05-27] [parity-chrome][failed-test]
Updated•13 years ago
|
Whiteboard: [strings] [rebased-05-27] [parity-chrome][failed-test] → [minotaur]
Assignee | ||
Comment 15•13 years ago
|
||
Rebased and fixed failing test.
Attachment #535741 -
Attachment is obsolete: true
Updated•13 years ago
|
Priority: -- → P1
Comment 16•13 years ago
|
||
Comment on attachment 554545 [details] [diff] [review] Highlighter context menu asking mossop for a little review.
Attachment #554545 -
Flags: review?(dtownsend)
Updated•13 years ago
|
Whiteboard: [minotaur] → [minotaur][has-patch][needs-review]
Comment 17•13 years ago
|
||
Comment on attachment 554545 [details] [diff] [review] Highlighter context menu Alas, I am not a browser peer
Attachment #554545 -
Flags: review?(dtownsend)
Comment 18•13 years ago
|
||
(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•13 years ago
|
||
Comment on attachment 554545 [details] [diff] [review] Highlighter context menu shifting to gavin for reviewage.
Attachment #554545 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 20•13 years ago
|
||
I'll take some load off Rob's shoulders now that I'm back.
Assignee: rcampbell → past
Comment 21•13 years ago
|
||
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-
Comment 22•13 years ago
|
||
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.
Assignee | ||
Comment 23•13 years ago
|
||
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•13 years ago
|
||
(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".
Assignee | ||
Comment 25•13 years ago
|
||
(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 26•13 years ago
|
||
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-
Assignee | ||
Comment 27•13 years ago
|
||
(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)
Assignee | ||
Comment 28•13 years ago
|
||
Here are the changes since the previous patch in order to help you with the review.
Comment 29•13 years ago
|
||
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+
Assignee | ||
Comment 30•13 years ago
|
||
(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
Assignee | ||
Updated•13 years ago
|
Whiteboard: [minotaur][has-patch][needs-review] → [minotaur][land-in-fx-team]
Updated•13 years ago
|
Whiteboard: [minotaur][land-in-fx-team] → [minotaur][fixed-in-fx-team]
Comment 31•13 years ago
|
||
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
Comment 32•13 years ago
|
||
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•13 years ago
|
||
backed out in https://hg.mozilla.org/integration/fx-team/rev/931650933b13
Whiteboard: [minotaur][fixed-in-fx-team] → [minotaur]
Comment 34•13 years ago
|
||
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
Assignee | ||
Comment 35•13 years ago
|
||
(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
Assignee | ||
Comment 36•13 years ago
|
||
Try results are green, this is ready to land.
Whiteboard: [minotaur] → [minotaur][land-in-fx-team]
Updated•13 years ago
|
Whiteboard: [minotaur][land-in-fx-team] → [minotaur][fixed-in-fx-team]
Comment 37•13 years ago
|
||
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
Comment 38•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f9d487c2b076
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Comment 39•13 years ago
|
||
Verified using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20110928 Firefox/10.0a1
Status: RESOLVED → VERIFIED
Keywords: verified-aurora
Updated•12 years ago
|
Comment 40•12 years ago
|
||
why are you making a mess in a bug that's been closed for 3 months? Please stop.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•