Open Bug 492557 Opened 16 years ago Updated 6 months ago

Create a keyboard shortcut HUD

Categories

(Firefox :: Keyboard Navigation, enhancement)

Product:
Firefox
Component:
Keyboard Navigation
Type:
enhancement
Points:
8

Tracking

()

People

(Reporter: davidb, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, feature, student-project)

Attachments

(6 files, 11 obsolete files)

screenshot of the feature in action...
895.87 KB, image/png
shorlander
: review-
Details
Lighter Panel Style Mockup
453.00 KB, image/png
Details
Patch with private methods and assorted bugfixes
30.78 KB, patch
mikedeboer
: review-
Details | Diff | Splinter Review
Fixed some issues in #Comment 68 .
31.71 KB, patch
mikedeboer
: feedback+
Details | Diff | Splinter Review
Added Context Menu Shortcuts as discussed.
36.29 KB, patch
Details | Diff | Splinter Review
about-shortcuts.png
92.38 KB, image/png
Details
Press "?" in a google app to see an example of this. Would folks like something like this for FF? (would need to be l10n aware). Maybe a good student project?
hello, Is this some thing that is related to the navigation as of in gmail some thing like g then i => go to inbox g then s => go to starred conversations It sounds good, I want to work on it
hi shirish, you guessed right :) I personally don't know most of the shortcuts in Firefox and it is not easy to find it in the help menu. Shall I search "shorcuts", "hot keys", "keyboard"?? In this case the help is not that useful :( I imagine this feature helping in two ways. * User presses "?" * The current website does not capture the event * Firefox then displays its own shortcuts a la gmail style * User presses "?" * The current website captures the event (like Gmail) and they could use display their own keyboard shortcuts. Could this help reveal accessibility shortcuts? Aside of just having the event, it would make sense to reveal the feature to the users by having a UI element or a menu item under the HELP menu. This bug is a feature that I would love to have but I don't know how helpful I could be for it and/or give time to it.
After giving some thought to the same, i guess most of the shortcuts are easy and are available for almost all the features, i guess this wont be of that much help that i thought it would be :(
Note that a general purpose HUD is being developed in bug 529086.
It would be cool to have this but I am fine to close it and let the general purpose bug to its thing. From my side we can close this or just let it seat until someone picks it up.
I think we can let it sit.
Note bug 529086 isn't really related (ignore my comment 4).
Hi, We are 3 french programming students looking for a 3-month project and we are interested in that one. Can it still be done?
(In reply to Timothee GARNAUD from comment #8) > Hi, > > We are 3 french programming students looking for a 3-month project and we > are interested in that one. Can it still be done? Yes please.
Whiteboard: [mentor: ???]
OK. Then we'll strat. Is there any mentor we can refer to for this project?
Mimicking the HUD from Gmail (stylistically speaking) would be a good start. Once it's working, I'm sure we can do some simple polish to make it fit well with the rest of the Firefox UI (border radius, opacity values, etc).
Keywords: uiwanted
Actually we got contacted by a french developper who can make us work on bug with HTML 5. We'll work with him. Maybe next time for the HUD. That remains interesting.
Timothee, any student interest these days?
Nop. Free to go for anyone interested.
Gavin, do you think any Fx devs would like to mentor this as a gsoc project? https://wiki.mozilla.org/Community:SummerOfCode12:Brainstorming
I mocked up an extension which adds this sort of functionality to Firefox. It's available at asperrima dot com/files/shortcuts@asperrima.com.xpi . As caveats, I've only had the opportunity to test it on my apple laptop, and a few strings are randomly broken. It represents the less complete but localization independent solution of just scraping the menus for the listed keyboard shortcuts. An alternate strategy would use the global key command set and a localized dtd file. Does anyone have any comments before I go to port this from being an extension?
(In reply to James Hobin from comment #16) > Does anyone have any comments before I go to port this from being an extension? This looks like a good first attempt. One thing I noticed is that you'll need to take into account whether the menu item has a "disabled" attribute, since some menu items are platform-specific, or dynamically disabled. There may be other corner cases to deal with. Ideally we may want to expose a useful way to get XUL-registered key event handlers in a more generic way, but that would probably be significantly more complicated to implement.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #17) Thanks for the feedback! One issue I found while testing it on a windows box was that the top right unified menu absolutely breaks the popup, so I'll be working on fixing that too.
James, how's it going?
I'm currently very busy with school, but I'm working on it whenever I have time.
This has only been tested on Mac, and almost definitely has an issue with Windows.
Attachment #621342 - Flags: review?(dbolter)
Thanks! My day is slammed but I'll at least take a look today (Monday).
Comment on attachment 621342 [details] [diff] [review] First attempt at full integration OK interesting. You've hooked it into the help menu, not to a global shortcut right? ... and not as a HUD. Still good progress! I'd recommend asking ":gavin" for review/feedback as I'm not a firefox peer.
Attachment #621342 - Flags: review?(dbolter)
James: Any further progress on this? If so, it'd be good to see your latest work and give feedback on that. I'm happy to help out as I can, and you can request feedback/review from me if you'd like.
Status: NEW → ASSIGNED
Whiteboard: [mentor: ???] → [mentor=bmcbride@mozilla.com]
Assignee: nobody → hobinjk
I have made a bit of progress (despite finals, illness, and vacation), and need feedback on a critical design question. I see two ways of going about displaying the keyboard shortcuts. The current way, a full on new window, is somewhat clunky, but can be kept around in the background for reference. Another way of doing it is how Google Docs handles it, as a window integrated within the main document, which necessarily obscures the current content and can not be kept around for reference. The current approach seems like it would be better for new users, who need the constant reference, but the Google approach is much more sleek and better for more experienced users. Which would you recommend?
I recommend the "window integrated within the document" approach. You could add an element to browser.xul so it would appear on top of the webpage content.
Why does this block 513165? The way I have it working is independent of the actual menu items that have keyboard commands.
(In reply to James Hobin from comment #27) > Why does this block 513165? The way I have it working is independent of the > actual menu items that have keyboard commands. Once we have this new way to display keyboard shortcuts, we can remove some menu items that we only keep around for users to learn the keyboard shortcuts.
This patch also has the popup not as a separate window, but as an integrated popup. It will probably not work too well on Windows using the App Menu, which is an ongoing issue that needs sorting out.
Attachment #621342 - Attachment is obsolete: true
Attachment #633404 - Flags: review?(bmcbride)
Comment on attachment 633404 [details] [diff] [review] Patch with code fully integrated into browser base Review of attachment 633404 [details] [diff] [review]: ----------------------------------------------------------------- This is pretty cool :) Some of the code style is quite different from the rest of the browser code. Have a read through https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide, but generally: * No space to the right of "(", and no space on the left of ")" * If-statements should be formatted like: if (condition) { * Functions should be formatted like: function doWhatever(param1, param2) { * Calling a function should be formatted like: doSomething(param1, param2); * Use [] to create an array, not Array() It would be good to have the code in keyboardShortcutsPopup.js contained within a single object, so we're not leaving so many functions and variables in the global scope. It's also a good way to avoid any potential naming conflicts with functions/variables. That will also let you rename functions like populateKeyboardShortcutPopup() to KeyboardShortcutPopup.populate() Instead of a <div> that covers the entire window, use a XUL <panel> that is opened in the center of the window. It's generally more suited for this (you won't need all the extra CSS to get it positioned right), and as a bonus it will automatically allow you to click outside the popup to close it (rather than needing to use a close button - although you should still keep that). And it should be a child of the mainPopupSet <popupset> - should probably just add that to browser.xul, rather than creating it every time. Though, I do like that you go through the menus and populate the table every time - since restartless extensions can add/remove menuitems at any time. The IDs of the elements are very generic - they need to be more unique, so they don't conflict with anything else (like extensions). ie, "keyboardShortcuts-popup" instead of "popup". Same goes for the CSS class names - many of the CSS selectors are very generic, and could potentially affect many elements, not just those in your popup. eg, "h3.groupheader" should be something like ".keyboardShortcuts-header". https://wiki.mozilla.org/DevTools/CSSTips is a good general reference for other things to avoid in CSS. I see various shortcuts that are either blank or are missing parts of their shortcuts (but show the joining "+"). eg: * Delete (Del) * Stop (Esc) * Fullscreen (F11) * Back (Alt+Left Arrow) * etc Those seem to be all special keys (ie, not alphanumeric). I also see "Switch Text Direction" under Edit, but the actual menuitem is hidden. The code looks like it should skip hidden items like that, but it's not working for that menuitem for some reason. The popup has a lot of <p> elements that aren't needed. Instead of creating <p> elements, you can use document.createTextNode(), or just set .textContent on the parent element (ie, the <td>). And you can set the relevant class on the <td> too. getContentGroups() returns a data structure of elements. I wonder if the code would be easier to follow it returned just the data, and let populateKeyboardShortcutPopup() create all the elements. That would also make it easier to set the classes on the <td> and <tr> elements. ::: browser/base/content/baseMenuOverlay.xul @@ +104,5 @@ > <menuitem id="aboutName" > accesskey="&aboutProduct.accesskey;" > label="&aboutProduct.label;" > oncommand="openAboutDialog();"/> > + <menuitem id="appmenu_keyboardShortcuts" Since this isn't the app-menu, the id here should be something like menu_keyboardShortcuts. Also, for the main menu, a good place for it would be right after the first help menuitem (menu_openHelp). ::: browser/base/content/browser-appmenu.inc @@ +452,5 @@ > <menuseparator/> > <menuitem id="appmenu_about" > label="&aboutProduct.label;" > oncommand="openAboutDialog();"/> > + <menuitem id="appmenu_keyboardShortcuts" In the app-menu the best place for this is probably right after "Getting Started". ::: browser/base/jar.mn @@ +86,5 @@ > * content/browser/baseMenuOverlay.xul (content/baseMenuOverlay.xul) > * content/browser/nsContextMenu.js (content/nsContextMenu.js) > + content/browser/keyboardShortcutsPopup.js (content/keyboardShortcutsPopup.js) > + content/browser/keyboardShortcutsPopup.css (content/keyboardShortcutsPopup.css) > + content/browser/close-circle.png (content/close-circle.png) Think you forgot to include close-circle.png in the patch. ::: browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd @@ +23,5 @@ > > <!ENTITY helpFeedbackPage.label "Submit Feedback…"> > <!ENTITY helpFeedbackPage.accesskey "S"> > > +<!ENTITY keyboardShortcuts.label "Show Keyboard Shortcuts"> "Keyboard Shortcuts" is enough (many menu items show something, but we don't put "Show" on them). ::: toolkit/content/macWindowMenu.inc @@ +33,5 @@ > + <menuitem command="minimizeWindow" > + key="key_minimizeWindow" > + label="&minimizeWindow.label;"/> > + <menuitem command="zoomWindow" > + label="&zoomWindow.label;"/> Does this have any other side-effects on Mac? (I'm on Windows right now, so I can't check.)
Attachment #633404 - Flags: review?(bmcbride) → review-
Summary: create a keyboard shortcut HUD? → Create a keyboard shortcut HUD
Attached patch Patch with suggested changes (obsolete) — — Splinter Review
This has most of the changes in it, but two things are still a bit off. While the popup is now a panel, it isn't opened by any Popup specific code, and is still positioned with CSS. Secondarily, I still am not sure how to make sure the special keys like Delete and F1-12 get displayed properly as it appears to be font-dependent (-moz-use-system-font might help).
Attachment #633404 - Attachment is obsolete: true
Attachment #640459 - Flags: review?(bmcbride)
Comment on attachment 640459 [details] [diff] [review] Patch with suggested changes Review of attachment 640459 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good :) (In reply to James Hobin from comment #31) > While the popup is now a panel, it isn't opened by any Popup specific code, > and is still positioned with CSS. https://developer.mozilla.org/en/XUL/PopupGuide/Positioning has some useful information on positioning where popups show. Basically, you'll need to set hidden="true" on the <panel>, and use openPopup() to show it. And call hidePopup() when the close button is clicked. > Secondarily, I still am not sure how to > make sure the special keys like Delete and F1-12 get displayed properly For those types of keys, the <key> element has a keycode attribute. For example, the "Delete" menuitem references a <key> element that has keycode="VK_DELETE". The strings for those keys are in the platformKeys.properties string bundle (ie, the one you're using in getKeyText). ::: browser/base/content/keyboardShortcutsPopup.js @@ +8,5 @@ > + modifierSeparator: null, > + shiftText: null, > + > + create: function(keyBrowserPass) { > + this.keyBrowser = keyBrowserPass; Don't need to pass in the document to this function (or store it), you can just use |document| directly instead of |this.keyBrowser| @@ +19,5 @@ > +// var popup = this.createDiv(); > +// popup.id = "keyboardShortcuts-popup"; > + > + var wrapper = document.createElement("panel");//document.createElementNS("http://www.w3.org/1999/xhtml", "div"); > + wrapper.id = "keyboardShortcuts-wrapper"; Hardcode the <panel> into browser.xul, along with the content that never changes (close button, header, etc) - saves having to regenerate all that every time here. I think it then makes more sense to call this function "open". @@ +23,5 @@ > + wrapper.id = "keyboardShortcuts-wrapper"; > + var closeCircle = document.createElementNS("http://www.w3.org/1999/xhtml", "img"); > + closeCircle.id = "close-circle"; > + closeCircle.src = "chrome://browser/content/close-circle.png"; > + closeCircle.addEventListener("click", function() { KeyboardShortcutsPopup.close(); }); Instead of an anonymous function wrapped around KeyboardShortcutsPopup.close() here, you can just use: this.close.bind(this) as the 2nd parameter. Which is the same as passing in this.close, but it makes sure |this| is what you expect it to be (this object) when the function is called. See: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Function/bind @@ +40,5 @@ > + }, > + > + onKeyPress: function(keyEvent) { > + if (keyEvent.keyCode) { > + if (keyEvent.keyCode == 27) //esc Use keyEvent.DOM_VK_ESCAPE instead of a magic number (27). @@ +46,5 @@ > + } > + }, > + > + populate: function(wrapperDiv) { > + document.addEventListener("keypress", this.onKeyPress); This should either go in open(), in which case you'll need to remove it in close(); or in an initilize() function. @@ +48,5 @@ > + > + populate: function(wrapperDiv) { > + document.addEventListener("keypress", this.onKeyPress); > + > + this.getKeyText(); This should be called in an initialize() function, so it's only ever called once. @@ +57,5 @@ > + var tableRow = this.createTableRow(); > + var contentIndex = 0; > + var rowCount = 0; > + var columnTableCell = this.createTableCell(); > + columnTableCell.className = "keyboardShortcuts-columntablecell"; You set className most times after calling createTableCell and other functions like that - perhaps those functions should take an optional parameter that sets the class name for you? @@ +63,5 @@ > + var columnTable = this.createTable(); > + columnTable.className = "keyboardShortcuts-columntable"; //each main column has its own table (makes it look better) > + columnTableCell.appendChild(columnTable); //put it in this column's td > + > + while(contentIndex < contentGroups.length) { Can convert this to a for..of loop, which simplifies it a bit (won't need contentIndex). See https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of for how they work (they're new!) @@ +67,5 @@ > + while(contentIndex < contentGroups.length) { > + var group = contentGroups[contentIndex]; > + > + //add all of the key shortcuts > + for(var j = 0; j < group.length; j++) { Same here - use a for..of loop, so you don't have to keep using group[j] (Same with other similar loops) @@ +123,5 @@ > + getKeyText: function() { > + var stringBundleService = Components.classes["@mozilla.org/intl/stringbundle;1"] > + .getService(Components.interfaces.nsIStringBundleService); > + > + var platformKeysBundle = stringBundleService.createBundle("chrome://global-platform/locale/platformKeys.properties"); Can use Services.strings instead of stringBundleService here. @@ +142,5 @@ > + var mainMenu = this.keyBrowser.getElementById("main-menubar"); > + > + for (var i = 0; i < mainMenu.childNodes.length; i++) { > + var subMenu = mainMenu.childNodes[i]; > + if ((subMenu.nodeName != "menu") && (subMenu.nodeName != "splitmenu")) { What's the purpose of this if-statement and its contents? AFAIK, the main menu shouldn't contain any splitmenus, and anything not a menu should be able to be ignored. @@ +163,5 @@ > + getMenuContent: function(subMenu) { > + //gets the content groups for a "menu" node > + var contentGroup = []; > + if (!subMenu.label) > + return contentGroup; Should return null here, and check for it in getContentGroups, to avoid adding an empty content group. @@ +166,5 @@ > + if (!subMenu.label) > + return contentGroup; > + > + var subMenuHeader = this.createHeader(subMenu.label); > + contentGroup.push({header: subMenu.label}); //add in menu header, note that first column is blanked out Since each content group only has one header, I wonder if getMenuContent() should be returning an object like: { header: "whatever", items: [a,b,c] } @@ +198,5 @@ > + continue; > + > + if(menuitem.hidden == "true") > + continue; > + if(menuitem.display == "none") .hidden will be a boolean property, not a string. And I don't think there is a .display property. I suspect what you want here is this: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js?mark=1702-1706#1702 @@ +251,5 @@ > + return content; > + }, > + > + > + getAccelText: function() { Move this function up, so it's grouped with getKeyText() - since they're related. @@ +255,5 @@ > + getAccelText: function() { > + //get the accelerator text based on platform > + > + var uiKeyPrefs = Components.classes["@mozilla.org/preferences-service;1"] > + .getService(Components.interfaces.nsIPrefService).getBranch("ui.key.") Can use Services.prefs for the prefs service. ::: browser/base/jar.mn @@ +90,5 @@ > * content/browser/baseMenuOverlay.xul (content/baseMenuOverlay.xul) > * content/browser/nsContextMenu.js (content/nsContextMenu.js) > + content/browser/keyboardShortcutsPopup.js (content/keyboardShortcutsPopup.js) > + content/browser/keyboardShortcutsPopup.css (content/keyboardShortcutsPopup.css) > + content/browser/close-circle.png (content/close-circle.png) keyboardShortcutsPopup.css and close-circle.png will eventually need to go in each of the various themes (with the later named as something like keyboardShortcuts-closeBtn.png).
Attachment #640459 - Flags: review?(bmcbride) → review-
Some additional random thoughts I had: * I don't think you need to worry about the app menu - since anything in there needs to be in the main menu anyway. * Should detect the bookmarks menu, and avoid iterating over all the bookmarks (since there may be hundreds/thousands of items there).
Sorry for the delay on this, been moving cross country. One other thing that may be a problem is that it shows the recently closed tabs and recently closed windows keyboard shortcuts. Going forward it would be good to have a retrievable list of all registered keyboard shortcuts with associated descriptions but for I think the problematic ones should either be ignored or left in.
Attachment #640459 - Attachment is obsolete: true
Attachment #652168 - Flags: review?(bmcbride)
Comment on attachment 652168 [details] [diff] [review] Patch that ignores some of bookmarks menu Review of attachment 652168 [details] [diff] [review]: ----------------------------------------------------------------- Hope the move went well, and you're settling in :) ::: browser/base/content/keyboardShortcutsPopup.js @@ +194,5 @@ > + var menuitem = subMenuPopup.childNodes[j]; > + if(menuitem.nodeName == "menu") > + subMenus.push(menuitem); > + if(menuitem.id == "bookmarksToolbarSeparator") > + break; //hack to prevent iterating through full bookmarks menu This seems reasonable. Should probably do the same for the history menu too. Is your concern with the Recent Closed Tabs/Recently Closed Windows menus that they're also mostly items without shortcuts? Thankfully those menus are limited to 10 items, so I don't think its such a big issue.
Attachment #652168 - Flags: review?(bmcbride) → review-
It looks like keyboardShortcutsPopup.js should either be a full-fledged module or #included in browser.js as browser-keyboardShortcutsPopup.js.
Have you been able to make any more progress here, James?
I was making progress, but then Firefox dropped support for gcc-4.2. My build is currently horrifically broken. I'm putting in whatever time I can to fix it.
Assignee: hobinjk → mdeboer
Note that there is no graphic attached; the close button is drawn with text and CSS. This is why it prolly needs some TLC, but I didn't want to waste effort with my sad Photoshop skills. The final design of this HUD will need an eye from a designer. The current implementation looks nice on OSX, but will it too on Win and Linux?
Attachment #652168 - Attachment is obsolete: true
Attachment #722230 - Flags: review?(bmcbride)
This builds off of Mike's awesomely shiny changes to make the key commands on Linux and Windows hopefully also look good by adding support for virtual keys (left arrow, home, delete, F11, etc.). This patch has only been tested on Linux.
Attachment #722668 - Flags: review?(bmcbride)
Comment on attachment 722247 [details] screenshot of the feature in action... This could do with some visual design input - thoughts, Stephen?
Attachment #722247 - Flags: review?(shorlander)
Comment on attachment 722668 [details] [diff] [review] Patch adding support for Virtual Keys and other edge cases Review of attachment 722668 [details] [diff] [review]: ----------------------------------------------------------------- Woo, collaboration! \o/ Hard to comment on the theme stuff at the moment, as it could use some input from shorlander. Aside from that, there's some some general stuff to do as mentioned in previous comments. eg: * Move css into theme (the base CSS stuff should go in the new /browser/theme/shared directory and be included in CSS files from the different themes) * Convert keyboardShortcutPopup.js into a JSM (I think this is most appropriate - makes it modular/re-usable, and lets us lazy-load the code) * Ignoring History menu * Most stuff (I think) mentioned in the review in comment 32 ::: browser/base/content/keyboardShortcutsPopup.js @@ +333,5 @@ > + case "VK_TAB": > + return "&#8677;"; > + } > + let keyName = keyCode.substring(3); //return the text after VK_ > + keyName = keyName.charAt(0).toLocaleUpperCase() + keyName.slice(1).toLocaleLowerCase(); Hmm, there are a few complications with this: * For the symbols used here, they may be common on OSX, but on Windows and Linux the convention is to use the word representation * For word representations of any key, it needs to be able to be localized * There's also conventions for using '+' to separate modifiers on Windows and Linux (OSX doesn't used a separator) Thankfully, this is already handled by the following string bundles: * toolkit/locales/en-US/chrome/global/keys.properties (chrome://global/locale/keys.properties) * toolkit/locales/en-US/chrome/global-platform/win/platformKeys.properties (chrome://global-platform/locale/platformKeys.properties) Note that for the likes of the arrow keys, the string bundles use the full word representation instead of the symbols you've used here. I think you should stick to what the string bundles use, for consistency with the rest of the application.
Attachment #722668 - Flags: review?(bmcbride) → feedback+
Attachment #722230 - Attachment is obsolete: true
Attachment #722230 - Flags: review?(bmcbride)
Hi James! I'm glad you like it :) Unfocused: thanks for the review, if James is OK with it, I can take a look and apply your suggestions. Meanwhile I will poke shorlander for feedback on the UI bits.
Mike, I'm fine with your applying the suggestions; I currently have midterms. Next week, if the changes still need implementing, I can volunteer much more of my time.
I'm excited to see this so close! Who has got the ball?
David: I'm basically waiting for Stephen Horlander (shorlander) to take a look at this. Then I can apply his suggestions and James' changes too.
Comment on attachment 722247 [details] screenshot of the feature in action... This looks great! It will be really awesome to have this built in. While using the patch I did find some UI issues (aside from what Blair mentioned): * Multiple instances of the overlay can stack; should only be one possible at a time — http://cl.ly/image/1Y35331Q1d2p * Text in the panel gets a selection cursor but apparently can't be selected * Has what looks like a draggable titlebar… - but it is docked to the inside of the window instead of it's own panel which can create some weirdness when resizing to a small window: http://cl.ly/image/2M2w3B430G0W - blocks part of the browser UI and the content without blocking interactions on either; puts it into a weird half-way state * Visual style: - we have changed all of our overlay panels from dark to light; this should be closer to the panels; e.g. Downloads Panel - Close icon should use /toolkit/themes/<themeName>/global/icons/close.png - Should also respect platform convention for close location; left OS X, right Window and most Linuxes - Could probably use a little more space between the bottom and top of categories Given the above issues it currently feels like it is half-way between a persistent panel and a transient overlay. I think we should pick one or the other. I would recommend going the transient overlay route and making it truly transient; i.e. dismiss it when you click on content or another UI element. It can continue to partially overlay chrome and content but should not overlay the chrome by much. You should still be able to access locationBar/toolbar items. If we were to go with a persistent panel instead then it should be draggable and independent of the main browser window.
Attachment #722247 - Flags: review?(shorlander) → review-
Mockup to go with my review comment
Stephen: awesome! I can't wait to start working on this :) James: how's your schedule atm? can we meet up on IRC to talk business? :)
My schedule is relatively open for the next week, when are you available?
James: this week I'm in Mountain View (time: PT) and next week I'm back in Holland (time: GMT+1). If you join the #fx-team channel on IRC, I'll definitely notice you!
Large areas of the code can still be improved based on comment #32 Still to do: Fix multiple stacking Move css into theme Convert keyboardShortcutPopup.js into a JSM (unsure how to do this, will research) Stop using keyBrowserPass Potentially convert to use openPopup and hidePopup Hardcode the panel and button things into browser.xul Use keyEvent.DOM_VK_ESCAPE where necessary Differentiate populate from initialize Potentially add className to creation utilities Use for..of loops Use Services.strings where possible Use Services.prefs Group functions logically and comment more
Attachment #722668 - Attachment is obsolete: true
Attachment #749753 - Flags: feedback?(mdeboer)
Attachment #749753 - Flags: feedback?(bmcbride)
Comment on attachment 749753 [details] [diff] [review] New patch implementing most of the UI/UX Review of attachment 749753 [details] [diff] [review]: ----------------------------------------------------------------- We're well underway here :) Things I don't see, including things you already mentioned: * Use of stock close button graphic * Grayed out disabled shortcut items * Title font size is too large, for the HUD title AND the section titles * Align the close button dependent on platform (to the right on your system) The transience as Stephen described is indeed an important next step. ::: browser/base/content/baseMenuOverlay.xul @@ +60,5 @@ > onclick="checkForMiddleClick(this, event);"/> > #endif > + <menuitem id="menu_keyboardShortcuts" > + label="&keyboardShortcuts.label;" > + oncommand="KeyboardShortcutsPopup.create(document);"/> Would be nice if we'd have an access key for this as well. Not having this feature accessible with a keyboard would be kind of, well, ironic ;) ::: browser/base/content/browser.xul @@ +9,5 @@ > <?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?> > <?xml-stylesheet href="chrome://browser/content/places/places.css" type="text/css"?> > <?xml-stylesheet href="chrome://browser/skin/devtools/common.css" type="text/css"?> > <?xml-stylesheet href="chrome://browser/skin/" type="text/css"?> > +<?xml-stylesheet href="chrome://browser/content/keyboardShortcutsPopup.css" type="text/css"?> When the move to skins is made, this line can be removed. ::: browser/base/content/keyboardShortcutsPopup.css @@ +1,4 @@ > +#keyboardShortcuts-wrapper { > + padding: 0 0 1em 0; > + background-color: -moz-dialog; /* rgba(242,241,240,0.98); */ > + /* border: 1px solid rgba(86, 86, 86, 0.87); */ you can remove unused styles... the old ones are still available for you in obsolete patches. @@ +31,5 @@ > + text-align: center; > +} > + > +#keyboardShortcuts-title .close-circle { > + background-image: url(moz-icon://stock/gtk-close?size=button); this is very OS-specific. I recommend moving the styles to the appropriate theme directories as soon as possible. A propos, do you have Windows/ OSX machines around? If not I can take the themeing for those OSes. ::: browser/base/content/keyboardShortcutsPopup.js @@ +1,1 @@ > +/* -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ hinting is good, but are you really using a tab-width of 4? @@ +176,5 @@ > + > + this.specialKeysBundle = stringBundleService.createBundle("chrome://global/locale/keys.properties"); > + }, > + > + //returns html content in groups (for each menu dropdown), i.e. shortcuts strings and shortcut headers nit: please use comments that start with a space and capitalize sentences. This applies for all comments present in this file. @@ +319,5 @@ > + return; > + } > + > + let key = keyNode.getAttribute("key").toLocaleUpperCase(); > + if (key.length === 0) { if (!key.length) would also be fine @@ +322,5 @@ > + let key = keyNode.getAttribute("key").toLocaleUpperCase(); > + if (key.length === 0) { > + key = keyNode.getAttribute("keytext").toLocaleUpperCase(); > + } > + if (key.length === 0) { same here, can you also add a comment here why this might be empty again? @@ +333,5 @@ > + //ignore keycommands without modifiers > + if (modifiers.length > 0) { > + modifiers = modifiers.split(/[, |]+/g); > + } > + for (let k = 0; k < modifiers.length; k++) { Instead of using a loop and switch like this, I generally prefer using a lookup table for this kind of pattern substitution: let map = { "meta": this.metaText, "control": this.controlText, "shift": this.shiftText, "alt": this.altText, "accel": this.accelText }; // ... if (modifiers.length) { modifiers = modifiers.split(/[, |]+/g).map(function(mod) { if (map[mod]) return map[mod]; return mod; }); } // done! @@ +381,5 @@ > + //virtual key code > + getStringFromKeyCode: function(keyCode) { > + let str = this.specialKeysBundle.GetStringFromName(keyCode); > + return str; > + /* Why did you comment this out? Would be cool if you'd place a short sentence explaining stuff when you do that. Makes life easier for the reviewer! (same goes for the other disabled bits of code) @@ +405,5 @@ > + let keyName = keyCode.substring(3); //return the text after VK_ > + keyName = keyName.charAt(0).toLocaleUpperCase() + keyName.slice(1).toLocaleLowerCase(); > + return keyName;*/ > + }, > + createDiv: function() { Wouldn't it be nicer to have one function called createElement and using it for all elements created in this file? const NS_XHTML = "http://www.w3.org/1999/xhtml"; function createElement(aName, aText) { let el = document.createElementNS(NS_XHTML, aName); if (aText) el.textContent = aText; return el; } And yes, this doesn't need to be a member function of this struct. ::: browser/base/jar.mn @@ +108,5 @@ > * content/browser/web-panels.xul (content/web-panels.xul) > * content/browser/baseMenuOverlay.xul (content/baseMenuOverlay.xul) > * content/browser/nsContextMenu.js (content/nsContextMenu.js) > + content/browser/keyboardShortcutsPopup.js (content/keyboardShortcutsPopup.js) > + content/browser/keyboardShortcutsPopup.css (content/keyboardShortcutsPopup.css) this will also change when you move styling to themes. ::: browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd @@ +30,5 @@ > > <!ENTITY helpFeedbackPage.label "Submit Feedback…"> > <!ENTITY helpFeedbackPage.accesskey "S"> > > +<!ENTITY keyboardShortcuts.label "Keyboard Shortcuts"> Would be nice if we'd have a command key for this as well. Not having this feature accessible with a keyboard would be kind of, well, ironic ;)
Attachment #749753 - Flags: feedback?(mdeboer) → feedback+
Comment on attachment 749753 [details] [diff] [review] New patch implementing most of the UI/UX Review of attachment 749753 [details] [diff] [review]: ----------------------------------------------------------------- Wonderful feedback pass by Mike, don't think I'm needed on this revision :)
Attachment #749753 - Flags: feedback?(bmcbride)
Attached patch Patch with Mike's feedback (obsolete) — — Splinter Review
The key combination to open the popup is accel-shift-K, which might overlap something on Windows or Linux. Everything else should work and be suitably shiny.
Attachment #749753 - Attachment is obsolete: true
Attachment #766351 - Flags: review?(mdeboer)
James, thanks for working on this! I wasn't able to review your patch last week, because I was on holiday in Greece. Thanks for your patience!
Comment on attachment 766351 [details] [diff] [review] Patch with Mike's feedback Review of attachment 766351 [details] [diff] [review]: ----------------------------------------------------------------- I did not investigate the look and feel of the popup yet, but it seems like you still have enough to do for a next iteration of this patch ;) Did you check how the popup looks in RTL mode (Right-To-Left)? To check this, you can use the following add-on: https://addons.mozilla.org/en-US/firefox/addon/force-rtl/ Thanks! We're getting a lot closer! ::: browser/base/content/browser-sets.inc @@ +34,5 @@ > <command id="cmd_closeWindow" oncommand="BrowserTryToCloseWindow()"/> > <command id="cmd_ToggleTabsOnTop" oncommand="TabsOnTop.toggle()"/> > <command id="cmd_CustomizeToolbars" oncommand="BrowserCustomizeToolbar()"/> > <command id="cmd_quitApplication" oncommand="goQuitApplication()"/> > + <command id="cmd_keyboardShortcuts" oncommand="KeyboardShortcutsPopup.create();"/> I'd go for a method name that is more descriptive of what the command does... How about `toggle()`? ::: browser/base/content/browser.xul @@ +8,5 @@ > > <?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?> > <?xml-stylesheet href="chrome://browser/content/places/places.css" type="text/css"?> > <?xml-stylesheet href="chrome://browser/skin/devtools/common.css" type="text/css"?> > +<?xml-stylesheet href="chrome://browser/skin/keyboardShortcutsPopup.css" type="text/css"?> Is it strictly necessary to declare the stylesheet explicitly here? I think you can remove it. ::: browser/base/content/keyboardShortcutsPopup.js @@ +16,5 @@ > + } > + > + let wrapper = document.createElement("panel"); > + wrapper.id = "keyboardShortcuts-wrapper"; > + let titleBar = document.createElementNS("http://www.w3.org/1999/xhtml", "div"); Please use the new this.createElement() @@ +29,5 @@ > + this.populate(wrapper); > + > + titleBar.innerHTML = "<span class=\"close-circle\"></span>" + > + "<h3>" + this.popupTitle + "</h3>"; > + console.log("firstw: "+window.getComputedStyle(wrapper)); Please remove all `console.log` statements from this file @@ +69,5 @@ > + populate: function(wrapperDiv) { > + // Forces update of edit menu visibility since it is special-cased elsewhere > + // This is necessary because running goUpdateCommand on certain menuitems > + // (e.g. newtab, close window) arbitrarily disables them > + goUpdateCommand("cmd_undo"); I'd suggest to write this as ```javascript const commands = ["cmd_undo", "cmd_redo", "cmd_cut", "cmd_copy", "cmd_paste", "cmd_selectAll", "cmd_delete", "cmd_switchTextDirection"]; for (let cmd of commands) { goUpdateCommand(cmd); } ``` @@ +92,5 @@ > + // Put it in this column's td > + columnTableCell.appendChild(columnTable); > + > + for (let contentGroup of contentGroups) { > + nit: please remove this newline @@ +104,5 @@ > + // Second cell is shortcut label for shortcuts and header for the header > + if (group.header) { //append nothing to first, header to second > + let header = this.createElement("h3", "keyboardShortcuts-groupheader"); > + header.textContent = group.header; > + secondColumnCell.appendChild( header ); nit: please omit the spaces between brace and argument: `(header)` @@ +144,5 @@ > + wrapperDiv.appendChild(table); > + }, > + > + fetchStrings: function() { > + let platformKeysBundle = Services.strings.createBundle("chrome://global-platform/locale/platformKeys.properties"); This function is executed each time the popup is shown, which is not something we'd want... Can you introduce an `init` function that does all the generic tasks (like fetching necessary string) that is called on popup.show(). Once initialized, set a boolean flag on the object to make sure the code inside `init` is not executed twice: ```javascript init: function() { if (this._initialized) { return; } let platformKeysBundle = ... ... this._initialized = true; } ``` @@ +190,5 @@ > + return contentGroups; > + }, > + > + getMenuContent: function(subMenu) { > + //gets the content groups for a "menu" node nit: please start comments with `//<space>Uppercase` @@ +363,5 @@ > + return el; > + }, > + > + close: function() { > + document.removeEventListener("keypress", KeyboardShortcutsPopup.onKeyPress); Each time the popup is closed, the entire DOM node is removed. I think that is rather wasteful and unnecessary. Please move the main popup creation to an `init` function and toggle its visibility with `hidden = true;`. Removing event listeners here is good. ::: browser/themes/linux/jar.mn @@ +29,5 @@ > skin/classic/browser/identity-icons-https.png > skin/classic/browser/identity-icons-https-ev.png > skin/classic/browser/identity-icons-https-mixed-active.png > skin/classic/browser/Info.png > +* skin/classic/browser/keyboardShortcutsPopup.css (keyboardShortcutsPopup.css) nit: you can put this line at the bottom of this file ::: browser/themes/osx/jar.mn @@ +36,5 @@ > skin/classic/browser/identity-icons-https-ev@2x.png > skin/classic/browser/identity-icons-https-mixed-active.png > skin/classic/browser/identity-icons-https-mixed-active@2x.png > skin/classic/browser/Info.png > +* skin/classic/browser/keyboardShortcutsPopup.css (keyboardShortcutsPopup.css) same nit here ::: browser/themes/shared/keyboardShortcutsPopup.inc.css @@ +1,3 @@ > +#keyboardShortcuts-wrapper { > + padding: 0 0 1em 0; > + background-color: -moz-dialog; /* rgba(242,241,240,0.98); */ please remove all commented-out rules from this file. @@ +5,5 @@ > + border-radius: 3px; > + box-shadow: 0 0 3px 3px rgba(0, 0, 0, 0.2); > + /* border-bottom: 1px solid rgba(66, 66, 66, 0.87); > + border-top: none; */ > + color: black; perhaps this is not necessary and are the system colors already good enough? @@ +7,5 @@ > + /* border-bottom: 1px solid rgba(66, 66, 66, 0.87); > + border-top: none; */ > + color: black; > + position: fixed; > + top: 80px; can you explain this 80px value? @@ +16,5 @@ > + -moz-appearance: none; > + -moz-user-select: text; > +} > + > +#keyboardShortcuts-title h3 { Can you replace all descendant selectors with child selectors in this file? For more background, please read https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS (worth the read, regardless of this bug!) @@ +30,5 @@ > + height: 14px; > + text-align: center; > +} > + > +#keyboardShortcuts-title .close-circle { Nit: please rename this class name - a close button does not necessarily imply a circle - to `close-button`. @@ +32,5 @@ > +} > + > +#keyboardShortcuts-title .close-circle { > + background-image: url("chrome://global/skin/icons/close.png"); > + color: black; no need to specify text color here. @@ +49,5 @@ > +#keyboardShortcuts-title .close-circle:hover { > + opacity: 1; > +} > + > +.keyboardShortcuts-keycommand, .keyboardShortcuts-keycommand-disabled { nit: when you combine rules, please put each respective rule on its own line. @@ +51,5 @@ > +} > + > +.keyboardShortcuts-keycommand, .keyboardShortcuts-keycommand-disabled { > + text-align: right; > + color: #ff7f00; would it be possible to use a system color here, like 'HighlightText' ? @@ +64,5 @@ > + text-align: left; > +} > + > +.keyboardShortcuts-keycommand-disabled { > + color: #777; would it be possible to use a system color here, like 'GrayText' ? @@ +68,5 @@ > + color: #777; > +} > + > +.keyboardShortcuts-keylabel-disabled { > + color: #777; please combine this rule with the one above. @@ +74,5 @@ > + > +.keyboardShortcuts-groupheader { > + font-weight: normal; > + font-size: 13px; > + color: #777; would it be possible to use a system color here, like 'GrayText' ? ::: browser/themes/windows/jar.mn @@ +32,5 @@ > skin/classic/browser/identity-icons-generic.png > skin/classic/browser/identity-icons-https.png > skin/classic/browser/identity-icons-https-ev.png > skin/classic/browser/identity-icons-https-mixed-active.png > +* skin/classic/browser/keyboardShortcutsPopup.css (keyboardShortcutsPopup.css) same nit here.
Attachment #766351 - Flags: review?(mdeboer) → review-
Attachment #766351 - Flags: review- → feedback+
Depends on: 896918
No longer blocks: 513165
Attached patch Patch with further improvements (obsolete) — — Splinter Review
I implemented nearly all of the feedback. However, I found that removing the stylesheet tag from browser.xul caused the popup to not be styled, as the file isn't included anywhere else. I think I could dynamically add it to the popup when it's initialized, but that seems a bit clunky. As far as the 80 pixel value, it's used to put the popup below the navigation bar and into the content area. A possible route of improvement is dynamically determining how offset the mainPopupSet is from where we want the popup to be, but I think the magic value is the lesser evil.
Attachment #766351 - Attachment is obsolete: true
Attachment #783512 - Flags: review?(mdeboer)
Comment on attachment 783512 [details] [diff] [review] Patch with further improvements Review of attachment 783512 [details] [diff] [review]: ----------------------------------------------------------------- James, I applied the patch locally first to check the visual appearances and I noticed a few tings that are off when I compare the current state to the shorlanders' mockup in attachment 733937 [details]: * There's some padding missing on the right and left side of the panel * The panel is positioned somewhere near the middle of the browser window, whereas it needs to overlap the toolbar a bit (negative margin from the browser element/ area) * On OSX, there 'Del' key is also represented by a symbol (among many others: http://macbiblioblog.blogspot.nl/2005/05/special-key-symbols.html). For this to work you can add an entry in mac/platformKeys.properties: VK_DELETE=\u232b and fetch it from there. But we can also do that in a follow-up if you wish!
Attachment #783512 - Flags: review?(mdeboer) → feedback+
Attached patch Patch with improved display (obsolete) — — Splinter Review
Thanks for the feedback! This patch makes the panel look closer to the mockup, and adds special support for the delete and tab keys. I've noticed that it doesn't include shortcuts like going back and switching tab groups. Further work should be done to support these ones without a menu-item, either by adding a keyboard-shortcuts-only tag (which I've seen for a few items already), or by creating that API for discovering which commands are active.
Attachment #783512 - Attachment is obsolete: true
Attachment #788587 - Flags: review?(mdeboer)
Comment on attachment 788587 [details] [diff] [review] Patch with improved display Review of attachment 788587 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/keyboardShortcutsPopup.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public As a more generic comment on this file: It looks like most of the methods and properties of `KeyboardShortcustPopup` are not supposed to be used outside of the scope of this module; only the methods `init`, `toggle` and `close` will be accessed from outside. This is why I think it's good to work with a function scope (a closure) that returns an API object for KeyboardShortcutsPopup. Example: ``` let KeyboardShortcutsPopup = (function() { const kMaxShortcutsPerRow = 16; const kNSXHTML = "http://www.w3.org/1999/xhtml"; let modText = null; let modifierSeparator = null; let popupTitle = null; let wrapperNode = null; function createElement(tagName, className) { let el = document.createElementNS(kNSXHTML, tagName); if (className) el.className = className; return el; } // ... other private methods go here ... // The public API that can (and will) be consumed outside of this scope: return { init: function() { if (initialized) return; // ...etc }, toggle: function() { this.init(); // ...etc }, close: function() { // body goes here } }; })(); ``` Note that this doesn't mean you have to throw away this implementation, it's just a refinement of the module structure. Also note that I'm suggesting to remove the `initialized` variable to keep track of state, but instead just check if `wrapperNode` is set, formerly known as `this.wrapper`. The pattern I presented above is very common to use for singleton classes in Javascript. @@ +42,5 @@ > + this.wrapper.style.marginLeft = left + "px"; > + this.wrapper.style.display = "none"; > + this._initialized = true; > + }, > + nit: one newline too many @@ +47,5 @@ > + > + toggle: function() { > + this.init(); > + > + if (this.wrapper.style.display !== "none") { use `!=` here @@ +59,5 @@ > + document.addEventListener("keypress", this.onKeyPress); > + }, > + > + onKeyPress: function(keyEvent) { > + if (keyEvent.keyCode && keyEvent.keyCode === keyEvent.DOM_VK_ESCAPE) { use `==` here @@ +66,5 @@ > + }, > + > + onClick: function(clickEvent) { > + clickEvent.stopPropagation(); > + if (clickEvent.target && clickEvent.target.id === "keyboardShortcuts-close-button") { use `==` here @@ +110,5 @@ > + let secondColumnCell = this.createElement("td"); > + > + // First cell is shortcut command for shortcuts or blank for header > + // Second cell is shortcut label for shortcuts and header for the header > + if (group.header) { //append nothing to first, header to second nit: properly format this comment to `// Append (...)` @@ +152,5 @@ > + table.appendChild(tableRow); > + wrapperDiv.appendChild(table); > + }, > + > + fetchStrings: function() { Please add protection for loading this twice ``` if (this._platformKeysBundle) return; ``` @@ +170,5 @@ > + > + this.specialKeysBundle = Services.strings.createBundle("chrome://global/locale/keys.properties"); > + }, > + > + // Returns html content in groups (for each menu dropdown), i.e. shortcuts strings and shortcut headers Please use Javadoc-style comments to document whole functions @@ +178,5 @@ > + let contentGroups = []; > + let mainMenu = document.getElementById("main-menubar"); > + > + for (let subMenu of mainMenu.childNodes) { > + if ((subMenu.nodeName != "menu") && (subMenu.nodeName != "splitmenu")) { No need to wrap these comparisons in braces. @@ +181,5 @@ > + for (let subMenu of mainMenu.childNodes) { > + if ((subMenu.nodeName != "menu") && (subMenu.nodeName != "splitmenu")) { > + // Traverse downwards to skip hboxes and other problematic things > + subMenu = null; > + for (let node = mainMenu.nextChild; node != null; node = node.firstChild) { It seems like writing this as a do...while loop is more correct: ``` let node = mainMenu.nextChild; do { if (node.nodeName != "menu" && node.nodeName != "splitmenu") continue; subMenu = node; break; } while (node = node.firstChild); ``` @@ +182,5 @@ > + if ((subMenu.nodeName != "menu") && (subMenu.nodeName != "splitmenu")) { > + // Traverse downwards to skip hboxes and other problematic things > + subMenu = null; > + for (let node = mainMenu.nextChild; node != null; node = node.firstChild) { > + if ((node.nodeName != "menu") && (node.nodeName != "splitmenu")) { ... my example shows that this double-bracing is unnecessary @@ +205,5 @@ > + if (!subMenu.label) { > + return contentGroup; > + } > + > + // add in menu header, note that first column is blanked out nit: properly capitalize this comment @@ +210,5 @@ > + contentGroup.push({header: subMenu.label}); > + > + let subMenuPopup = null; > + for (let childNode of subMenu.childNodes) { > + // traverse the children of the menu to find the menu popup (most likely the first child) nit: same here. @@ +214,5 @@ > + // traverse the children of the menu to find the menu popup (most likely the first child) > + if (childNode.nodeName != "menupopup") { > + continue; > + } > + // add in key commands, the child node is a menupopup nit: ...and here @@ +218,5 @@ > + // add in key commands, the child node is a menupopup > + contentGroup = contentGroup.concat(this.getMenuPopupContent(childNode)); > + } > + if (contentGroup.length <= 1) { > + return []; //didn't find any shortcuts for a header nit: properly format this comment like `// Didn't (...)` We tend to think that comments are usually important enough to deserve their own line. Can you make sure of that throughout this file? @@ +236,5 @@ > + subMenus.push(menuitem); > + continue; > + } > + > + if (menuitem.id === "bookmarksToolbarSeparator") { No `===` necessary here. @@ +241,5 @@ > + // Hack to prevent iterating through full bookmarks menu > + break; > + } > + > + if (menuitem.nodeName !== "menuitem") { ditto @@ +245,5 @@ > + if (menuitem.nodeName !== "menuitem") { > + continue; > + } > + > + if (menuitem.hidden === "true") { ditto @@ +249,5 @@ > + if (menuitem.hidden === "true") { > + continue; > + } > + > + if (typeof menuitem.display !== "undefined" && menuitem.display === "none") { ditto @@ +254,5 @@ > + continue; > + } > + > + let keyLabelString = menuitem.getAttribute("label"); > + if (menuitem.id === "menu_keyboardShortcuts") { ditto @@ +260,5 @@ > + this.popupTitle = keyLabelString; > + } > + > + let keyId = menuitem.getAttribute("key"); > + if (keyId.length === 0) { you can write this as `if (!keyId.length) {` @@ +294,5 @@ > + > + getAccelText: function() { > + // Get the accelerator key text based on platform > + let uiKeyPrefs = Services.prefs.getBranch("ui.key."); > + let accelKey = uiKeyPrefs.getIntPref("accelKey"); Please simplify this to ``` let accelKey = Services.prefs.getIntPref("ui.key.accelKey"); ``` @@ +303,5 @@ > + return this.modText["alt"]; > + case KeyEvent.DOM_VK_CONTROL: > + default: > + return this.modText["control"]; > + } I don't think this logic deserves a switch...case block. You can write this with an if...else like: ``` if (accelKey == KeyEvent.DOM_VK_META) return this.modText["meta"]; if (accelKey == KeyEvent.DOM_VK_ALT) return this.modText["alt"]; return this.modText["control"]; ``` @@ +314,5 @@ > + return; > + } > + > + let key = keyNode.getAttribute("key").toLocaleUpperCase(); > + if (key.length === 0) { You can write this as `if (!key.length) {` @@ +317,5 @@ > + let key = keyNode.getAttribute("key").toLocaleUpperCase(); > + if (key.length === 0) { > + key = keyNode.getAttribute("keytext").toLocaleUpperCase(); > + } > + if (key.length === 0) { same here @@ +319,5 @@ > + key = keyNode.getAttribute("keytext").toLocaleUpperCase(); > + } > + if (key.length === 0) { > + key = this.getStringFromKeyCode( > + keyNode.getAttribute("keycode") nit: I'm seeing four-spaces indentation here. Convert it to two or put the whole statement on one line. @@ +339,5 @@ > + > + let keyCommandString = key; > + if (modifiers.length > 0) { > + keyCommandString = modifiers.join(this.modifierSeparator) + > + this.modifierSeparator + key; nit: please align this line properly @@ +346,5 @@ > + return keyCommandString; > + }, > + > + getKeyCommandDisabled: function(commandId) { > + if (commandId.length === 0) { Please write this as `if (!commandId.length) {` @@ +358,5 @@ > + return commandNode.getAttribute("disabled"); > + }, > + > + getStringFromKeyCode: function(keyCode) { > + // Returns a string representation for a given virtual key code This looks like you're documenting a method. Can you put this above the method in a javadoc style comment? @@ +367,5 @@ > + let str = keyCode; > + try { > + str = this.platformKeysBundle.GetStringFromName(keyCode); > + } catch(e) { > + } nit: this dangling brace can be put next to its sibling on the same line. @@ +371,5 @@ > + } > + > + // If the string either doesn't exist or is the same as it was before, the bundle > + // didn't have it so we look for it in the global special keys bundle > + if(!str || str === keyCode) { nit: please add a space here: `if (...`
Attachment #788587 - Flags: review?(mdeboer) → feedback+
James, we're getting so much closer! This looks really nice and complete when I ran this on my mac. Great work! If you have any questions regarding my feedback above, please let me know. This part is about getting the Javascript code part exactly right.
(In reply to James Hobin from comment #62) > I've noticed that it doesn't include shortcuts like going back and switching > tab groups. Further work should be done to support these ones without a > menu-item, either by adding a keyboard-shortcuts-only tag (which I've seen > for a few items already), or by creating that API for discovering which > commands are active. Since bug 513165, bug 513168 and bug 519937 landed with a dependency on bug 896918, those shortcuts being exposed should now be a requirement for this patch to land.
This patch incorporates all of Mike's feedback from #63, moving all methods but init and toggle to private in addition to some other more minor changes. A major unprompted improvement in this version is that the popup will now adjust where it is positioned to take into account whether or not the menubar is visible, which was a problem on Ubuntu. This patch also fixes a mistyping which prevented "Keyboard Shortcuts" itself from being included in the Keyboard Shortcuts popup. Finally, the shortcut is now Option-Shift-K. Sadly, this gets captured by a few websites, including BugZilla. It may do something strange on Mac, but this is currently untested. Tab group switching, back, forward, and other keyboard shortcuts are still missing. Should the visibility of these be spun off to further bugs or dealt with here?
Attachment #788587 - Attachment is obsolete: true
Attachment #804251 - Flags: review?(mdeboer)
(In reply to James Hobin from comment #66) > Finally, the shortcut is now Option-Shift-K. Sadly, this gets captured by a > few websites, including BugZilla. It may do something strange on Mac, but > this is currently untested. Alt+Shift is reserved for access keys in web content. You'll need a different combination. I'm not even sure we need a keyboard shortcut for this, though. > Tab group switching, back, forward, and other keyboard shortcuts are still > missing. Should the visibility of these be spun off to further bugs or dealt > with here? See comment 65. I think we should solve that before landing this.
Comment on attachment 804251 [details] [diff] [review] Patch with private methods and assorted bugfixes Review of attachment 804251 [details] [diff] [review]: ----------------------------------------------------------------- The new module structure looks good, James! I do have a couple of comments on it, but nothing major. What Dão mentions in comment 65 and comment 67 _is_ major, I'm afraid. Can you look into making these 'hidden' shortcuts available in the HUD? ::: browser/base/content/keyboardShortcutsPopup.js @@ +4,5 @@ > + > +"use strict"; > + > +let KeyboardShortcutsPopup = (function() { > + const maxRowsPerColumn = 20; Please rename this to `kMaxRowsPerColumn` - this is a convention we use. @@ +13,5 @@ > + popupTitle = null; > + > + let modText = null, > + modifierSeparator = null, > + specialKeysBundle = null; Separate all these variable declarations like ```js let wrapperNode = null; let popupTitle = null; ... ``` Then you also won't need the newline between `popupTitle` and `modText`. @@ +53,5 @@ > + let browserTop = document.getElementById("browser") > + .getBoundingClientRect().top; > + wrapperNode.style.top = browserTop + "px"; > + > + wrapperNode.style.display = "none"; XUL elements can use the `hidden` attribute, which is better. Please move up and rewrite to: ```js wrapperNode.id = "keyboardShortcuts-wrapper"; wrapperNode.setAttribute("hidden", "true"); ``` @@ +59,5 @@ > + > + function toggle() { > + init(); > + > + if (wrapperNode.style.display != "none") { You can then rewrite this to ```js if (!wrapperNode.hidden) { close(); return; } ``` @@ +64,5 @@ > + close(); > + return; > + } > + > + wrapperNode.style.display = ""; and this to ```js wrapperNode.removeAttribute("hidden"); ``` @@ +390,5 @@ > + > + function getStringFromKeyCode(keyCode) { > + // Returns a string representation for a given virtual key code > + // Previously used platform-specific switch-case > + let str = specialKeysBundle.GetStringFromName(keyCode); The fact that this function used to be different is not enough to keep this function around, IMHO. Please remove it and replace all invocations with `specialKeysBundle.GetStringFromName`. @@ +404,5 @@ > + > + function close() { > + document.removeEventListener("keypress", KeyboardShortcutsPopup.onKeyPress); > + document.removeEventListener("click", KeyboardShortcutsPopup.onWindowClick); > + let popup = document.getElementById("keyboardShortcuts-wrapper"); I *think* you already have a reference to the wrapperNode. If you don't, you can bail out here. ```js if (!wrapperNode) return; ``` @@ +406,5 @@ > + document.removeEventListener("keypress", KeyboardShortcutsPopup.onKeyPress); > + document.removeEventListener("click", KeyboardShortcutsPopup.onWindowClick); > + let popup = document.getElementById("keyboardShortcuts-wrapper"); > + popup.removeEventListener("click", KeyboardShortcutsPopup.onClick); > + popup.style.display = "none"; Here you can do ```js wrapperNode.setAttribute("hidden", "true"); ``` again
Attachment #804251 - Flags: review?(mdeboer) → review-
Assignee: mdeboer → hobinjk
Depends on: 927605
Keywords: feature
Flags: firefox-backlog?
I think we should work to get this shipped as soon as reasonably possible, so granting fx-backlog. James, do you think you'll have time to work on this in the near future? :-)
Flags: needinfo?(hobinjk)
Flags: firefox-backlog?
Flags: firefox-backlog+
Perfect timing! I just started my summer time off from college and have some time to work on this upcoming week. Sadly (well, not so sadly, quite happily in fact), after that I will be starting as an intern on the Firefox OS team. I am not sure how Gregor feels about sharing, but if this feature somehow still defies me I am willing to provide any documentation required to ease a hand-off. I anticipate most of the remaining work to be hunting down the keyboard shortcuts implemented using event listeners. I have not performed a comprehensive survey, but I believe that these listeners will nearly all have logic easily mapped into <key>, <command>, and <menuitem> elements, making the process trivial.
Flags: needinfo?(hobinjk)
Mentor: bmcbride
Whiteboard: [mentor=bmcbride@mozilla.com]
Assignee: hobinjk → nobody
Hi David sir, Gijs sir, I see this has been inactive for quite a while now. I would like to take this up as a 2 Month Project which is a requirement of my school ( need a certification for the same ). I see good amount of progress has been made on this and have gone through most of the comments and got confused on what all has been completed and what all is left. Sir, please share some pointers on the same and if its possible for me to take this up and start working immediately.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dbolter)
Mike knows more than me here.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mdeboer)
Mentor: bmcbride → mdeboer
Points: --- → 8
Flags: needinfo?(mdeboer) → qe-verify+
Status: ASSIGNED → NEW
(In reply to Manraj Singh [:manrajsingh] from comment #71) > Hi David sir, Gijs sir, > > I see this has been inactive for quite a while now. I would like to take > this up as a 2 Month Project which is a requirement of my school ( need a > certification for the same ). I see good amount of progress has been made on > this and have gone through most of the comments and got confused on what all > has been completed and what all is left. Sir, please share some pointers on > the same and if its possible for me to take this up and start working > immediately. Hi Manraj! I'd be very happy if you choose to take on this bug and will be able to finish it to completion! * Do you already have a checkout of the mozilla-central source code and a Firefox build up and running? * Are you able to test your work on OSX, Windows and Linux? * Are you able to apply the latest patch, resolve any conflicts that are the result of bitrot and see the result in your local build? * Improvements that need to happen on top of the latest patch attached here is mentioned in comment 68. After that is done, it is essential to find all the shortcuts that are not accessible through menu items, but are 'hidden' instead. Examples are keyboard shortcuts for 'Back', 'Forward', 'Stop', 'Home', etc. If you can tick all the boxes above - which I'm happy to help with! - then feel free to assign this bug to yourself and make this your awesome summer project :-) It'd be great if we could connect and talk via IRC; my nick is 'mikedeboer' and my timezone is GMT+1 (my availability is not strictly limited to office hours). I usually hang out in the #developers, #firefox, #fx-team channels and more. Feel free to ask questions in this bug, but IRC and email are by far the most effective. James, do you have anything to add that I might've forgot?
Flags: needinfo?(dbolter) → needinfo?(hobinjk)
I don't have much to add beyond what I said in comment 70. The key is to look at the keyboard shortcuts listed in https://support.mozilla.org/en-US/kb/keyboard-shortcuts-perform-firefox-tasks-quickly, figure out which are missing, then fix whatever the cause is. The cause will usually be that the shortcut is implemented through addEventListener('keydown', ...) instead of the XUL-based <key>, <command>, <menuitem> pattern.
Flags: needinfo?(hobinjk)
Hi Mike sir, > * Do you already have a checkout of the mozilla-central source code and a Firefox build up and running? Yes sir. I have previously worked and patched 4 bugs. I am currently rebuilding it by running `mach clobber` and `mach build` after force updating it. > * Are you able to test your work on OSX, Windows and Linux? Yes sir. I currently work on Windows but have Linux( Ubuntu ) installed on VMware(Don't have a checked out code on it though). I currently don't have an OSX device with me. Please guide me with the same. > * Are you able to apply the latest patch, resolve any conflicts that are the result of bitrot and see the result in your local build? Sure sir. Have been waiting for build to finish successfully. It is throwing XPCOM error on `mach run` . I'm rebuilding it and hope it succeeds and will apply patch soon. > * Improvements that need to happen on top of the latest patch attached here is mentioned in comment 68. After that is done, it is essential to find all the shortcuts that are not accessible through menu items, but are 'hidden' instead. Examples are keyboard shortcuts for 'Back', 'Forward', 'Stop', 'Home', etc. Sure sir. Lets talk on IRC. My nick is 'manrajsingh' . I'll be back around 7pm ( GMT+1 ) and will drop a message, if its okay with you. :) Sorry for late reply. Have been fixing this build on my own in the meantime.
Flags: needinfo?(mdeboer)
UPDATE: I have got the build running on Windows again and have tried to apply the patch attached in this thread but it is throwing loads of conflicts and errors. I guess this is because the files have changed since the patch was last updated.
(In reply to Manraj Singh [:manrajsingh] from comment #76) > UPDATE: > > I have got the build running on Windows again and have tried to apply the > patch attached in this thread but it is throwing loads of conflicts and > errors. I guess this is because the files have changed since the patch was > last updated. That's indeed because the files have changed much since the last time James worked on it. So the first challenge for you is the get the latest patch working again on your machine!
Flags: needinfo?(mdeboer)
Hi Mike sir, I have removed all conflicts and got the build running. Please review the code and let me know the changes, if any. I haven't tested it though as I don't know what all to. Also I'm not sure where will the changes of browser/base/content/browser-appmenu.inc go as this file no longer exist and ids are not locatable on DXR.
Flags: needinfo?(mdeboer)
Attachment #8621996 - Flags: review?(mdeboer)
Hi sir, Sorry if I am disturbing. Please review my patch and let me know further steps/changes. Thank you. :)
Comment on attachment 8621996 [details] [diff] [review] Removed all conflicts and got the build running. Few doubts in description. Review of attachment 8621996 [details] [diff] [review]: ----------------------------------------------------------------- I was out sick yesterday, so apologies for the delay. Alright, thanks for the update here! I tried it locally and it seems to work quite well. The next step is to continue improving this patch as mentioned in comment 73 and comment 74. So making sure all the keyboard shortcuts on https://support.mozilla.org/en-US/kb/keyboard-shortcuts-perform-firefox-tasks-quickly are also listed in the HUD is quite essential. Then it still needs to look good too!
Attachment #8621996 - Flags: review?(mdeboer)
Flags: needinfo?(mdeboer)
Fixed some issues mentioned in #comment 68 . Some suggestions were tested and were not working. Eg. > XUL elements can use the `hidden` attribute, which is better. > > Please move up and rewrite to: > > ```js > wrapperNode.id = "keyboardShortcuts-wrapper"; > wrapperNode.setAttribute("hidden", "true"); > ```XUL elements can use the `hidden` attribute, which is better. > > Please move up and rewrite to: > > ```js > wrapperNode.id = "keyboardShortcuts-wrapper"; > wrapperNode.setAttribute("hidden", "true"); > ``` And so they were not included in this patch. I am still figuring out this portion. > After that is done, it is essential to find all the shortcuts that are not > accessible through menu items, but are 'hidden' instead. Examples are keyboard > shortcuts for 'Back', 'Forward', 'Stop', 'Home', etc. > The cause will usually be that the shortcut is implemented through > addEventListener('keydown', ...) instead of the XUL-based <key>, <command>, > <menuitem> pattern. Sir please review the patch and let me know if I missed anything. Also please help me with the highlighted portion above. I am sorry for the delay in between as I was away for a hackathon representing Mozilla.
Attachment #8621996 - Attachment is obsolete: true
Flags: needinfo?(mdeboer)
Attachment #8625461 - Flags: review?(mdeboer)
I was away too, attending a work week :) I'll take a look at the patch asap.
Flags: needinfo?(mdeboer)
Comment on attachment 8625461 [details] [diff] [review] Fixed some issues in #Comment 68 . Review of attachment 8625461 [details] [diff] [review]: ----------------------------------------------------------------- Alright, this seems to look like an OK starting point. Please continue to find the rest of the keyboard shortcuts that can be used in Firefox.
Attachment #8625461 - Flags: review?(mdeboer) → feedback+
Sir, can we discuss the highlighted points mentioned in #comment 81 on IRC? Please let me know the time when we can talk. :)
Flags: needinfo?(mdeboer)
Please, ping me anytime - I'll be around probably.
Flags: needinfo?(mdeboer)
Hi Mike Sir, I have included "hidden" Context Menu Shortcuts in this new patch as discussed on IRC. Please review the patch and let me know the changes and further steps. Also I noticed, I haven't been assigned the project[bug] till now. Could you please assign it to me? :) Also I feel we should include everyday-use keyboard shortcuts only in HUD else it will be more scrolling down in HUD. Please share your thoughts on the same.
Flags: needinfo?(mdeboer)
Attachment #8632762 - Flags: review?(mdeboer)
Assignee: nobody → manrajsinghgrover
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
Comment on attachment 8632762 [details] [diff] [review] Added Context Menu Shortcuts as discussed. I'm sorry, but the HUD is supposed to contain all the keyboard shortcuts that can be invoked _globally_. What I mean by that is that at the list should become as comprehensive as https://support.mozilla.org/en-US/kb/keyboard-shortcuts-perform-firefox-tasks-quickly. So, part of the problem that you, as engineer, have to solve is how to collect most - hopefully all - the keyboard shortcuts that are listed on that web page. The shortcuts you added here are not really helpful for users, I'm afraid.
Attachment #8632762 - Flags: review?(mdeboer)
Hi Mike sir, Sorry for this huge delay from my side. I had been involved in loads of academic and our Local Mozilla Community leading to this. Last update(as per IRC) was etherpad that was created for shortcuts that were left and need for creation of their shortcuts if its not possible to extract them programmatically. I wish to start working on this again and would first like to re-discuss things on IRC. Sorry for the delay again.
Flags: needinfo?(mdeboer)
OK, sure, I'm available anytime! If I happen to not be around on IRC - could happen when I'm asleep - you can always ask for help in the #fx-team IRC channel.
Flags: needinfo?(mdeboer)
Unassigning due to a long period of inactivity.
Assignee: manrajsinghgrover → nobody
Status: ASSIGNED → NEW
See Also: → 565993
Type: defect → enhancement
Attached image about-shortcuts.png —

Since XUL is deprecated, wouldn't it be better to use an about: pages (e.g about:shortcuts) for the HUD. Also, I've attached a quick mockup of what it could looks like with the Photon Design System (Dark variant).

Hi, is this ticket still available and to implement this, I essentially have to make an about page for shortcuts? and make a shortcut to this about page?

(In reply to jacquelinechan2 from comment #92)

Hi, is this ticket still available and to implement this, I essentially have to make an about page for shortcuts? and make a shortcut to this about page?

Thanks for your interest to help on the development of Firefox :). Needinfo'ing Mike de Boer (mentor of this bug) so your comment doesn't slip through the cracks.

Flags: needinfo?(mdeboer)

Hi Mike,
This sounds a great project for my summers break, would like to contribute to it, could you guide me through the process.

This sounds interesting, can I work on this?

Hi, I am new to open source and want to start. If it is still open I am ready to contribute. Also, it would be a great help if some guide me through the starting process

Unfortunately we don't have someone ready to mentor this to help it move forward at the current time.

Mentor: mdeboer
Flags: needinfo?(mdeboer)
Severity: normal → S3
See Also: → 1746583
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: