Last Comment Bug 650760 - Create Help menu for Scratchpad
: Create Help menu for Scratchpad
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools: Scratchpad (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: Firefox 13
Assigned To: Johan C
: Julian Descottes [:jdescottes]
Depends on:
  Show dependency treegraph
Reported: 2011-04-18 05:10 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2012-04-28 14:41 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch 0.1 (4.10 KB, patch)
2011-12-16 09:47 PST, Johan C
rcampbell: feedback+
Details | Diff | Splinter Review
patch 0.2 (6.42 KB, patch)
2012-02-01 17:34 PST, Johan C
no flags Details | Diff | Splinter Review
patch 0.3 (6.83 KB, patch)
2012-02-07 14:28 PST, Johan C
rcampbell: feedback+
Details | Diff | Splinter Review
patch 0.4 (6.51 KB, patch)
2012-02-09 08:20 PST, Johan C
no flags Details | Diff | Splinter Review
patch 0.5 (6.30 KB, patch)
2012-02-11 06:11 PST, Johan C
rcampbell: review+
Details | Diff | Splinter Review

Description User image Rob Campbell [:rc] (:robcee) 2011-04-18 05:10:06 PDT
The Workspace (new feature) should have a Help menu + item which takes a user to a documentation page on MDN, also to be created.
Comment 1 User image Kevin Dangoor 2011-04-21 07:43:51 PDT
Marking this dev-doc-needed so that we can work with the doc folks to get a doc page created.

The feature page is here:
Comment 2 User image Pavel Cvrcek [:JasnaPaka] 2011-08-09 14:22:31 PDT
Which items should be in Help menu?
Comment 3 User image Johan C 2011-12-16 09:47:20 PST
Created attachment 582304 [details] [diff] [review]
patch 0.1

Currently "F1" opens the documentation page on Windows. After a discussion with Rob the equivalent keyboard shortcut on mac (accel + ?) usually only opens the help menu. Any feedback on this?
Comment 4 User image Rob Campbell [:rc] (:robcee) 2011-12-16 13:11:04 PST
Comment on attachment 582304 [details] [diff] [review]
patch 0.1

yeah, I think this looks good.
Comment 5 User image Mihai Sucan [:msucan] 2012-01-17 08:42:01 PST
Triaging. Filter on PEGASUS.

This is a feature that would help newcomers to Scratchpad, especially when we have more features built into it. A quick link to MDN would be nice.
Comment 6 User image Mihai Sucan [:msucan] 2012-01-17 08:42:46 PST
Comment 7 User image Johan C 2012-02-01 17:34:53 PST
Created attachment 593672 [details] [diff] [review]
patch 0.2

+ focus browser and MDN page
+ test
Comment 8 User image Rob Campbell [:rc] (:robcee) 2012-02-03 07:21:28 PST
Comment on attachment 593672 [details] [diff] [review]
patch 0.2

+++ b/browser/devtools/scratchpad/test/
@@ -63,6 +63,7 @@
 		browser_scratchpad_bug690552_display_outputs_errors.js \
 		browser_scratchpad_bug650345_find_ui.js \
 		browser_scratchpad_bug714942_goto_line_ui.js \
+    browser_scratchpad_bug_650760_help_key.js \
 		head.js \

in your makefile, indentation is important. Makefiles like tabs.

in your test, rather than doing the linkClicked check in executeSoon blocks, I would register a listener for TabSelect and do your test in there.


win.gBrowser.tabContainer.addEventListener("TabSelect", tabOpenedTest, false);

Also, I would pull the correct key and accel combos out of the menuitem itself. You can do that via

let keyid = document.getElementById("sp-menu-documentation");
let key = document.getElemementById(keyid);

and do a lookup on the key's attributes to find any additional keyboard modifiers.
Comment 9 User image Johan C 2012-02-07 14:28:55 PST
Created attachment 595185 [details] [diff] [review]
patch 0.3

+ reworked my code in 'scratchpad.xul'
+ pull key/keycode & modifiers from menuitem
- don't rely on executeSoon()
Comment 10 User image Rob Campbell [:rc] (:robcee) 2012-02-08 12:41:42 PST
Comment on attachment 595185 [details] [diff] [review]
patch 0.3

+    let newTab = this.gBrowser.addTab(this.strings.GetStringFromName("help.openDocumentationPage"));

This line's too long. We try to keep them under 80 characters.

Maybe add a local variable for the uri string.

you have some whitespace on empty lines and in your test code:


+  content.location = "data:text/html,Test keybindings for opening Scratchpad MDN Documentation, bug 650760";
+  gBrowser.selectedBrowser.addEventListener("load", function onTabLoad() {
+    gBrowser.selectedBrowser.removeEventListener("load", onTabLoad, true);
+    /// here
+    ok(window.Scratchpad, "Scratchpad variable exists");
+    /// here
+    openScratchpad(runTest);
+  }, ...

for this section:

+  var aEvent = {
+    shiftKey: false,
+    ctrlKey: false,
+    altKey: false,
+    metaKey: false,
+    accelKey: false
+  }
+  if (modifiers.match("shift"))
+    aEvent.shiftKey = true;

you could do something like:

+  var aEvent = {
+    shiftKey: modifiers.match("shift"),

and do away with all the if checking.

you don't really need the extra isPageOpen function. You could just use:

ok(linkClicked, "MDN page will open);

and then call finishTest();

f+ with those fixes! Lookin' good.
Comment 11 User image Johan C 2012-02-09 08:20:40 PST
Created attachment 595758 [details] [diff] [review]
patch 0.4
Comment 12 User image Rob Campbell [:rc] (:robcee) 2012-02-10 08:53:37 PST
you are going to love this.

Testing this patch, Cmd-Shift-/ is activating the "Apple" menu. It's probably looking for some non-existent overlay.

Let's get rid of the extra settings and just use the F1 key on all platforms. I will cancel this review request.
Comment 13 User image Johan C 2012-02-11 06:11:20 PST
Created attachment 596314 [details] [diff] [review]
patch 0.5
Comment 14 User image Rob Campbell [:rc] (:robcee) 2012-02-17 16:28:12 PST
Comment on attachment 596314 [details] [diff] [review]
patch 0.5

looks good, I think this'll do it.

Thanks Johan!
Comment 15 User image Rob Campbell [:rc] (:robcee) 2012-02-21 03:10:34 PST
Comment 16 User image Rob Campbell [:rc] (:robcee) 2012-02-22 09:38:05 PST
Comment 17 User image Eric Shepherd [:sheppy] 2012-04-28 14:41:55 PDT

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