650760 - Create Help menu for Scratchpad

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P3)

Description

[Rob Campbell [:rc] (:robcee)]

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

[Kevin Dangoor]

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:

https://wiki.mozilla.org/DevTools/Features/Workspaces

Comment 2

[Pavel Cvrcek [:JasnaPaka]]

Which items should be in Help menu?

Comment 3

[Johan C]

Attachment: 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

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 582304 [details] [diff] [review]
patch 0.1

yeah, I think this looks good.

Comment 5

[Mihai Sucan [:msucan]]

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

[Mihai Sucan [:msucan]]

PEGASUS

Comment 7

[Johan C]

Attachment: patch 0.2

+ focus browser and MDN page
+ test

Comment 8

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 593672 [details] [diff] [review]
patch 0.2

+++ b/browser/devtools/scratchpad/test/Makefile.in
@@ -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.

e.g.,

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

[Johan C]

Attachment: patch 0.3

+ reworked my code in 'scratchpad.xul'
+ pull key/keycode & modifiers from menuitem
- don't rely on executeSoon()

Comment 10

[Rob Campbell [:rc] (:robcee)]

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:

e.g.,

+  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

[Johan C]

Attachment: patch 0.4

Comment 12

[Rob Campbell [:rc] (:robcee)]

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

[Johan C]

Attachment: patch 0.5

Comment 14

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 596314 [details] [diff] [review]
patch 0.5

looks good, I think this'll do it.

Thanks Johan!

Comment 15

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/3d77924fb970

Comment 16

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/3d77924fb970

Comment 17

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/Tools/Scratchpad#Using_Scratchpad