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

Status: VERIFIED FIXED

(Firefox :: Theme, enhancement, P3)

Description

[Florian Quèze [:florian]]

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

[Dão Gottwald [::dao]]

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 [e-mail] 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/[e-mail]

Comment 2

[Bharat Raghunathan]

Attachment: Bug 1343827 - Removed icon for "Undo Closed Tab" and its references

Removed all rules of #alltabs_undoCloseTab, class="menuitem-iconic" and the .png
images

Review commit: https://reviewboard.mozilla.org/r/121284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121284/

Comment 3

[Bharat Raghunathan]

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?

Comment 4

[Dão Gottwald [::dao]]

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!

Comment 5

[Bharat Raghunathan]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121284/diff/1-2/

Comment 6

[Bharat Raghunathan]

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)

Comment 7

[Dão Gottwald [::dao]]

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 :(

Comment 8

[Bharat Raghunathan]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121284/diff/2-3/

Comment 9

[Bharat Raghunathan]

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)

Comment 10

[Dão Gottwald [::dao]]

ESLint only covers JS, not CSS. hg diff should definitely show you these changes.

Comment 11

[Dão Gottwald [::dao]]

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!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14b74d9448cc
Removed icon for "Undo Closed Tab" and its references r=dao

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/14b74d9448cc

Comment 14

[Bharat Raghunathan]

Thanks, ::dao <[e-mail]> !!

Comment 15

[Almas Hossain[:mAlmas]]

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]