1000513 - Combined navigation items in the context menu

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Improving Context Menus in Firefox Desktop - Concept.pdf

See attached PDF document describing potential feature behaviors. The first part of this project is just to implement the navigation elements, with customization coming in a later phase.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

So far the things that stick out to me that we will need to figure out how to do are:
- Arrange the menuitems horizontally while still allowing keyboard navigation in the menu
- Stop drawing the menupopup styling of the vertical separator behind these buttons (since the row takes up the full width)
- Show the dropdown for dragging or long-presses on the back and forward buttons

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Patch

This is rough WIP patch to see what issues we may encounter. The <hbox> appears to be breaking keyboard navigation.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Patch v2

Fixed keyboard navigation and the line being drawn behind the back button. Need to get correctly sized icons and fix the color of the bottom border to match that of the menupopup separators.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 1 - Allow menuitems in a menupopup to be children of an <hbox> for horizontal arrangement

Neil, are you OK with this? Unfortunately there is some duplication within the each of the Get{Next,Previous}MenuItem functions, but I didn't want to do a bunch of refactoring here because it may make the code harder to follow.

Comment 5

[Neil Deakin]

Some questions/comments:

1. I would prefer if a new element name such as menugroup was used rather than hbox. That way theming it would be easier.

2. Is the intent that you would press down/up to navigate the items, even though they are displayed horizontally?

3. You'll also need to change nsXULPopupManager::UpdateMenuItems, nsMenuFrame::UpdateMenuSpecialState and nsMenuPopupFrame::FindMenuWithShortcut. You may want to file a followup bug on creating an dedicated iterator after this to refactor all of the similar code.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 1 v2 - Allow menuitems in a menupopup to be children of a <menugroup> for horizontal arrangement

Thanks for the feedback. I've addressed the items that you mentioned.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Front-end patch v3

Just need the larger icons before this patch is complete.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8416774 [details] [diff] [review]
WIP Front-end patch v3

Review of attachment 8416774 [details] [diff] [review]:
-----------------------------------------------------------------

Blair, can you give me an early feedback pass on this before I get the final icons in?

Things to note:
1) Up/down allow for moving between horizontal items. This is intentional as to not break muscle memory with the previous context menu. We could introduce left/right arrow navigation, but I'd like to handle that in a follow-up since we may need to investigate how to handle <menu> children of <menugroup>s (submenus that are opened on left/right keys).
2) The bookmark star doesn't get filled in when the page is bookmarked. This is not a regression from current context menu behavior (it always says "Bookmark This Page"). I'd like to fix this in a follow-up.
3) The spec calls for adding in popups for deeper back/forward navigation, similar to what users get when the right-click or drag the back/forward buttons. I haven't implemented that with this patch, as I'm not sure if we want to have popups within popups.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 2 - WIP Front-end patch v3.1

Added missing contextmenu.inc.css. Same notes as above.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8416810 [details] [diff] [review]
Part 2 - WIP Front-end patch v3.1

Review of attachment 8416810 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, this is *much* cleaner than I was expecting :)

::: browser/themes/shared/contextmenu.inc.css
@@ +13,5 @@
> +#context-navigation > .menuitem-iconic > .menu-iconic-left {
> +  -moz-appearance: none;
> +}
> +
> +#context-navigation > #context-back > .menu-iconic-left {

menu-iconic-left? Wow, we suck at naming classes.

Also, do you even have to specify that? I would have expected having the selector just select the menuitem node would have been enough (and make reading this CSS nicer).

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> 3) The spec calls for adding in popups for deeper back/forward navigation,
> similar to what users get when the right-click or drag the back/forward
> buttons. I haven't implemented that with this patch, as I'm not sure if we
> want to have popups within popups.

Down with popups over popups. But, we could do a subview similar to what we do in the panelUI.

All those you mentioned in comment 8 seem good to have as separate followup bugs.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Front-end patch

Fixed nits and Linux styling. Tested on Windows and Linux but not on OSX yet.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: OSX - hover of navigation items

