1197652 - Bookmarks Toolbar Items have no icon in Customization mode if they are placed in Navigation toolbar

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[arni2033]

Attachment: Bookmarks Toolbar Items have no icon in Customization mode if they are placed in Navigation toolbar.png

STR:   (Win7, Nightly 43.0a1 (2015-08-20))
1. Click (≡)->Customize
2. Drag Bookmarks Toolbar Items in Navigation toolbar (near urlbar)

Result:       Bookmarks Toolbar Items have no icon
Expectations: Bookmarks Toolbar Items should have icon

Comment 1

[Alice0775 White]

Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=f16cb820431b&tochange=27061dc242e4

Suspect:
+0000	69f04d5d6700	Gijs Kruitbosch — Bug 971956 - fix styling of items in widget overflow list, r=mconley

Comment 2

[:Gijs (he/him)]

This is happening on windows and linux, but not OSX.

The reason is that the width of the icon is set to 16px for all toolbars, and then the "normal" padding and margin for the navbar and/or tabs toolbar is applied (this doesn't happen on the bookmarks toolbar) which doesn't leave enough room for the icon itself.

There are issues with the margin and padding as well. I'm trying to figure out what the most straightforward fix is.

Comment 3

[:Gijs (he/him)]

Attachment: Patch v1

Tested on win8 and Linux, this seems to wfm and be a relatively simple and workable solution.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8652263 [details] [diff] [review]
Patch v1

I don't understand why we set the toolbarbutton-1 class on the placeholder. It's quite different from other toolbar buttons; it has a label and it's not supposed to be interactive. Looking at bug 971956, from what I can tell the real bug there is that the placeholder is displayed at all in the overflow menu.

Comment 5

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 8652263 [details] [diff] [review]
> Patch v1
> 
> I don't understand why we set the toolbarbutton-1 class on the placeholder.
> It's quite different from other toolbar buttons; it has a label and it's not
> supposed to be interactive. Looking at bug 971956, from what I can tell the
> real bug there is that the placeholder is displayed at all in the overflow
> menu.

Removing the class will break third-party themes and require a much larger patch + verification, ensuring it continues to look reasonable in the palette, on the panel, and on all 4 default toolbars, on all platforms. I don't think the severity of this bug warrants making such a large change. What's actually wrong with the smaller fix which addresses the issue at hand?

Comment 6

[Dão Gottwald [:dao]]

The problem with the patch is that it builds on something broken, making the whole thing even weirder and harder to maintain if (God forbid) we need to touch it again.

The placeholder wasn't originally intended to be displayed in the panel. Its label doesn't even make sense there. Maybe we should have another button that would be displayed specifically in that case? This should be straightforward and shouldn't need extraordinary verification efforts.

I see no difference at all in the palette when removing the class. Am I missing something?

Comment 7

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #6)
> The problem with the patch is that it builds on something broken, making the
> whole thing even weirder and harder to maintain if (God forbid) we need to
> touch it again.

I don't think removing the specialcasing of this button in 2 places, adding a single rule next to an existing one specific to this button, and adding 1 property to that existing rule, really makes a material maintenance difference.

> The placeholder wasn't originally intended to be displayed in the panel. Its
> label doesn't even make sense there.

What's wrong with the label being "Bookmark toolbar items" in the panel? I don't follow.

> Maybe we should have another button
> that would be displayed specifically in that case? This should be
> straightforward and shouldn't need extraordinary verification efforts.

In my mind, that is a lot less straightforward than the patch currently on this bug.

> I see no difference at all in the palette when removing the class. Am I
> missing something?

My point was about the increase in scope, which is significant, rather than about the specifics of that increase in scope. Maybe we don't need to touch the palette styling, but we do need to add a separate button to make it work in the panel(s) ? That's a lot more work. I don't think the severity of this bug justifies that work.

Comment 8

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #7)
> > The placeholder wasn't originally intended to be displayed in the panel. Its
> > label doesn't even make sense there.
> 
> What's wrong with the label being "Bookmark toolbar items" in the panel? I
> don't follow.

IMHO it should be either "Bookmarks Toolbar" and open a subview or just "Bookmarks" and open all bookmarks in the library. I think the former is preferable as it stays true to the real bookmarks toolbar's spirit (allowing you to pick from a more easily accessible subset of your bookmarks), but the latter is probably much simpler. The current implementation feels half-assed and rushed, which in fact it probably is given the pressure to ship Australis...

> That's a lot more work. I don't think the severity of this
> bug justifies that work.

It probably doesn't. It would be a maintenance effort that hopefully will pay off beyond just fixing this particular bug. I'll give it a shot.

Comment 9

[Justin [:JW_SoftvisionQA]]

I can not reproduce this issue on  	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

I will close this issue as wfm. Please reopen if you can still reproduce.

Comment 10

[:Gijs (he/him)]

(In reply to Justin [:JW_SoftvisionQA] from comment #9)
> I can not reproduce this issue on  	Mozilla/5.0 (Windows NT 6.1; WOW64;
> rv:48.0) Gecko/20100101 Firefox/48.0
> 
> I will close this issue as wfm. Please reopen if you can still reproduce.

I can still reproduce on nightly and release.

Comment 11

[Justin [:JW_SoftvisionQA]]

Whoops, I was looking at the wrong icon. This is also reproducible on Win 8.1 and Win 10.

Comment 12

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 13

[Dão Gottwald [:dao]]

I believe bug 1391017 fixed this.