Remove support for obsolete -moz-images-in-menus and -moz-images-in-buttons media queries

RESOLVED FIXED in Firefox 52

Status

()

Core
Widget: Gtk
P2
normal
RESOLVED FIXED
11 months ago
5 months ago

People

(Reporter: dao, Assigned: jkt)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-complete})

Trunk
mozilla52
All
Linux
addon-compat, dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox52 fixed)

Details

(Whiteboard: [userContextId])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

11 months ago
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.
(Assignee)

Comment 1

11 months ago
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.
(Assignee)

Comment 2

11 months ago
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?
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 3

11 months ago
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.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 4

11 months ago
Will look at this tomorrow to match Windows. Thanks!
Assignee: nobody → jkt
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 7

11 months ago
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.
Attachment #8790809 - Flags: review?(dao+bmo) → review+
(Reporter)

Comment 8

11 months ago
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.
Attachment #8790810 - Flags: review?(dao+bmo) → review+
(Reporter)

Comment 9

11 months ago
(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 hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8790810 - Attachment is obsolete: true
(Assignee)

Updated

11 months ago
Attachment #8790810 - Attachment is obsolete: false
(Reporter)

Comment 11

11 months ago
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
Attachment #8790809 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
(Reporter)

Updated

11 months ago
Attachment #8790809 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 13

11 months ago
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!
Attachment #8790810 - Flags: review?(karlt)
Attachment #8790810 - Flags: review?(dholbert)
(Assignee)

Comment 14

11 months ago
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 :)
Attachment #8790809 - Flags: review?(karlt)
Attachment #8790809 - Flags: review?(dholbert)
Attachment #8790809 - Flags: review+ → review?(dao+bmo)
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

11 months ago
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 :(
Attachment #8790810 - Attachment is obsolete: true
Attachment #8790810 - Flags: review?(karlt)
Attachment #8790810 - Flags: review?(dholbert)
(Reporter)

Updated

11 months ago
Attachment #8790809 - Flags: review?(dao+bmo) → review+
(Reporter)

Updated

11 months ago
Attachment #8791271 - Flags: review?(dao+bmo) → review+

Comment 21

11 months ago
mozreview-review
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.
Attachment #8790809 - Flags: review?(dholbert) → review+

Comment 22

11 months ago
mozreview-review
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
Attachment #8791271 - Flags: review?(dholbert) → review+
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 hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8791271 - Attachment is obsolete: true
Attachment #8791271 - Flags: review?(karlt)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8790809 - Attachment is obsolete: true
Attachment #8790809 - Flags: review?(karlt)
Attachment #8790809 - Flags: review?(dao+bmo)
(Assignee)

Updated

11 months ago
Attachment #8790809 - Attachment is obsolete: false
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...
Flags: needinfo?(jkt)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8791385 - Attachment is obsolete: true
Attachment #8791385 - Flags: review?(karlt)
Attachment #8791385 - Flags: review?(dholbert)
Attachment #8791385 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8791451 - Attachment is obsolete: true
Attachment #8791451 - Flags: review?(karlt)
Attachment #8791451 - Flags: review?(dholbert)
Attachment #8791451 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8790809 - Attachment is obsolete: true
Flags: needinfo?(jkt)
(Assignee)

Comment 30

11 months ago
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.
Flags: needinfo?(dholbert)

Comment 31

11 months ago
mozreview-review
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.)
Attachment #8791452 - Flags: review?(dholbert) → review+
(Reporter)

Updated

11 months ago
Attachment #8791452 - Flags: review?(dao+bmo) → review+
Keywords: dev-doc-needed
Flags: needinfo?(dholbert)
Blocks: 1191418
Priority: -- → P2
Whiteboard: [userContextId]
(Reporter)

Updated

10 months ago
Summary: Remove -moz-images-in-menus media query → Remove support for obsolete -moz-images-in-menus and -moz-images-in-buttons media queries
(Assignee)

Comment 32

10 months ago
Hi Karl,
Are you the right person to review this or is there someone else available to check it out?
Thanks
Flags: needinfo?(karlt)
(Reporter)

Updated

10 months ago
Blocks: 996532
(Assignee)

Comment 33

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

Thanks
Flags: needinfo?(bzbarsky)

Comment 34

10 months ago
mozreview-review
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.
Attachment #8791452 - Flags: review+
(Reporter)

Comment 35

10 months ago
FWIW I don't think this needs ui-review.
Flags: needinfo?(karlt)
Flags: needinfo?(bzbarsky)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

10 months ago
Rebased patch, no changes made other than fixing the rebase.
Keywords: checkin-needed

Comment 38

10 months ago
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
Keywords: checkin-needed

Comment 39

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e7680c5fead3
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Reporter)

Updated

10 months ago
Attachment #8791452 - Flags: review?(dao+bmo) → review+

Comment 40

10 months ago
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.
(Assignee)

Comment 41

10 months ago
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

10 months ago
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.

Updated

10 months ago
Keywords: addon-compat

Updated

10 months ago
status-firefox51: affected → wontfix

Updated

8 months ago
Blocks: 1319355

Updated

8 months ago
Depends on: 1322059
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.