OSX definitely needs work. Here's a screenshot of what it looks like (10.9) when you're hovering over one of the new items. In addition to inverting the colour of the icon, I wonder if we should consider making the highlighted background should extend up to the top edge of the menu - even though this isn't convention on OSX.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: OSX - hover on normal items

Also seems to have affected other items in the menu on OSX (10.9 again), altering the position of the highlight for normal items in the menu.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Win8 - hover on navigation items

Windows this time. The forward button is disabled, but still has a hover style - it shouldn't.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride [:Unfocused] from comment #15)
> Created attachment 8419917 [details]
> Win8 - hover on navigation items
> 
> Windows this time. The forward button is disabled, but still has a hover
> style - it shouldn't.

Oh, duh, this is of course per platform convention. But for some reason it feels extra weird for these items. Thoughts?

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8419587 [details] [diff] [review]
Front-end patch

Review of attachment 8419587 [details] [diff] [review]:
-----------------------------------------------------------------

The spec document has the background of this row of items having a slightly darker background colour.

Also, I feel this change makes it more obvious that the disabled state of the star widget isn't always in sync with the state of the bookmark item in this menu. eg, when on a about: page, the star widget is disable, but you can still bookmark via the context menu. Forget the bug number for that, but I think we should adjust the priority for it now, considering this bug.

::: browser/base/content/browser-context.inc
@@ +221,5 @@
>        <menuseparator id="context-sep-ctp"/>
> +      <menugroup id="context-navigation">
> +        <menuitem id="context-back"
> +                  class="menuitem-iconic"
> +                  label="&backCmd.label;"

Given these are just icons now (the label is useful for accessibility, but not to anyone not using accessibility features), I think we should consider adding tooltiptext to these items.

@@ +222,5 @@
> +      <menugroup id="context-navigation">
> +        <menuitem id="context-back"
> +                  class="menuitem-iconic"
> +                  label="&backCmd.label;"
> +                  accesskey="&backCmd.accesskey;"

These accesskeys no longer seem to work. And we can't just leave them here, because they're exposed to screenreaders.

::: browser/themes/linux/browser.css
@@ +469,5 @@
>  }
>  
> +#context-back:-moz-locale-dir(rtl),
> +#context-forward:-moz-locale-dir(rtl) {
> +  transform: scaleX(-1);

Hm, why aren't you doing this for the other OSes?

@@ +477,2 @@
>  #context-forward[disabled] {
> +  opacity: .5;

Ditto.

::: browser/themes/shared/contextmenu.inc.css
@@ +14,5 @@
> +  -moz-appearance: none;
> +}
> +
> +#context-navigation > #context-back > .menu-iconic-left {
> +  list-style-image: url("chrome://browser/skin/Toolbar.png");

Will need different DPI versions for all these.

@@ +24,5 @@
> +  -moz-image-region: rect(0, 72px, 18px, 54px);
> +}
> +
> +#context-navigation > #context-reload > .menu-iconic-left {
> +  list-style-image: url("chrome://browser/skin/reload-stop-go.png");

Wow this looks ugly scaled up :)

Comment 18

[u428464]

I know it would be an inconsistency, but wouldn't it be possible to use the Australis styling (as seen for example in the bookmarks menu and submenus) for the whole context menus.

Comment 19

[Neil Deakin]

Comment on attachment 8416773 [details] [diff] [review]
Part 1 v2 - Allow menuitems in a menupopup to be children of a <menugroup> for horizontal arrangement

This looks ok. We may need to change it if menugroup has a binding attached I think, but we can do that later.

Do you plan on writing some tests. We can file a followup to clean up this code and add some tests.

Comment 20

[[e-mail]]

Comment on attachment 8416773 [details] [diff] [review]
Part 1 v2 - Allow menuitems in a menupopup to be children of a <menugroup> for horizontal arrangement

I have a suspicion that this code will get confused if someone tries to nest menugroups ;-)

I don't like the way the code is duplicated; I'm wondering whether it might be possible to call nsXULPopupManager::GetNextMenuItem from more places.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Blair McBride [:Unfocused] from comment #14)
> Created attachment 8419915 [details]
> OSX - hover on normal items
> 
> Also seems to have affected other items in the menu on OSX (10.9 again),
> altering the position of the highlight for normal items in the menu.

This is bug 1002235 and not the result of these patches.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 1 - <menugroup> support

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 2 - Front-end patch

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 3 - Front-end tests

Try server push: https://tbpl.mozilla.org/?tree=Try&rev=6f3f716a4e10

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> Created attachment 8426324 [details] [diff] [review]
> Part 1 - <menugroup> support

nsXULPopupManager::UpdateMenuItems doesn't use GetNextMenuItem because I couldn't figure out how to get the proper frame to pass to GetNextMenuItem for it all to work.

Comment 26

[Alice0775 White]

The tryserver build break keyboard navigation(keypress Shift+F10 then B, F R S and m).
For Example,

Keypress Shift+F10
Keypress B

This should be execute navigation-back.

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: OSX - horizontal space

On OSX, the minimum width of the navigation items is quite wide - resulting in the whole context menu being much wider than it needs to be. This is thanks to <menuitem>s on OSX having a 21px left & right padding.

Comment 28

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Alice0775 White from comment #26)
> The tryserver build break keyboard navigation(keypress Shift+F10 then B, F R
> S and m).
> For Example,
> 
> Keypress Shift+F10
> Keypress B
> 
> This should be execute navigation-back.

Hmm, that's a point - even though the access keys are no longer advertised in any way, if we remove that functionality then we'll be breaking muscle memory :\

Comment 29

[Dão Gottwald [:dao]]

That's a necessary consequence of the design. If we don't like that, we should go back to the drawing board. Magic access keys that users have no chance to discover shouldn't be considered a viable solution.

Comment 30

[u428464]

