Closed Bug 589139 Opened 9 years ago Closed 9 years ago

Keyboard short-cuts missing from Firefox button menu items

Categories

(Firefox :: Menus, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: jmjjeffery, Assigned: Margaret)

References

Details

Attachments

(1 file, 10 obsolete files)

With the landing of bug 583386 the keyboard short-cuts are missing from the menu's.  These being missing does not lead a user to the advantages of keyboard short-cuts if they are not presented.

I recall some discussion that the Menu-button would not be keyboard accessible in and of itself, but the Menu-items should display the short-cuts as as they are in the Menu-bar.
In bug https://bugzilla.mozilla.org/show_bug.cgi?id=571750 it would appear that keyboard short-cuts would not be used in the Menu-button and were removed in that patch.  However - there seems to either be an oversight or a change of mind in that the mockups for i6 of the Button show the short-cuts. 

Can we get some clarification on this please >?
Alex, we need some clarification here. Presently we're shipping the new Firefox button, but also leaving the toolbar on and visible when you press ALT.

Is the path forward that we will ship both the Firefox button and the menuBar, or just one or the other?
blocking2.0: --- → ?
For the record, the latest dev version of Chrome with their re-do of the menu/tools does show keyboard shortcuts. 

Further, new users that have never seen Firefox and download 4.0 for the first time will have no clue that a hidden sub-menu exits.  Been my somewhat limited experience that new users just don't go pushing buttons to see what happens. They will look at the Menu-button and shrug and ask questions in support about "Are there any keyboard short-cuts in Firefox ? "
blocking2.0: ? → ---
I guess what I'm most confused about is: if we're keeping the menuBar (which I think we are) then are there plans to make it a bit more consistent with the Firefox Button?
(In reply to comment #2)
> Alex, we need some clarification here. Presently we're shipping the new Firefox
> button, but also leaving the toolbar on and visible when you press ALT.
> 
> Is the path forward that we will ship both the Firefox button and the menuBar,
> or just one or the other?

I think we'll want to keep both, see bug 575279 comment 6.

This bug is about displaying keyboard shortcuts for the individual items in the menu, though.
Summary: Keyboard short-cuts missing from Menu Button → Keyboard short-cuts missing from Firefox button menu items
(In reply to comment #5)
> This bug is about displaying keyboard shortcuts for the individual items in the
> menu, though.

Oh, displaying them in the labels, not adding keyboard accessible strokes. Sorry, got mixed up there.
blocking2.0: --- → ?
Needs to match the mockup of i6, which has these in the submenus (but not the main menu)
blocking2.0: ? → beta5+
The main menu items on the Firefox button do not display keyboard shortcuts inline, but they need to get shortcut tooltips(this is standard for all other instances of this style of control).  The submenus do display keyboard shortcuts, and since the first item is redundant with the main command, the user now has multiple ways to learn the shortcut.

>I guess what I'm most confused about is: if we're keeping the menuBar (which I
>think we are) then are there plans to make it a bit more consistent with the
>Firefox Button?

Some users, especially in accessibility cases, have built up muscle memory (kind of komani code style) for accessing commands, like edit copy is alt-right-down-down-down.  It's in our interest to try to keep these pretty consistent, however a few commands are going to be removed from both the Firefox menu and traditional menu bar, the current list is here: http://people.mozilla.com/~faaborg/files/20100718-firefoxButtonDetails/menuMigration-i1.png
I may be able to pick this up if no one else is working on it. Does fixing this bug involve adding the shortcuts to the submenus, creating shortcut tooltips, or both?
The submenus should show shortcuts on the right (just like native menus).  The main menu should only display shortcuts in tooltips.
Assignee: nobody → margaret.leibovic
Status: NEW → ASSIGNED
Attached patch WIP (obsolete) — Splinter Review
Adding the text to the sub-menus was easy, but I'm running into some difficulty getting the tooltips to work. I made a new menuitem binding which I thought would work, but it isn't. I'm definitely getting the acceltext, but the tooltiptext attribute doesn't seem to be working, even when I tried setting it with a hard-coded string outside of the xbl:inherits.
Attached patch patch (obsolete) — Splinter Review
Attachment #470106 - Flags: review?(gavin.sharp)
Comment on attachment 470106 [details] [diff] [review]
patch

Can we put the binding in browser/ for now? Not sure this is something that's worth adding to toolkit/ yet.

I think you can avoid overriding the content, and have your binding just have a constructor that does:
this.setAttribute("tooltiptext", this.getAttribute("acceltext"));

since the accelText is actually set on the menuitem node itself, and just inherited onto menuitem-label.

There are some tabs in there, you should get rid of those :)
(In reply to comment #13)
> Comment on attachment 470106 [details] [diff] [review]
> patch
> 
> Can we put the binding in browser/ for now? Not sure this is something that's
> worth adding to toolkit/ yet.

Sure, that makes sense. Any thoughts on where it should go? Should I create a new file?

> I think you can avoid overriding the content, and have your binding just have a
> constructor that does:
> this.setAttribute("tooltiptext", this.getAttribute("acceltext"));
> 
> since the accelText is actually set on the menuitem node itself, and just
> inherited onto menuitem-label.

If I don't override the content, the acceltext shows up on the right side of the menuitem because of its key attribute. However, I can definitely clean it up. I think there are unnecessary things in there from when I was trying to figure out what would get the tooltiptext to work.

> There are some tabs in there, you should get rid of those :)

Oops! I'm working in Windows, and I didn't have my text editor preferences set correctly :)
Attached patch patch-v2 (obsolete) — Splinter Review
I just moved the bindings to browser/base/content/urlbarBindings.xml for now, but that doesn't seem to be the right place for them.
Attachment #470068 - Attachment is obsolete: true
Attachment #470106 - Attachment is obsolete: true
Attachment #470202 - Flags: review?(gavin.sharp)
Attachment #470106 - Flags: review?(gavin.sharp)
(In reply to comment #14)
> If I don't override the content, the acceltext shows up on the right side of
> the menuitem because of its key attribute.

Ah right, forgot about that. I think it would be better to just hide menu-accel-container with CSS, something like:

.menuitem-tooltip > .menu-accel-container { display: none; }

The |menuitem.menuitem-tooltip| selectors should probably only be ".menuitem-tooltip" (per https://developer.mozilla.org/en/Writing_Efficient_CSS#Don.e2.80.99t_qualify_Class_Rules_with_tag_names ).
Attached patch patch-v3 (obsolete) — Splinter Review
This patch is more elegant, but adding the .menuitem-tooltip class to the .menuitem-iconic elements is causing their icons to disappear, and I'm not sure why.
(In reply to comment #17)
> Created attachment 470218 [details] [diff] [review]
> patch-v3
> 
> This patch is more elegant, but adding the .menuitem-tooltip class to the
> .menuitem-iconic elements is causing their icons to disappear, and I'm not sure
> why.

Because you're extending #menuitem instead of #menuitem-iconic.
Attached patch patch-v4 (obsolete) — Splinter Review
Here's a patch that works.

Dao, thanks for the tip. I had hoped I could just write one binding to add the tooltip, but making a #menuitem-iconic-tooltip binding that extends #menuitem-iconic solves the icon problem.
Attachment #470202 - Attachment is obsolete: true
Attachment #470218 - Attachment is obsolete: true
Attachment #470308 - Flags: review?(gavin.sharp)
Attachment #470202 - Flags: review?(gavin.sharp)
blocking2.0: beta5+ → beta6+
Whiteboard: [strings]
Whiteboard: [strings]
Comment on attachment 470308 [details] [diff] [review]
patch-v4

>+.menuitem-tooltip {
>+  -moz-binding: url("chrome://browser/content/urlbarBindings.xml#menuitem-tooltip");
>+}

>           <menuitem id="appmenu_fullScreen"
>+                    class="menuitem-tooltip"
>                     label="&fullScreenCmd.label;"
>                     type="checkbox"
>-                    observes="View:FullScreen"/>
>+                    observes="View:FullScreen"
>+                    key="key_fullScreen"/>

type="checkbox" needs the iconic binding as well. http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#363
Attached patch unbitrotted patch (obsolete) — Splinter Review
I updated this to post-http://hg.mozilla.org/mozilla-central/rev/89d517900126 because I wanted to test it. I found that it seems to be messing with padding of the menu:

http://grab.by/69aK - with patch
http://grab.by/69aM - without
Attachment #470308 - Attachment is obsolete: true
Attachment #470308 - Flags: review?(gavin.sharp)
I added css to use iconic binding for .menuitem-tooltip[type="checkbox"] and .menuitem-tooltip[type="radio"] (comment 20).

To fix the padding issue, I set the value of the accel labels to "" instead of hiding them with css. This fixes the problem because their css rules give us margins (http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/menu.css#111).
Attachment #470479 - Attachment is obsolete: true
Attachment #470543 - Flags: review?(gavin.sharp)
Oh, I should have thought of that. Can we just use visibility: hidden instead?
(In reply to comment #23)
> Oh, I should have thought of that. Can we just use visibility: hidden instead?

Just tried it, and it made the menuitems super wide because the accel text labels are long.
>+document.getAnonymousElementByAttribute(this, "anonid", "accel").firstChild.setAttribute("value", "");

I was going to suggest this.setAttribute("acceltext", "");, but then I discovered bug 592424. We should probably switch once that's fixed.
Bug https://bugzilla.mozilla.org/show_bug.cgi?id=465090 added the keyboard command to directly access the Addons-Manager.  Chrl+Shft+a 

Probably should be added to the MenuButton sub-menu Add-ons
Attached patch alternative patch (obsolete) — Splinter Review
I made this cause I wanted to test out an alternative approach. It uses slightly more complicated CSS rules to avoid adding the class to all top-level items in the menu.
Attachment #471231 - Flags: feedback?(margaret.leibovic)
Attachment #471231 - Flags: feedback?(dao)
Comment on attachment 471231 [details] [diff] [review]
alternative patch

Seems sensible to me, but I'm not the expert, you are :) It's nice to not add the new classes, but the css looks a little gnarly, and I'm wondering if that will make menu changes more confusing down the road.
Attachment #471231 - Flags: feedback?(margaret.leibovic) → feedback+
Comment on attachment 471231 [details] [diff] [review]
alternative patch

I'd prefer the classes, they're certainly more efficient and I don't really see the authoring/maintenance cost, assuming this is why you wanted to drop them.

>+#appmenuPrimaryPane > :-moz-any(.menuitem-iconic,
>+                                menuitem[type="checkbox"],
>+                                menuitem[type="radio"]),

:-moz-any at the end of a selector isn't optimized and going to be extra inefficient. This would have to be spelled out:

#appmenuPrimaryPane > .menuitem-iconic,
#appmenuPrimaryPane > menuitem[type="checkbox"],
etc.
Attachment #471231 - Flags: feedback?(dao)
Comment on attachment 470543 [details] [diff] [review]
patch w/ checkbox fix and padding fix

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>            <menuitem id="appmenu_addons"

This needs a menuitem-iconic-tooltip class addition and key="key_openAddons", per comment 26

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>+  <binding id="menuitem-tooltip" extends="chrome://global/content/bindings/menu.xml#menuitem">
>+  <binding id="menuitem-iconic-tooltip" extends="chrome://global/content/bindings/menu.xml#menuitem-iconic">

>+        document.getAnonymousElementByAttribute(this, "anonid", "accel").firstChild.setAttribute("value", "");

Add a "TODO: Simplify this to this.setAttribute("acceltext", "") once bug 592424 is fixed" comment here?
Attachment #470543 - Flags: review?(gavin.sharp) → review+
Attachment #471231 - Attachment is obsolete: true
Attached patch final patch (obsolete) — Splinter Review
With fixes from comment 30.
Attachment #470543 - Attachment is obsolete: true
Attached patch final patch (for real!) (obsolete) — Splinter Review
Fixed tabs and bitrot.
Attachment #471541 - Attachment is obsolete: true
Keywords: checkin-needed
More bitrot:

patching file browser/base/content/browser.xul
Hunk #4 FAILED at 612
1 out of 6 hunks FAILED -- saving rejects to file browser/base/content/browser.xul.rej
Keywords: checkin-needed
Attachment #471645 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/911b8c87f66e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Margaret. 
Now it seems that the menu item of New Tab has a shortcut. Is this inteneded?  
Doesn't bug 571750 make it that no top menu item in Firefox have a shortcut?  
No other top menu item has a shortcut currently. So I think you might have made a mistake. 

By the way Print... has a keyboard shortcut. Ctrl+P
(In reply to comment #36) 
> Now it seems that the menu item of New Tab has a shortcut. Is this inteneded?  
> Doesn't bug 571750 make it that no top menu item in Firefox have a shortcut?  
> No other top menu item has a shortcut currently. So I think you might have made
> a mistake. 

The top level menu items have tooltip shortcuts (see comment 10).

> By the way Print... has a keyboard shortcut. Ctrl+P

Oops, I guess I missed the sub-menu shortcut on that one. Thanks for pointing it out! I will file a follow-up bug and patch.
(In reply to comment #37)
> The top level menu items have tooltip shortcuts (see comment 10).

And New Tab doesn't in the nightly! I found the problem and will fix it.
Blocks: 604650
You need to log in before you can comment on or make changes to this bug.