Remove icon for "Undo Close Tab" in the All Tabs menu

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Theme
P3
normal
VERIFIED FIXED
6 months ago
4 months ago

People

(Reporter: florian, Assigned: Bharat Raghunathan, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 55
good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [good first bug][lang=css])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
Caught by the test in bug 1316187.

I think we just need to copy the code at http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/browser/themes/windows/browser.css#943 to http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/browser/themes/linux/browser.css#541

Comment 1

5 months ago
This icon is pretty ugly, imho, and we should just remove it altogether.

Bug 1014313 comment 0 says "Everything in this menu has an icon, so this needs one too." This is bogus. Items with favicons get that icon displayed, as it should be, but this doesn't mean everything else in the menu needs an icon. The bookmarks and history menus can be seen as a role model here. In fact the All Tabs menu now has a new "New Container Tab" entry that doesn't have an icon either.

Here's what's needed to fix this bug:

1. Remove all rules involving #alltabs_undoCloseTab from browser/themes/linux/browser.css, browser/themes/osx/browser.css and browser/themes/windows/browser.css

2. Remove class="menuitem-iconic" here: http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/browser/base/content/browser.xul#620

3. Remove undoCloseTab.png and undoCloseTab@2x.png entries here: http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/browser/themes/shared/jar.inc.mn#113-114

4. Actually remove browser/themes/shared/undoCloseTab.png and browser/themes/shared/undoCloseTab@2x.png
Blocks: 1014313
Mentor: dao+bmo@mozilla.com
Keywords: good-first-bug
OS: Linux → All
Priority: -- → P3
Hardware: Unspecified → All
Summary: chrome://browser/skin/undoCloseTab@2x.png is packaged but unused on Linux → Remove icon for "Undo Close Tab" in the All Tabs menu
Whiteboard: [good first bug][lang=css]
Version: 53 Branch → Trunk
Comment hidden (mozreview-request)
(Assignee)

Comment 3

5 months ago
Comment on attachment 8848384 [details]
Bug 1343827 - Removed icon for "Undo Closed Tab" and its references

Are there any more #alltabs_undoClosedTab rules to be removed or any more instances of class="menuitem-iconic" to be removed?
Flags: needinfo?(dao+bmo)
Attachment #8848384 - Flags: review?(dao+bmo)

Comment 4

5 months ago
mozreview-review
Comment on attachment 8848384 [details]
Bug 1343827 - Removed icon for "Undo Closed Tab" and its references

https://reviewboard.mozilla.org/r/121284/#review123376

::: browser/base/content/browser.xul:1
(Diff revision 1)
> -#filter substitution
> +	#filter substitution

You accidentally added a tab stop here. Please revert this.

::: browser/themes/linux/browser.css
(Diff revision 1)
> -.unified-nav-forward[_moz-menuactive] {
> -  list-style-image: url("moz-icon://stock/gtk-go-forward-ltr?size=menu") !important;
> -}
> -.unified-nav-forward[_moz-menuactive]:-moz-locale-dir(rtl) {
> -  list-style-image: url("moz-icon://stock/gtk-go-forward-rtl?size=menu") !important;
> -}

The .unified-nav-back and .unified-nav-forward rules are unrelated to Undo Close Tab. We need to keep these.

Looks good otherwise. Thanks!
Attachment #8848384 - Flags: review?(dao+bmo) → review-

Updated

5 months ago
Assignee: nobody → bharatraghunthan9767
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

5 months ago
Comment on attachment 8848384 [details]
Bug 1343827 - Removed icon for "Undo Closed Tab" and its references

Is this ok or any more whitespace changes needed? (./mach eslint always keeps showing 0 problems,0 errors so please pardon if I couldn't find them)
Flags: needinfo?(dao+bmo)
Attachment #8848384 - Flags: review?(dao+bmo)

Comment 7

5 months ago
mozreview-review
Comment on attachment 8848384 [details]
Bug 1343827 - Removed icon for "Undo Closed Tab" and its references

https://reviewboard.mozilla.org/r/121284/#review123412

::: browser/themes/linux/browser.css:551
(Diff revision 2)
>  }
>  .unified-nav-forward[_moz-menuactive] {
> -  list-style-image: url("moz-icon://stock/gtk-go-forward-ltr?size=menu") !important;
> +  list-style-image: url("moz-icon://stock/gtk-go-forward-ltr?size=menu") !important;	
>  }
>  .unified-nav-forward[_moz-menuactive]:-moz-locale-dir(rtl) {
> -  list-style-image: url("moz-icon://stock/gtk-go-forward-rtl?size=menu") !important;
> +  list-style-image: url("moz-icon://stock/gtk-go-forward-rtl?size=menu") !important;	

you accidentally added tab stops here again :(
Attachment #8848384 - Flags: review?(dao+bmo) → review-

Updated

5 months ago
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

5 months ago
Comment on attachment 8848384 [details]
Bug 1343827 - Removed icon for "Undo Closed Tab" and its references

As I said, ESLint is not really helping me with regard to whitespace changes, could you please suggest on how I should check for accidental tabs and spaces myself before committing and not after committing on MozReview? (Even hg diff does not seem to be of any use)
Flags: needinfo?(dao+bmo)
Attachment #8848384 - Flags: review?(dao+bmo)
ESLint only covers JS, not CSS. hg diff should definitely show you these changes.
Flags: needinfo?(dao+bmo)

Comment 11

5 months ago
mozreview-review
Comment on attachment 8848384 [details]
Bug 1343827 - Removed icon for "Undo Closed Tab" and its references

https://reviewboard.mozilla.org/r/121284/#review123420

Looks good!
Attachment #8848384 - Flags: review?(dao+bmo) → review+

Comment 12

5 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14b74d9448cc
Removed icon for "Undo Closed Tab" and its references r=dao

Comment 13

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/14b74d9448cc
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Comment 14

5 months ago
Thanks, ::dao <dao+bmo@mozilla.com> !!
I have reproduce this bug with Nightly 54.0a1 (2017-03-02) (64- bit) in Windows 10.

This bug's fix is verified with latest Nightly 55.0a1.
 
Build ID   :	20170426030329
User Agent :	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170426]

Updated

4 months ago
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.