(In reply to Dão Gottwald [:dao] from comment #29)
> That's a necessary consequence of the design. If we don't like that, we
> should go back to the drawing board. Magic access keys that users have no
> chance to discover shouldn't be considered a viable solution.

Tooltip`s for the "visual items" ?

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8426325 [details] [diff] [review]
Part 2 - Front-end patch

Review of attachment 8426325 [details] [diff] [review]:
-----------------------------------------------------------------

In addition to the existing bug mentioned in comment 17 for the bookmark star, I've noticed that it's now more obvious (because it's more visual) that the stop/reload button isn't kept in sync while the context menu is open. So, another followup bug.

r- due to comment 27 and comment 28. I'm hoping comment 28 isn't going to be too much of a problem, because I think we should shy away from landing with a regression like that.

Comment 32

[Dão Gottwald [:dao]]

(In reply to Guillaume C. [:ge3k0s] from comment #30)
> (In reply to Dão Gottwald [:dao] from comment #29)
> > That's a necessary consequence of the design. If we don't like that, we
> > should go back to the drawing board. Magic access keys that users have no
> > chance to discover shouldn't be considered a viable solution.
> 
> Tooltip`s for the "visual items" ?

Exposing command keys in tooltips is a reasonable compromise, but access keys in tooltips would be more non-standard and weird, I think.

Comment 33

[u428464]

(In reply to Dão Gottwald [:dao] from comment #32)
> (In reply to Guillaume C. [:ge3k0s] from comment #30)
> > (In reply to Dão Gottwald [:dao] from comment #29)
> > > That's a necessary consequence of the design. If we don't like that, we
> > > should go back to the drawing board. Magic access keys that users have no
> > > chance to discover shouldn't be considered a viable solution.
> > 
> > Tooltip`s for the "visual items" ?
> 
> Exposing command keys in tooltips is a reasonable compromise, but access
> keys in tooltips would be more non-standard and weird, I think.

AFAIK the long term goal for context menu is customization (as seen in this doc : http://people.mozilla.org/~sfranks/ContextMenus/Improving%20Context%20Menus%20in%20Firefox%20Desktop%20-%20Concept.pdf ). So tooltips may become more standard in this area.

Comment 34

[Elbart]

(In reply to Guillaume C. [:ge3k0s] from comment #33)
> AFAIK the long term goal for context menu is customization (as seen in this
> doc :
> http://people.mozilla.org/~sfranks/ContextMenus/
> Improving%20Context%20Menus%20in%20Firefox%20Desktop%20-%20Concept.pdf ).

Will it be possible or is it planned to also be able to remove elements from the navigation-section, or remove it as a whole during customization?

Comment 35

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to Dão Gottwald [:dao] from comment #29)
> That's a necessary consequence of the design. If we don't like that, we
> should go back to the drawing board. Magic access keys that users have no
> chance to discover shouldn't be considered a viable solution.

So once again you want to throw those that are impaired or possibly blind under the bus by removing any hope of navigation.  Obscure of not, those that are impaired will learn / find those 'magic keys' to assist in their use of the internet.  I doubt a screen reader will be able to recognize navigation icons in the context menu.

Comment 36

[Dão Gottwald [:dao]]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #35)
> (In reply to Dão Gottwald [:dao] from comment #29)
> > That's a necessary consequence of the design. If we don't like that, we
> > should go back to the drawing board. Magic access keys that users have no
> > chance to discover shouldn't be considered a viable solution.
> 
> So once again you want to throw those that are impaired or possibly blind
> under the bus by removing any hope of navigation.  Obscure of not, those
> that are impaired will learn / find those 'magic keys' to assist in their
> use of the internet.  I doubt a screen reader will be able to recognize
> navigation icons in the context menu.

I simply outlined our options without saying which side I preferred.

However, I don't think there's a big accessibility impact here as Alt+Left/Right Arrow continues to work. Then again, this poses the question why those items exist at all in the context menu.

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 2 - Front-end patch (addressed comment #27 and #28)

Try push, https://tbpl.mozilla.org/?tree=Try&rev=f4c48d5e6d6c

Comment 38

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8427350 [details] [diff] [review]
Part 2 - Front-end patch (addressed comment #27 and #28)

Review of attachment 8427350 [details] [diff] [review]:
-----------------------------------------------------------------

As mentioned on IRC, I had trouble getting the access keys to work. All worked on Windows, but only the accesskey for Back worked on Linux/OSX.

Between accessibility and muscle memory, Jared and I agreed it was important to keep the accesskeys working. As an comparable situation, Jared reminded me that we support some keyboard shortcuts to avoid breaking muscle memory for people used to IE's shortcuts - and some of those we don't expose in the UI either.

Comment 39

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 1 v2 - <menugroup> support

Fixed the issue that Blair found with disabled menuitems inside of menugroups.

Comment 40

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8427350 [details] [diff] [review]
Part 2 - Front-end patch (addressed comment #27 and #28)

Re-flagging for review now that part 1 has been updated.

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

Try push, https://tbpl.mozilla.org/?tree=Try&rev=93abed04bbd9

Comment 42

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8427350 [details] [diff] [review]
Part 2 - Front-end patch (addressed comment #27 and #28)

Review of attachment 8427350 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, working good on Linux and OSX too now.

Comment 43

[Dão Gottwald [:dao]]

Comment on attachment 8427350 [details] [diff] [review]
Part 2 - Front-end patch (addressed comment #27 and #28)

I decidedly disagree -- we should not have hidden access keys. Please don't confuse command keys with access keys. Access keys are contextual and need to be visible on screen. Command keys work more globally and are documented on SUMO, for instance, which is why putting them as additional hints in tooltips makes sense or why we can even avoid exposing them at all in the UI.

Another thing to note is that access keys usually differ by locale. For people who may deal with Firefox in different locales (maybe at home vs. work, a friend's computer etc.) varying hidden access keys are particularly bad.

Comment 44

[Neil Deakin]

Comment on attachment 8427490 [details] [diff] [review]
Part 1 v2 - <menugroup> support

>-  nsIFrame* sib = GetParent()->GetFirstPrincipalChild();
>+  nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
>+  if (!pm) {
>+    return;
>+  }
> 
>+  nsIFrame* firstMenuItem = pm->GetNextMenuItem(GetParent(), nullptr, true);

GetNextMenuItem is static so you can just use nsXULPopupManager::GetNextMenuItem.


>+  if (aStart) {
>+    if (aStart->GetNextSibling())
>+      currFrame = aStart->GetNextSibling();
>+    else if (aStart->GetParent()->GetContent()->GetNameSpaceID() == kNameSpaceID_XUL &&
>+             aStart->GetParent()->GetContent()->Tag() == nsGkAtoms::menugroup)

If you're going to make more changes, you might as well change this and other places to just use aStart->GetParent()->GetContent()->IsXUL(nsGkAtoms::menugroup)


> nsXULPopupManager::UpdateMenuItems doesn't use GetNextMenuItem because I couldn't figure out how to get the proper frame to pass to GetNextMenuItem for it all to work.

It shouldn't anyway since we want to iterate over disabled items.

Comment 45

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Part 2 - Front-end patch (with accesskeys removed)

We talked a lot about this on IRC, decided to add them back in so as not to introduce new technical debt down the road.

Comment 46

[Dão Gottwald [:dao]]

Comment on attachment 8427953 [details] [diff] [review]
Part 2 - Front-end patch (with accesskeys removed)

you need to remove this:

http://hg.mozilla.org/mozilla-central/annotate/c695c2bd13ae/browser/themes/linux/browser.css#l441

and this:

http://hg.mozilla.org/mozilla-central/annotate/c695c2bd13ae/browser/themes/linux/browser.css#l450

Comment 47

[Jared Wein [:jaws] (please needinfo? me)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/df4be0b6c018
remote:   https://hg.mozilla.org/integration/fx-team/rev/0626cae186f9
remote:   https://hg.mozilla.org/integration/fx-team/rev/b98a72074b03

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df4be0b6c018
https://hg.mozilla.org/mozilla-central/rev/0626cae186f9
https://hg.mozilla.org/mozilla-central/rev/b98a72074b03

Comment 51

[:Gijs (he/him)]

The changes here will impact add-ons dealing with the context menu. We should devdoc that.

Comment 52

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

There seem to be several automated tests for this... could you let us know what would need to be covered by the Manual QA team here?

Comment 53

[Marco Mucci [:MarcoM]]

Hi Juan, can I QA Contact be assigned to this bug for final verification.

Comment 54

[juan becerra [:juanb]]

We don't have a specific contact person for this area, but it will be verified or tested around manually by someone in the team as part of the bug verifications for this sprint.

Comment 55

[Dan]

This was a terrible idea

Comment 56

[Marco Mucci [:MarcoM]]

Hi Cornel, will it be possible to have this bug verified by end of day this Friday?  Our current Iteration ends on Monday June 9.

Comment 57

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (X11; Linux i686; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0

Verified on latest Nightly, build ID: 20140602072051 that the navigation elements are correctly implemented. There were no issues found except for the known ones.

Jared, will the customization phase be tracked separately? If so, we can mark this bug verified and I'll keep an eye on it's dependencies.

Comment 58

[Jared Wein [:jaws] (please needinfo? me)]

Yes, customization will be tracked separately and will happen at some future point in time.

Comment 59

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Marking as [qa!] since verification is complete.

Comment 60

[Jared Wein [:jaws] (please needinfo? me)]

Release Note Request (optional, but appreciated)
[Why is this notable]: new change to the context menu in desktop firefox
[Suggested wording]: Easier back, forward, reload, and bookmarking through the context menu
[Links (documentation, blog post, etc)]: http://msujaws.wordpress.com/2014/05/27/experimenting-with-context-menus/

Comment 61

[sawyer]

This new UI is atrocious. Was any UX testing actually done on this?