Closed Bug 1016582 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: ntim, Assigned: Gijs)

References

Details

(Whiteboard: p=2 s=it-32c-31a-30b.3 [qa!])

Attachments

(1 file, 1 obsolete file)

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.
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws) → needinfo?(sfranks)
If we are keeping it at the same place, we should at least use a full width separator.
Visually, I think we should *always* place these navigation items at the top.
(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.
Flags: needinfo?(sfranks)
This seems like much the simplest solution here.
Attachment #8430719 - Flags: review?(jaws)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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? :-)
Flags: needinfo?(mmucci)
Whiteboard: p=2
Added to Iteration 32.3
Flags: needinfo?(mmucci) → firefox-backlog+
Whiteboard: p=2 → p=2 s=it-32c-31a-30b.3 [qa?]
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.
Attachment #8430719 - Flags: review?(jaws)
(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 ?
Flags: needinfo?(jaws)
Flags: needinfo?(jaws)
Hi Gijs, any recommendation if this bug should be set to [qa+] or [qa-] for verification.
Flags: needinfo?(gijskruitbosch+bugs)
QA+ because I'm seeing differences already between how our automated tests see things and how it works on e.g. MDN.
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: p=2 s=it-32c-31a-30b.3 [qa?] → p=2 s=it-32c-31a-30b.3 [qa+]
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/.
Attachment #8430719 - Attachment is obsolete: true
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?
Attachment #8432445 - Flags: feedback?(jaws)
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.
Attachment #8432445 - Flags: feedback?(jaws) → review+
Thanks!

remote:   https://hg.mozilla.org/integration/fx-team/rev/df7a4c3e5eaf
Whiteboard: p=2 s=it-32c-31a-30b.3 [qa+] → [fixed-in-fx-team] p=2 s=it-32c-31a-30b.3 [qa+]
https://hg.mozilla.org/mozilla-central/rev/df7a4c3e5eaf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team] p=2 s=it-32c-31a-30b.3 [qa+] → p=2 s=it-32c-31a-30b.3 [qa+]
Target Milestone: --- → Firefox 32
QA Contact: cornel.ionce
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.
Status: RESOLVED → VERIFIED
Whiteboard: p=2 s=it-32c-31a-30b.3 [qa+] → p=2 s=it-32c-31a-30b.3 [qa!]
You need to log in before you can comment on or make changes to this bug.