1248639 - adding images under the Open link in a Container Tab context menu

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[Kamil Jozwiak [:kjozwiak]]

I think it would be a good idea if we added the "container" images that are found under "File -> New Container Tab" into the "Open link in a Container Tab" context menu. This way, it will be easier for users to distinguish the difference between each container and would match the file menu. However, I'm not sure if there's any internal design rules about adding images into context menu's.

Comment 1

[Tanvi Vyas[:tanvi]]

Paolo, can we have images in the context menu?

Comment 2

[:Paolo Amadini]

That's technically possible, if this is what you're asking. Whether this is appropriate here is a question for the UX team, but personally I don't see why we couldn't do that in the submenu, as it improves clarity.

Comment 3

[Alfred Kayser]

Attachment: container_icons.png

Like this?

This is my theme "full themes": Nautipolis, Walnut, Walnut2, LittleFox, MicroFox, Bricks and Metal.

This is why having "full themes" is important, like addons it allows to develop and test extensions and new functionality without directly impacting main development.

Comment 4

[Tanvi Vyas[:tanvi]]

(In reply to :Paolo Amadini from comment #2)
> That's technically possible, if this is what you're asking. Whether this is
> appropriate here is a question for the UX team, but personally I don't see
> why we couldn't do that in the submenu, as it improves clarity.

Bram, are you okay with this?  Can we add the images that are in the file menu to the context menu?

Comment 5

[Bram Pitoyo [:bram]]

Absolutely yes. My original proposal is for the coloured images to always appear on every menu it’s put on.

For example, Container has an access point on the tab bar (we’re working on this feature at the moment). When this access point is clicked, a menu shows a list. And on this menu, images should appear on the side of each container name.

Comment 6

[Jonathan Kingston [:jkt] he/him]

Attachment: MozReview Request: Bug: 1248639 - Making GTK icons visible for the new container menu

Review commit: https://reviewboard.mozilla.org/r/53212/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53212/

Comment 7

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0f391c9eb05

Comment 8

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8753311 [details]
MozReview Request: Bug: 1248639 - Making GTK icons visible for the new container menu

This is just to enable the menu icons for GTK as windows and osx already have these icons. The work was done it was just hidden.

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 8753311 [details]
MozReview Request: Bug: 1248639 - Making GTK icons visible for the new container menu

https://reviewboard.mozilla.org/r/53212/#review50068

::: toolkit/content/xul.css:410
(Diff revision 1)
>    :-moz-any(menuitem[type], .menuitem-with-favicon) > .menu-iconic-left {
>      visibility: visible;
>    }
> +  #menu_newUserContext > menupopup > menuitem > .menu-iconic-left {
> +    visibility: visible;
> +  }

This kind of change should be in a theme CSS file, not in xul.css.

Also, as the line above this implies, setting the right class on the menuitem should make this work, no CSS needed.

Comment 10

[Jonathan Kingston [:jkt] he/him]

Attachment: MozReview Request: Bug: 1248639 - Making GTK icons visible for the new container menu

Review commit: https://reviewboard.mozilla.org/r/53336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53336/

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 8753531 [details]
MozReview Request: Bug: 1248639 - Making GTK icons visible for the new container menu

https://reviewboard.mozilla.org/r/53336/#review52038

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 13

[Jonathan Kingston [:jkt] he/him]

This should be resolved by: 1275432

Comment 14

[Tanvi Vyas[:tanvi]]

Hi Kamil,

Can you check if this is fixed by bug 1275432, and if it is, mark as closed duplicate?

Thank you!

Comment 15

[Kamil Jozwiak [:kjozwiak]]

Attachment: issue.png

This is still broken under Linux/Ubuntu.

* attached an image to illustrate the issue

Test Cases Used:

* ensured that the images are being displayed when using about: window/tabs
* ensured that the images are being displayed when using non-e10s window/tabs
* ensured that the images match the correct container in the context menu
* ensured that there's no UI/UX issues (burry images, not correctly aligned etc..)
* ensured each context menu opened the correct container with the correct image/color
* ensured that the context menu is not available in PB

Platforms Tested:

