1016582 - Make HTML 5 context menus show below the navigation context menu items

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Tim Nguyen :ntim]

STR :
- Go on an MDN wiki page (https://developer.mozilla.org/en-US/docs/Web/CSS)
- Right click
- See that there are 2 menu items on top of the navigation items.

It feels really weird.

Comment 1

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

Sevaan, what do you think we should do here? Should the navigation items *always* be on the top? Traditionally we have let webpages place their own context menus at the top.

Comment 2

[Tim Nguyen :ntim]

If we are keeping it at the same place, we should at least use a full width separator.

Comment 4

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

Visually, I think we should *always* place these navigation items at the top.

Comment 5

[Sevaan Franks [:sevaan]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Visually, I think we should *always* place these navigation items at the top.

Definitely agree. Because they are iconic as oppose to textual, we can consider the navigation items separate to the rest of the context menu and part of the "container". As such, the webpages custom context menu entries can still "be at the top"...but that just means above other text entires, not outside of the container.

Comment 6

[:Gijs (he/him)]

Attachment: pin new back/fwd/refresh/stop/star group at the top of the context menu,

This seems like much the simplest solution here.

Comment 7

[:Gijs (he/him)]

Marco, this is fallout from bug 1000513, can we add this to this iteration, too, so we can be sure we ship a nice thing for 32? :-)

Comment 8

[Marco Mucci [:MarcoM]]

Added to Iteration 32.3

Comment 9

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

Comment on attachment 8430719 [details] [diff] [review]
pin new back/fwd/refresh/stop/star group at the top of the context menu,

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

Thank you for helping out with the context menu work.

::: browser/themes/shared/contextmenu.inc.css
@@ +4,5 @@
>  }
>  
> +#context-sep-navigation,
> +#context-navigation {
> +  -moz-box-ordinal-group: 0;

I like the novelty of this approach, but I do think we should actually move these items within browser-context.inc, even if we still need to do this.

Comment 10

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Comment on attachment 8430719 [details] [diff] [review]
> pin new back/fwd/refresh/stop/star group at the top of the context menu,
> 
> Review of attachment 8430719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for helping out with the context menu work.
> 
> ::: browser/themes/shared/contextmenu.inc.css
> @@ +4,5 @@
> >  }
> >  
> > +#context-sep-navigation,
> > +#context-navigation {
> > +  -moz-box-ordinal-group: 0;
> 
> I like the novelty of this approach, but I do think we should actually move
> these items within browser-context.inc, even if we still need to do this.

I don't think that will help for the HTML5 case. Are you saying we should do and/and ?

Comment 11

[Marco Mucci [:MarcoM]]

Hi Gijs, any recommendation if this bug should be set to [qa+] or [qa-] for verification.

Comment 12

[:Gijs (he/him)]

QA+ because I'm seeing differences already between how our automated tests see things and how it works on e.g. MDN.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8430719 [details] [diff] [review]
pin new back/fwd/refresh/stop/star group at the top of the context menu,

>--- a/browser/themes/shared/contextmenu.inc.css
>+++ b/browser/themes/shared/contextmenu.inc.css
>@@ -1,15 +1,20 @@
> 
> menugroup > .menuitem-iconic[disabled="true"] > .menu-iconic-left {
>   opacity: .3;
> }
> 
>+#context-sep-navigation,
>+#context-navigation {
>+  -moz-box-ordinal-group: 0;
>+}

This doesn't look like it belongs in browser/themes/.

Comment 14

[:Gijs (he/him)]

Attachment: pin new back/fwd/refresh/stop/star group at the top of the context menu,

This works. Try push: https://tbpl.mozilla.org/?tree=Try&rev=077a9b73383a . (Hoping it works there, too... hence only feedback). Note that this puts these items above the 'leave fullscreen' and CTP items. AIUI, that's intentional. Is that right, Jared?

Comment 15

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

Comment on attachment 8432445 [details] [diff] [review]
pin new back/fwd/refresh/stop/star group at the top of the context menu,

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

Yes, it was intentional to place the "Exit fullscreen" and CTP menuitems at the top before, but with this new design they should be placed beneath the navigation items. We hope that visually it shouldn't seem so different given the nature of the visual difference between the <menugroup> and the standalone <menuitem>s.

::: browser/base/content/test/general/test_contextmenu.html
@@ +82,5 @@
>      inspectItems = ["---", null,
>                      "context-inspect", true];
>    }
>  
> +  info("Checking case " + testNum);

This line shouldn't be necessary as line 78 above is basically the same thing.

Comment 16

[:Gijs (he/him)]

Thanks!

remote:   https://hg.mozilla.org/integration/fx-team/rev/df7a4c3e5eaf

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/df7a4c3e5eaf

Comment 18

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

Mozilla/5.0 (Windows NT 6.3; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (X11; Linux x86_64; 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

Verified fixed on Windows 7 64bit, Ubuntu 13.04 64bit and Mac OS X 10.9 using latest Nightly, build ID: 20140603030220. The navigation items are now displayed in the top.