Last Comment Bug 1302157 - Remove support for obsolete -moz-images-in-menus and -moz-images-in-buttons media queries
: Remove support for obsolete -moz-images-in-menus and -moz-images-in-buttons m...
Status: RESOLVED FIXED
[userContextId]
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: Trunk
: All Linux
: P2 normal (vote)
: mozilla52
Assigned To: Jonathan Kingston [:jkt]
:
:
Mentors:
Depends on: 1322059
Blocks: ContextualIdentity 415810 996532 1319355
  Show dependency treegraph
 
Reported: 2016-09-12 10:16 PDT by Dão Gottwald [:dao]
Modified: 2017-01-23 06:53 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
Screenshot from 2016-09-12 18-35-07.png (582.84 KB, image/png)
2016-09-12 10:37 PDT, Jonathan Kingston [:jkt]
no flags Details
Bug 1302157 - Remove images-in-menu code (& associated icons) since it's deprecated in GTK. (58 bytes, text/x-review-board-request)
2016-09-13 09:30 PDT, Jonathan Kingston [:jkt]
dholbert: review+
Details | Review
Bug 1302157 - removal of images-in-button code as GTK has deprecated this option, removed all relevant CSS parsing and icons. (58 bytes, text/x-review-board-request)
2016-09-13 09:30 PDT, Jonathan Kingston [:jkt]
dao+bmo: review+
Details | Review
Bug 1302157 - removal of images-in-button code as GTK has deprecated this option, removed all relevant CSS parsing and icons. (58 bytes, text/x-review-board-request)
2016-09-14 08:57 PDT, Jonathan Kingston [:jkt]
dao+bmo: review+
dholbert: review+
Details | Review
Bug 1302157 - Remove images-in-button code (& associated icons) since it's deprecated in GTK. (58 bytes, text/x-review-board-request)
2016-09-14 14:17 PDT, Jonathan Kingston [:jkt]
no flags Details | Review
Bug 1302157 - Remove images-in-menu code (& associated icons) since it's deprecated in GTK. (58 bytes, text/x-review-board-request)
2016-09-14 18:41 PDT, Jonathan Kingston [:jkt]
no flags Details | Review
Bug 1302157 - Remove images-in-menu and images-in-button code (& associated icons) since it's deprecated in GTK. (58 bytes, text/x-review-board-request)
2016-09-14 18:42 PDT, Jonathan Kingston [:jkt]
dao+bmo: review+
dholbert: review+
bzbarsky: review+
Details | Review