* OSX 10.11.5 x64 - PASSED
** using fx50.0a1 buildID: 20160608030219, changeset: f8ad071a6e14

* Win 10 Pro x64 - PASSED
** using fx50.0a1, buildID: 20160608030219, changeset: f8ad071a6e14

* Ubuntu 14.04.4 x64 - FAILED
** using fx50.0a1, buidID: 20160608030219, changeset: f8ad071a6e14

Comment 16

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1248639 - Adding in context menu icons for gtk layout.

Review commit: https://reviewboard.mozilla.org/r/58684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58684/

Comment 17

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8761576 [details]
Bug 1248639 - Adding in context menu icons for gtk layout.

This is the right bug number now, sorry about that.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8761576 [details]
Bug 1248639 - Adding in context menu icons for gtk layout.

https://reviewboard.mozilla.org/r/58684/#review55730

This is confusing. Why do we need both selectors? Why doesn't the existing one work but the new does, when the command attribute is set on these items, judging by the code from the other bug? If only the new one works, why do we need the existing one?

Orthogonally, this patch on its own is confusing without context - no images are set in it. For the next review, please ask :jaws as he reviewed the other patch. :-)

Comment 19

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1248639 - Adding in context menu icons for gtk layout.

Review commit: https://reviewboard.mozilla.org/r/58890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58890/

Comment 20

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8761825 [details]
Bug 1248639 - Adding in context menu icons for gtk layout.

Thanks Gijs, I didn't want to change it mostly as I was being paranoid, I checked all the menus are still working.

The latest patches elsewhere make the usercontext selector work everywhere now it seems which is great is it's way more understandable.

Pushing to :jaws for review.

Comment 21

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

Comment on attachment 8761825 [details]
Bug 1248639 - Adding in context menu icons for gtk layout.

https://reviewboard.mozilla.org/r/58890/#review55922

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/ec173d9293ab
Adding in context menu icons for gtk layout. r=jaws

Comment 23

[Carsten Book [:Tomcat]]

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

Comment 24

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I assume we don't need to uplift UI stuff for test pilot since the test pilot UI will be an addon?

Comment 25

[Kamil Jozwiak [:kjozwiak]]

Using the test cases from comment # 15, I made sure that the container images are correctly being displayed under the context menu on the following platforms:

* Ubuntu 14.04.4 LTS x64 VM - PASSED
** fx50.0a1 buildID: 20160706030233, changeset: 95ffbc4ff635

* Win 10 x64 VM (Version 1511, OS Build: 10586.318) - PASSED
** fx50.0a1 buildID: 20160706030233, changeset: 95ffbc4ff635

* OSX 10.11.5 x64: PASSED
** fx50.0a1 buildID: 20160706030233, changeset: 95ffbc4ff635

Link to the build being used for verification:
* https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-06-03-02-33-mozilla-central/

Comment 26

[Saheda Reza [:Antora]]

Reproduced this on Firefox nightly according to (2016-02-16)  

It's verified on Latest Nightly--Build ID:(20160711034039),User Agent: Mozilla/5.0 (Windows NT 10.0; rv:50.0) Gecko/20100101 Firefox/50.0


Tested OS--Windows10 32bit

Comment 27

[Kamil Jozwiak [:kjozwiak]]

(In reply to Saheda Reza [:Antora] from comment #26)
> Reproduced this on Firefox nightly according to (2016-02-16)  
> 
> It's verified on Latest Nightly--Build ID:(20160711034039),User Agent:
> Mozilla/5.0 (Windows NT 10.0; rv:50.0) Gecko/20100101 Firefox/50.0
> 
> 
> Tested OS--Windows10 32bit

Thanks for going through the issue and verifying that it's working under Win 10 x86 :)

Comment 28

[Farhadur Reja Fahim]

I have reproduced this bug with Nightly 47.0a1 (2016-02-16) on Ubuntu 14.04, 64 bit!

The bug's fix is now verified on latest Aurora 50.0a2.

Aurora 50.0a2:
Build ID 	20160824004001
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0

[bugday-20160824]

Comment 29

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

Thanks!