Closed Bug 1000513 Opened 6 years ago Closed 6 years ago

Combined navigation items in the context menu

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
relnote-firefox --- 32+

People

(Reporter: jaws, Assigned: jaws)

References

(Depends on 6 open bugs)

Details

(Keywords: addon-compat, dev-doc-needed, Whiteboard: p=13 s=it-32c-31a-30b.3 [qa!] )

Attachments

(4 files, 14 obsolete files)

4.16 MB, application/pdf
Details
27.85 KB, patch
Unfocused
: review+
Details | Diff | Splinter Review
16.07 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
13.66 KB, patch
dao
: review+
Details | Diff | Splinter Review
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.
Flags: firefox-backlog?
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
Attached patch WIP Patch (obsolete) — Splinter Review
This is rough WIP patch to see what issues we may encounter. The <hbox> appears to be breaking keyboard navigation.
Flags: firefox-backlog? → firefox-backlog+
Attached patch WIP Patch v2 (obsolete) — Splinter Review
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.
Assignee: nobody → jaws
Attachment #8411348 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Summary: Breakdown: Combined navigation items in the context menu → Combined navigation items in the context menu
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.
Attachment #8415705 - Flags: review?(enndeakin)
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.
Thanks for the feedback. I've addressed the items that you mentioned.
Attachment #8415705 - Attachment is obsolete: true
Attachment #8415705 - Flags: review?(enndeakin)
Attachment #8416773 - Flags: review?(enndeakin)
Attached patch WIP Front-end patch v3 (obsolete) — Splinter Review
Just need the larger icons before this patch is complete.
Attachment #8415612 - Attachment is obsolete: true
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.
Attachment #8416774 - Flags: feedback?(bmcbride)
Added missing contextmenu.inc.css. Same notes as above.
Attachment #8416774 - Attachment is obsolete: true
Attachment #8416774 - Flags: feedback?(bmcbride)
Attachment #8416810 - Flags: feedback?(bmcbride)
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).
Attachment #8416810 - Flags: feedback?(bmcbride) → feedback+
(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.
Attached patch Front-end patch (obsolete) — Splinter Review
Fixed nits and Linux styling. Tested on Windows and Linux but not on OSX yet.
Attachment #8416810 - Attachment is obsolete: true
Attachment #8419587 - Flags: review?(bmcbride)
Attachment #8416773 - Flags: review?(neil)
Attached image OSX - hover of navigation items (obsolete) —
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.
Attached image OSX - hover on normal items (obsolete) —
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.
Attached image Win8 - hover on navigation items (obsolete) —
Windows this time. The forward button is disabled, but still has a hover style - it shouldn't.
(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 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 :)
Attachment #8419587 - Flags: review?(bmcbride) → review-
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 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.
Attachment #8416773 - Flags: review?(enndeakin) → review+
Attachment #8416773 - Flags: review?(neil)
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.
Whiteboard: p=13
Whiteboard: p=13 → p=13 s=it-32c-31a-30b.2
Whiteboard: p=13 s=it-32c-31a-30b.2 → p=13 s=it-32c-31a-30b.2 [qa?]
Whiteboard: p=13 s=it-32c-31a-30b.2 [qa?] → p=13 s=it-32c-31a-30b.2 [qa+]
(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.
Attached patch Part 1 - <menugroup> support (obsolete) — Splinter Review
Attachment #8416773 - Attachment is obsolete: true
Attachment #8419587 - Attachment is obsolete: true
Attachment #8426324 - Flags: review?(enndeakin)
Attached patch Part 2 - Front-end patch (obsolete) — Splinter Review
Attachment #8426325 - Flags: review?(bmcbride)
(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.
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.
Attachment #8419913 - Attachment is obsolete: true
Attachment #8419915 - Attachment is obsolete: true
Attachment #8419917 - Attachment is obsolete: true
Attached image OSX - horizontal space (obsolete) —
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.
(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 :\
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.
(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 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.
Attachment #8426325 - Flags: review?(bmcbride) → review-
Attachment #8426328 - Flags: review?(bmcbride) → review+
(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.
(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.
(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?
(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.
(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 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.
Attachment #8427350 - Flags: review?(bmcbride) → review-
Attachment #8426853 - Attachment is obsolete: true
Fixed the issue that Blair found with disabled menuitems inside of menugroups.
Attachment #8426324 - Attachment is obsolete: true
Attachment #8426324 - Flags: review?(enndeakin)
Attachment #8427490 - Flags: review?(enndeakin)
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.
Attachment #8427350 - Flags: review- → review?(bmcbride)
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.
Attachment #8427350 - Flags: review?(bmcbride) → review+
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.
Attachment #8427350 - Flags: review-
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.
Attachment #8427490 - Flags: review?(enndeakin) → review+
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.
Attachment #8427350 - Attachment is obsolete: true
Attachment #8427953 - Flags: review?(dao)
Flags: in-testsuite+
Whiteboard: p=13 s=it-32c-31a-30b.2 [qa+] → p=13 s=it-32c-31a-30b.2 [qa+] [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/df4be0b6c018
https://hg.mozilla.org/mozilla-central/rev/0626cae186f9
https://hg.mozilla.org/mozilla-central/rev/b98a72074b03
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: p=13 s=it-32c-31a-30b.2 [qa+] [fixed-in-fx-team] → p=13 s=it-32c-31a-30b.2 [qa+]
Target Milestone: --- → Firefox 32
Depends on: 1016109
Depends on: 1016114
Depends on: 1016120
Whiteboard: p=13 s=it-32c-31a-30b.2 [qa+] → p=13 s=it-32c-31a-30b.3 [qa+]
Blocks: 996118
No longer blocks: 996118
Duplicate of this bug: 996118
Depends on: 777680
Depends on: 1016415
Blocks: 85871
Duplicate of this bug: 85871
No longer blocks: 1016405, 1016414, 85871
Depends on: 1016405, 1016414
No longer depends on: 1016120
Depends on: 1016582
Depends on: 1016735
No longer depends on: 1016735
Depends on: 1016887
Depends on: 1016917
No longer depends on: 1016887
The changes here will impact add-ons dealing with the context menu. We should devdoc that.
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?
Blocks: 1017580
Hi Juan, can I QA Contact be assigned to this bug for final verification.
Flags: needinfo?(jbecerra)
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.
Flags: needinfo?(jbecerra)
QA Contact: cornel.ionce
Depends on: 1018197
No longer depends on: 1016114
Depends on: 1018610
Depends on: 1018595
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.
Flags: needinfo?(cornel.ionce)
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.
Flags: needinfo?(cornel.ionce) → needinfo?(jaws)
Yes, customization will be tracked separately and will happen at some future point in time.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jaws)
Marking as [qa!] since verification is complete.
Whiteboard: p=13 s=it-32c-31a-30b.3 [qa+] → p=13 s=it-32c-31a-30b.3 [qa!]
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/
relnote-firefox: --- → ?
Depends on: 1061949
Depends on: 1064371
Depends on: 1044214
Depends on: 1075299
Depends on: 1075089
Depends on: 1087332
No longer depends on: 1061949
Depends on: 1061949
See Also: → 1083494
Depends on: 1083494
See Also: 1083494
Depends on: 1378301
Depends on: 1418097
You need to log in before you can comment on or make changes to this bug.