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)
Firefox
Menus
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)
15.59 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(jaws)
Comment 1•9 years ago
|
||
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)
Reporter | ||
Comment 2•9 years ago
|
||
If we are keeping it at the same place, we should at least use a full width separator.
Comment 4•9 years ago
|
||
Visually, I think we should *always* place these navigation items at the top.
Comment 5•9 years ago
|
||
(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)
Assignee | ||
Comment 6•9 years ago
|
||
This seems like much the simplest solution here.
Attachment #8430719 -
Flags: review?(jaws)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•9 years ago
|
||
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
Comment 8•9 years ago
|
||
Added to Iteration 32.3
Flags: needinfo?(mmucci) → firefox-backlog+
Whiteboard: p=2 → p=2 s=it-32c-31a-30b.3 [qa?]
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
(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)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(jaws)
Comment 11•9 years ago
|
||
Hi Gijs, any recommendation if this bug should be set to [qa+] or [qa-] for verification.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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/.
Assignee | ||
Updated•9 years ago
|
Attachment #8430719 -
Attachment is obsolete: true
Assignee | ||
Comment 14•9 years ago
|
||
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 15•9 years ago
|
||
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+
Assignee | ||
Comment 16•9 years ago
|
||
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+]
Comment 17•9 years ago
|
||
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
Updated•9 years ago
|
QA Contact: cornel.ionce
Comment 18•9 years ago
|
||
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.
Description
•