According to <https://developer.gnome.org/gtk3/stable/GtkSettings.html#GtkSettings--gtk-menu-images>, the gtk-menu-images setting is deprecated. So we should remove our -moz-images-in-menus media query and the code that hides icons in menus.

We should likely continue hiding the other menu images as they are not visible on other platforms either and mostly outdated last time I checked.

Without the CSS mentioned you see lots of unwanted icons, should we be removing these from the menus themselves also? Is there any instance that these should be shown?

I'd suggest we take our Windows theme as a guideline and remove the icons from those menu items that don't have an icon on Windows either.

Will look at this tomorrow to match Windows. Thanks!

Comment on attachment 8790809 [details] Bug 1302157 - Remove images-in-menu code (& associated icons) since it's deprecated in GTK. Please remove this too: https://dxr.mozilla.org/mozilla-central/rev/b1156b0eb96fcb49966b20e5fcf6a01f634ea2ee/browser/themes/linux/browser.css#1894-1897 This still needs review from Widget: Gtk and Layout peers.

Comment on attachment 8790810 [details] Bug 1302157 - removal of images-in-button code as GTK has deprecated this option, removed all relevant CSS parsing and icons. This also needs review from Widget: Gtk and Layout peers.

(In reply to Dão Gottwald [:dao] from comment #7) > Comment on attachment 8790809 [details] > Bug 1302157 - removal of images-in-menu code as GTK has deprecated this > option, removed all relevant CSS parsing and icons to match the windows > layout > > Please remove this too: > https://dxr.mozilla.org/mozilla-central/rev/ > b1156b0eb96fcb49966b20e5fcf6a01f634ea2ee/browser/themes/linux/browser. > css#1894-1897 And this can probably go as well: https://dxr.mozilla.org/mozilla-central/rev/b1156b0eb96fcb49966b20e5fcf6a01f634ea2ee/browser/themes/linux/browser.css#1880

Comment on attachment 8790809 [details] Bug 1302157 - Remove images-in-menu code (& associated icons) since it's deprecated in GTK. You haven't removed this: https://dxr.mozilla.org/mozilla-central/rev/b1156b0eb96fcb49966b20e5fcf6a01f634ea2ee/browser/themes/linux/browser.css#1894-1897

Comment on attachment 8790810 [details] Bug 1302157 - removal of images-in-button code as GTK has deprecated this option, removed all relevant CSS parsing and icons. Karl would you be able to review this, I see you are the module owner, I can't see any peers. Daniel you are a peer on layout would you be able to look at this also or to pass onto someone else please? Thanks!

Comment on attachment 8790809 [details] Bug 1302157 - Remove images-in-menu code (& associated icons) since it's deprecated in GTK. Same as other review on this bug. Thanks for your time in advance :)

It looks like mozreview doesn't pick up on review-flag changes that are made manually in bugzilla's own UI. (MozReview wasn't showing a review request in the mozreview UI, and it didn't seem to know that dao had r+'d it). I added myself and karlt at https://reviewboard.mozilla.org/r/78464/ (using the "pencil" icon), so that I can r+ it from MozReview. Unfortunately, that seems to have reset dao's r+ back to a r? somehow. So, dao, please re-r+ this if you don't mind. :) (I'll file a MozReview bug, too, if I can figure out reliable STR to trigger this.)

> (MozReview wasn't showing a review request in the mozreview UI, and it didn't seem to know that dao had r+'d it). I meant to say "wasn't showing a review request *for me or karl* in the mozreview UI"

(MozReview got a bit confused, too, because an obsoleted attachment (for a discarded mozreview request) was un-obsoleted here. Moral of the story: if you're using MozReview, don't manually edit any of Bugzilla's state about mozreview-managed attachments; information flows from mozreview to bugzilla, and not the other direction :)). I think jkt is re-pushing to mozreview with an updated commit list, which should generate a mozreview request that has both patches, and then all should be well. :)

Comment on attachment 8790810 [details] Bug 1302157 - removal of images-in-button code as GTK has deprecated this option, removed all relevant CSS parsing and icons. Obsoleting to clean up the mess caused by pushing patches in the wrong order :(

Comment on attachment 8790809 [details] Bug 1302157 - Remove images-in-menu code (& associated icons) since it's deprecated in GTK. https://reviewboard.mozilla.org/r/78464/#review77312 r=me on the /layout changes.

Comment on attachment 8791271 [details] Bug 1302157 - removal of images-in-button code as GTK has deprecated this option, removed all relevant CSS parsing and icons. https://reviewboard.mozilla.org/r/78728/#review77314 r=me

Optional grammar nit: so, right now the commit messages mix two different tenses ("removal of... removed..."), neither of which are the predominant tense that we use in commit messages (for gecko patches at least), which is the present tense command/active form ("Remove ...") Consider rewriting both commit messages as something like the following (which is IMO clearer): > Bug 1302157 - Remove images-in-menu code (& associated icons) since it's deprecated in GTK. Not a big deal, though.

I think in your most recent push, you might've just pushed the first patch again (instead of pushing both patches). At least, the MozReview UI is back to only showing me a single patch instead of showing me both...

I combined it into one commit, whatever I tried it just pushed the other out of mozreview for some reason. I'm not really sure if they needed to be seperate patches in the first place to be fair.

Comment on attachment 8791452 [details] Bug 1302157 - Remove images-in-menu and images-in-button code (& associated icons) since it's deprecated in GTK. https://reviewboard.mozilla.org/r/78856/#review77488 Agreed, this is fine as one patch. r=me (Sorry for whatever was giving you trouble with hg <--> mozreview interactions there. :-/ If it's possible to figure out what the problem was, that would be awesome in the interests of fixing whatever was broken, and/or documenting what the footgun was & how to avoid it.) (I'm afraid this probably hasn't increased dao's faith in MozReview :) though I suspect the culprit towards the end here was more likely to be our local hg extensions (and something either broken or unintuitive there), rather than the MozReview UI itself.)

Hi Karl, Are you the right person to review this or is there someone else available to check it out? Thanks

Bz it looks like you have reviewed this code in the past would you be able to stand in for Karl's approval? Thanks

Comment on attachment 8791452 [details] Bug 1302157 - Remove images-in-menu and images-in-button code (& associated icons) since it's deprecated in GTK. https://reviewboard.mozilla.org/r/78856/#review79974 r=me on the widget/ changes, assuming the overall UX change is fine.

FWIW I don't think this needs ui-review.

Rebased patch, no changes made other than fixing the rebase.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e7680c5fead3 Remove images-in-menu and images-in-button code (& associated icons) since it's deprecated in GTK. r=bz,dholbert

So is icons for menus completely gone now or is there anyway to get them back in firefox? Text only menus is much more cumbersome to use then ones with icons.

The patch removes all the menu icons that are not in other platforms, this brings it in parity with all the others. The distros I looked at had it turned off by default anyway. Perhaps we could opt for a different theme to add the icons back in?

From what I've been told by gnome folks (though this could definitely be wrong). Is that the pref was removed in favor of apps having there own options to control if icons should be shown or not. I don't think there is any plans to remove the icon resources.

I've checked MDN, and we don't seem to mention these media types anywhere. I've added a note to the Firefox 52 release notes: https://developer.mozilla.org/en-US/Firefox/Releases/52#CSS