Closed Bug 1302157 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Widget: Gtk, defect, P2)

Product:
Core
Component:
Widget: Gtk
Platform:
All
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: dao, Assigned: jkt)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-complete, Whiteboard: [userContextId])

Attachments

(2 files, 5 obsolete files)

Screenshot from 2016-09-12 18-35-07.png
582.84 KB, image/png
Details
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
dao
: review+
dholbert
: review+
bzbarsky
: review+
Details 
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?
Flags: needinfo?(dao+bmo)
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)
Will look at this tomorrow to match Windows. Thanks!
Assignee: nobody → jkt
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+
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+
(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
Attachment #8790810 - Attachment is obsolete: true
Attachment #8790810 - Attachment is obsolete: false
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)
Attachment #8790809 - Flags: review?(dao+bmo) → review+
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)
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 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)
Attachment #8790809 - Flags: review?(dao+bmo) → review+
Attachment #8791271 - Flags: review?(dao+bmo) → 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 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.
Attachment #8791271 - Attachment is obsolete: true
Attachment #8791271 - Flags: review?(karlt)
Attachment #8790809 - Attachment is obsolete: true
Attachment #8790809 - Flags: review?(karlt)
Attachment #8790809 - Flags: review?(dao+bmo)
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)
Attachment #8791385 - Attachment is obsolete: true
Attachment #8791385 - Flags: review?(karlt)
Attachment #8791385 - Flags: review?(dholbert)
Attachment #8791385 - Flags: review?(dao+bmo)
Attachment #8791451 - Attachment is obsolete: true
Attachment #8791451 - Flags: review?(karlt)
Attachment #8791451 - Flags: review?(dholbert)
Attachment #8791451 - Flags: review?(dao+bmo)
Attachment #8790809 - Attachment is obsolete: true
Flags: needinfo?(jkt)
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 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+
Attachment #8791452 - Flags: review?(dao+bmo) → review+
Flags: needinfo?(dholbert)
Blocks: ContextualIdentity
Priority: -- → P2
Whiteboard: [userContextId]
Summary: Remove -moz-images-in-menus media query → Remove support for obsolete -moz-images-in-menus and -moz-images-in-buttons media queries
Hi Karl,
Are you the right person to review this or is there someone else available to check it out?
Thanks
Flags: needinfo?(karlt)
Blocks: 996532
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 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+
FWIW I don't think this needs ui-review.
Flags: needinfo?(karlt)
Flags: needinfo?(bzbarsky)
Rebased patch, no changes made other than fixing the rebase.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/e7680c5fead3
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Attachment #8791452 - Flags: review?(dao+bmo) → review+
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.
Keywords: addon-compat
Blocks: 1319355
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
Blocks: 1625636
Blocks: 1625637
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: