Closed Bug 1275432 Opened 4 years ago Closed 4 years ago

Dropdown arrow Containers submenu doesn’t have icons

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bram, Assigned: jkt)

References

(Blocks 1 open bug)

Details

(Whiteboard: [userContextId][domsecurity-active])

Attachments

(4 files)

It should come with icons, like the Containers submenu in File-Edit-View.
Bram, did you see this on Windows?  We need to check on other platforms too.
Whiteboard: [userContextId]
(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.
Attached image 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/
Assignee: nobody → jkingston
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.
Attachment #8756803 - Flags: review?(amarchesini)
Blocks: 1248639, 1272067
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?
Attachment #8756803 - Flags: review?(amarchesini)
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 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!
Attachment #8756803 - Flags: review?(paolo.mozmail)
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 :-(
Attachment #8756803 - Flags: review?(paolo.mozmail) → review?(jaws)
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!
Status: NEW → ASSIGNED
Priority: -- → P1
Blocks: 1276412
Whiteboard: [userContextId] → [userContextId][domsecurity-active]
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 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 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.
Attachment #8756803 - Flags: review?(jaws)
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).
Flags: needinfo?(jkingston)
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
Flags: needinfo?(jkingston)
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.
Attachment #8756803 - Flags: review?(jaws) → review+
(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?
Flags: needinfo?(jaws)
Ah yes that's exactly it. If you rebase while using reviewboard any oyher changes that happened in the interim appear in the diff.
Flags: needinfo?(jaws)
Cool thanks :jaws! I thought I was going crazy :D.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76b6cebf4b36
showing container icons in file and tab menu. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/76b6cebf4b36
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.