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

VERIFIED FIXED in Firefox 55

Status

()

enhancement
P3
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: florian, Assigned: bharatraghunthan9767, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

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

Attachments

(1 attachment)

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
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 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 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-
Assignee: nobody → bharatraghunthan9767
Flags: needinfo?(dao+bmo)
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 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-
Flags: needinfo?(dao+bmo)
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 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+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14b74d9448cc
Removed icon for "Undo Closed Tab" and its references r=dao
https://hg.mozilla.org/mozilla-central/rev/14b74d9448cc
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.