Description User image Dão Gottwald [:dao] 2016-09-12 10:16:53 PDT
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.
Comment 1 User image Jonathan Kingston [:jkt] 2016-09-12 10:22:50 PDT
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.
Comment 2 User image Jonathan Kingston [:jkt] 2016-09-12 10:37:19 PDT
Created attachment 8790366 [details]
Screenshot from 2016-09-12 18-35-07.png

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?
Comment 3 User image Dão Gottwald [:dao] 2016-09-12 10:43:48 PDT
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.
Comment 4 User image Jonathan Kingston [:jkt] 2016-09-12 11:08:23 PDT
Will look at this tomorrow to match Windows. Thanks!
Comment 5 User image Jonathan Kingston [:jkt] 2016-09-13 09:30:41 PDT Comment hidden (mozreview-request)
Comment 6 User image Jonathan Kingston [:jkt] 2016-09-13 09:30:41 PDT Comment hidden (mozreview-request)
Comment 7 User image Dão Gottwald [:dao] 2016-09-13 09:41:18 PDT
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 8 User image Dão Gottwald [:dao] 2016-09-13 09:41:36 PDT
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.
Comment 9 User image Dão Gottwald [:dao] 2016-09-13 09:43:11 PDT
(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 10 User image Jonathan Kingston [:jkt] 2016-09-13 12:07:17 PDT Comment hidden (mozreview-request)
Comment 11 User image Dão Gottwald [:dao] 2016-09-13 12:33:38 PDT
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 12 User image Jonathan Kingston [:jkt] 2016-09-13 15:54:40 PDT Comment hidden (mozreview-request)
Comment 13 User image Jonathan Kingston [:jkt] 2016-09-14 01:46:10 PDT
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 14 User image Jonathan Kingston [:jkt] 2016-09-14 01:46:24 PDT
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 :)
Comment 15 User image Daniel Holbert [:dholbert] 2016-09-14 08:09:25 PDT
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.)
Comment 16 User image Daniel Holbert [:dholbert] 2016-09-14 08:11:33 PDT
> (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"
Comment 17 User image Daniel Holbert [:dholbert] 2016-09-14 08:45:32 PDT
(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 18 User image Jonathan Kingston [:jkt] 2016-09-14 08:57:41 PDT Comment hidden (mozreview-request)
Comment 19 User image Jonathan Kingston [:jkt] 2016-09-14 08:57:41 PDT Comment hidden (mozreview-request)
Comment 20 User image Jonathan Kingston [:jkt] 2016-09-14 09:02:48 PDT
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 21 User image Daniel Holbert [:dholbert] 2016-09-14 09:19:57 PDT
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 22 User image Daniel Holbert [:dholbert] 2016-09-14 09:22:46 PDT
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
Comment 23 User image Daniel Holbert [:dholbert] 2016-09-14 09:29:28 PDT
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.
Comment 24 User image Jonathan Kingston [:jkt] 2016-09-14 14:16:23 PDT Comment hidden (mozreview-request)
Comment 25 User image Jonathan Kingston [:jkt] 2016-09-14 14:17:14 PDT Comment hidden (mozreview-request)
Comment 26 User image Daniel Holbert [:dholbert] 2016-09-14 15:42:23 PDT
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...
Comment 27 User image Jonathan Kingston [:jkt] 2016-09-14 18:41:22 PDT Comment hidden (mozreview-request)
Comment 28 User image Jonathan Kingston [:jkt] 2016-09-14 18:42:50 PDT Comment hidden (mozreview-request)
Comment 29 User image Jonathan Kingston [:jkt] 2016-09-14 18:46:41 PDT Comment hidden (mozreview-request)
Comment 30 User image Jonathan Kingston [:jkt] 2016-09-14 18:50:23 PDT
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 31 User image Daniel Holbert [:dholbert] 2016-09-14 20:01:24 PDT
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.)
Comment 32 User image Jonathan Kingston [:jkt] 2016-09-20 15:45:00 PDT
Hi Karl,
Are you the right person to review this or is there someone else available to check it out?
Thanks
Comment 33 User image Jonathan Kingston [:jkt] 2016-09-27 06:27:25 PDT
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 34 User image Boris Zbarsky [:bz] (still a bit busy) 2016-09-27 08:03:48 PDT
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.
Comment 35 User image Dão Gottwald [:dao] 2016-09-27 09:41:22 PDT
FWIW I don't think this needs ui-review.
Comment 36 User image Jonathan Kingston [:jkt] 2016-09-28 17:38:03 PDT Comment hidden (mozreview-request)
Comment 37 User image Jonathan Kingston [:jkt] 2016-09-28 17:40:12 PDT
Rebased patch, no changes made other than fixing the rebase.
Comment 38 User image Pulsebot 2016-09-28 17:46:41 PDT
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
Comment 39 User image Carsten Book [:Tomcat] 2016-09-29 02:47:00 PDT
https://hg.mozilla.org/mozilla-central/rev/e7680c5fead3
Comment 40 User image Cork 2016-09-30 13:00:52 PDT
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.
Comment 41 User image Jonathan Kingston [:jkt] 2016-09-30 14:10:56 PDT
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?
Comment 42 User image Cork 2016-10-01 09:42:54 PDT
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.
Comment 43 User image Chris Mills (Mozilla, MDN editor) [:cmills] 2017-01-23 06:53:37 PST
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

Note You need to log in before you can comment on or make changes to this bug.