1275432 - Dropdown arrow Containers submenu doesn’t have icons

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Bram Pitoyo [:bram]]

Attachment: containers dropdown menu

It should come with icons, like the Containers submenu in File-Edit-View.

Comment 1

[Tanvi Vyas[:tanvi]]

Bram, did you see this on Windows?  We need to check on other platforms too.

Comment 2

[Bram Pitoyo [:bram]]

Attachment: containers dropdown menu (windows)

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #1)
> Bram, did you see this on Windows?  We need to check on other platforms too.

No. I didn’t see this on Windows. I assume that the icon doesn’t exist on Linux, either, but I’ll double check.

Comment 3

[Kamil Jozwiak [:kjozwiak]]

Attachment: dropdownSubmenu.png

Bram, apologies it took me so long to check if this is affecting linux (as per our IRC conversation).. I was having internet issues here at home :/ It looks like Ubuntu 14.04.4 LTS is affected as well.. The icons are missing from the following locations:

* File Menu (File -> New Container Tab)
* Dropdown Arrow Submenu (+ -> New Container Tab)

Used the following build:

* fx49.0a1 buildId: 20160524073714, changeset: 829d3be6ba64
* https://archive.mozilla.org/pub/firefox/nightly/2016/05/2016-05-24-07-37-14-mozilla-central/

Comment 4

[Jonathan Kingston [:jkt] he/him]

Attachment: MozReview Request: Bug 1275432 - showing container icons in file and tab menu.

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

Comment 5

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8756803 [details]
MozReview Request: Bug 1275432 - showing container icons in file and tab menu.

Also resolves 1248639 as the file that change was in got removed in a refactor.

Comment 6

[:Paolo Amadini]

Comment on attachment 8756803 [details]
MozReview Request: Bug 1275432 - showing container icons in file and tab menu.

I stumbled upon this while investigating an old unrelated issue with SVG in menus.

Jonathan, unless I'm missing something specific to the Containers project, you should always flag a Firefox peer for changes in "browser":

https://wiki.mozilla.org/Modules/All#Firefox

By the way, r=me for the simple change in utilityOverlay.js.

You definitely shouldn't change anything in "toolkit" for this bug. Some parts of Toolkit are close to being platform code shared between products, and should normally have no reference to like features in "browser" like the Containers front-end.

Is the change to that file really necessary? If so, why? Is there an alternative solution?

Comment 7

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8756803 [details]
MozReview Request: Bug 1275432 - showing container icons in file and tab menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55418/diff/1-2/

Comment 8

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8756803 [details]
MozReview Request: Bug 1275432 - showing container icons in file and tab menu.

Moved the code to browser.css this is specifically because gtk has lots of disabled icons, thanks for the heads up on the toolkit separation.

Without the CSS change icons in linux are not visible, as mentioned however this is far less severe when in browser/theme code.

Regarding the peer review, is there a page explaining or list of these people I should add?

Thanks!

Comment 9

[:Paolo Amadini]

Comment on attachment 8756803 [details]
MozReview Request: Bug 1275432 - showing container icons in file and tab menu.

