Closed Bug 650760 Opened 11 years ago Closed 10 years ago

Create Help menu for Scratchpad


(DevTools Graveyard :: Scratchpad, defect, P3)



(Not tracked)

Firefox 13


(Reporter: rcampbell, Assigned: johan.charlez)


(Keywords: dev-doc-complete, Whiteboard: [good-first-bugs][fixed-in-fx-team])


(1 file, 4 obsolete files)

The Workspace (new feature) should have a Help menu + item which takes a user to a documentation page on MDN, also to be created.
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:
Keywords: dev-doc-needed
Summary: Create Help menu for workspace → Create Help menu for Scratchpad
Whiteboard: [good-first-bugs]
Which items should be in Help menu?
Attached patch patch 0.1 (obsolete) — Splinter Review
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?
Attachment #582304 - Flags: feedback?(rcampbell)
Comment on attachment 582304 [details] [diff] [review]
patch 0.1

yeah, I think this looks good.
Attachment #582304 - Flags: feedback?(rcampbell) → feedback+
Assignee: nobody → johan.charlez
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.
Priority: -- → P3
Component: Developer Tools → Developer Tools: Scratchpad
QA Contact: →
Attached patch patch 0.2 (obsolete) — Splinter Review
+ focus browser and MDN page
+ test
Attachment #582304 - Attachment is obsolete: true
Attachment #593672 - Flags: feedback?(rcampbell)
Assignee: johan.charlez → nobody
Version: unspecified → Trunk
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.
Attached patch patch 0.3 (obsolete) — Splinter Review
+ reworked my code in 'scratchpad.xul'
+ pull key/keycode & modifiers from menuitem
- don't rely on executeSoon()
Attachment #593672 - Attachment is obsolete: true
Attachment #593672 - Flags: feedback?(rcampbell)
Attachment #595185 - Flags: feedback?(rcampbell)
Assignee: nobody → johan.charlez
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.
Attachment #595185 - Flags: feedback?(rcampbell) → feedback+
Attached patch patch 0.4 (obsolete) — Splinter Review
Attachment #595185 - Attachment is obsolete: true
Attachment #595758 - Flags: review?(rcampbell)
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.
Attachment #595758 - Flags: review?(rcampbell)
Attached patch patch 0.5Splinter Review
Attachment #595758 - Attachment is obsolete: true
Attachment #596314 - Flags: review?(rcampbell)
Comment on attachment 596314 [details] [diff] [review]
patch 0.5

looks good, I think this'll do it.

Thanks Johan!
Attachment #596314 - Flags: review?(rcampbell) → review+
Whiteboard: [good-first-bugs] → [good-first-bugs][land-in-fx-team]
Whiteboard: [good-first-bugs][land-in-fx-team] → [good-first-bugs][fixed-in-fx-team]
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.