(In reply to Jonathan Kingston [:kingstonTime] from comment #8)
> Without the CSS change icons in linux are not visible

It just seems suspect to me that setting an "image" attribute on a menuitem doesn't automatically show the image. Jared has worked on theming so he might know?

> Regarding the peer review, is there a page explaining or list of these
> people I should add?

I remember we had something called "submodules" so that people would know who to ask for review when touching a particular file. I couldn't find anything recent for Firefox though, only for Toolkit, so I guess your best bet is still to look at the reviewers in the hg log of the file you changed, and ask review from a peer that previously reviewed that file, who can redirect the review if necessary. It's a lot of paperwork, I know :-(

Comment 10

[Jonathan Kingston [:jkt] he/him]

Yeah the icons are hidden in gtk because there are lots in there by default that don't want to be shown. It's a new change that we have icons I believe and I'm guessing the existing icons that are there are from legacy days when every menuitem had an icon.

Yeah the suggested are usually good, I will do a blame and review the bug in future. Thanks!

Comment 11

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

Comment on attachment 8756803 [details]
MozReview Request: Bug 1275432 - showing container icons in file and tab menu.

https://reviewboard.mozilla.org/r/55418/#review53364

::: browser/themes/linux/browser.css:1992
(Diff revision 2)
> +menuitem[command="Browser:NewUserContextTab"] image {
> +  visibility: visible;
> +}

Please don't use the descendent selector for this. Using the descendent selector will first search the DOM for all <image> elements, then walk up the tree until the root element is processed before the <image> can be removed from the pile. The child selector will limit the tree traversal to the depth that the selector defines.

Further, is there a CSS class name that can be used instead of the tag name?

Comment 12

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8756803 [details]
MozReview Request: Bug 1275432 - showing container icons in file and tab menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55418/diff/2-3/

Comment 13

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8756803 [details]
MozReview Request: Bug 1275432 - showing container icons in file and tab menu.

Hi Jared, thanks for the first review.
This should cover your previous comments, I was foolish not to add the class to the dynamic list rather than get a nicer CSS selector as mentioned.

Comment 14

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

How do I enable containers in either Nightly or my local build? I don't see this menuitem in the File menu or the dropdown arrow at the end of the tabstrip. Bug 1276412 is not fixed, and there is no description in that bug about a pref (it mentions a build time flag).

Comment 15

[Jonathan Kingston [:jkt] he/him]

Hey Jared,

The preference to enable is: privacy.userContext.enabled

This change fixes the icons in the [file] menu and also the dropdown arrow in the tab row when there are a large amount of tabs. Other bugs solve other missing icons. Specifically Linux had the main issue here for showing icons at all in either menu but all OS needed the icon attribute change for the tab overflow menu.

Thanks

Comment 16

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

Comment on attachment 8756803 [details]
MozReview Request: Bug 1275432 - showing container icons in file and tab menu.

https://reviewboard.mozilla.org/r/55418/#review53514

r=me if you undo the indexedDB change. If that is necessary please re-request review.

::: browser/themes/linux/browser.css:1021
(Diff revisions 2 - 3)
> -.indexedDB-notification-icon,
> +.indexedDB-icon {
> -#indexedDB-notification-icon {
>    list-style-image: url(moz-icon://stock/gtk-dialog-question?size=16);

Why did this rule get changed? It doesn't seem related to contexts.

Comment 17

[Jonathan Kingston [:jkt] he/him]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> Comment on attachment 8756803 [details]
> MozReview Request: Bug 1275432 - showing container icons in file and tab
> menu.
> 
> https://reviewboard.mozilla.org/r/55418/#review53514
> 
> r=me if you undo the indexedDB change. If that is necessary please
> re-request review.
> 
> ::: browser/themes/linux/browser.css:1021
> (Diff revisions 2 - 3)
> > -.indexedDB-notification-icon,
> > +.indexedDB-icon {
> > -#indexedDB-notification-icon {
> >    list-style-image: url(moz-icon://stock/gtk-dialog-question?size=16);
> 
> Why did this rule get changed? It doesn't seem related to contexts.

I have no idea how that got there, I notice the change has happened in live(https://dxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#1021) however the squashed changeset doesn't do this (https://reviewboard.mozilla.org/r/55416/diff/3#index_header).


So my only guess is reviewboard considers some of the rebased code as the changeset?

Comment 18

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

Ah yes that's exactly it. If you rebase while using reviewboard any oyher changes that happened in the interim appear in the diff.

Comment 19

[Jonathan Kingston [:jkt] he/him]

Cool thanks :jaws! I thought I was going crazy :D.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76b6cebf4b36
showing container icons in file and tab menu. r=jaws

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76b6cebf4b36