922834 - Australis: Need a more high-res version of the bookmarks toolbar item placeholder icon for the palette/panel

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[:Gijs (out for now; he/him)]

The current icon isn't very big, because it was designed for use in toolbars. We should have a more high-resolution one, and/or decide if this icon fits with the other icons in the panel. Stephen, this probably needs your help (or that of someone else who can do the icon-wrangling for us).

(Filed based on bug 878544 comment 4, but I noticed this myself when looking at bookmark toolbar items + panel stuff)

Comment 1

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Screen Shot 2013-12-10 at 12.15.21 PM.png

Looks hi-res to me. Is this asset still needed?

Comment 2

[:Gijs (out for now; he/him)]

(In reply to mmaslaney from comment #1)
> Created attachment 8345462 [details]
> Screen Shot 2013-12-10 at 12.15.21 PM.png
> 
> Looks hi-res to me. Is this asset still needed?

That's the bookmarks menu button, not the bookmarks toolbar item.

Comment 3

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: [e-mail]

Comment 4

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: bookmarkToolbar_menuPanel.zip

Comment 5

[u428464]

2 things to note :

This icon should also be updated in the "bookmarks sidebar" and the library.

The text is cropped a bit weirdly in customization mode. It should appear on two lines.

Comment 6

[:Gijs (out for now; he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #5)
> 2 things to note :
> 
> This icon should also be updated in the "bookmarks sidebar" and the library.

Why? The icon isn't changing outside the panel/palette. It'll be the same in toolbars as well. This bug is purely out of concern for the resolution of the icon when it's displayed at 36px^2. Certainly changing the icon everywhere doesn't need to happen as part of this patch. Furthermore, the flat icon will look out of place in the current bookmarks UI, so we'd have to refresh all of those icons, which is even more out of scope here.

Comment 7

[:Gijs (out for now; he/him)]

Attachment: Australis: Need a more high-res version of the bookmarks toolbar item placeholder icon for the palette/panel,

So this patch does fix the alignment of the icon by making palette wrappers remove and re-add flex for the things they wrap. I chose not to fix the text wrapping in here because we're still figuring out what the desired endstate is for cropped single-line vs. multiline labels in bug 944947.

Comment 8

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: bookmarkToolbar-menuPanel.zip

Updated with all platform themes.

Comment 9

[:Gijs (out for now; he/him)]

Comment on attachment 8360417 [details] [diff] [review]
Australis: Need a more high-res version of the bookmarks toolbar item placeholder icon for the palette/panel,

Cancelling review because I should integrate the other icons for Windows.

Comment 10

[:Gijs (out for now; he/him)]

Attachment: Australis: Need a more high-res version of the bookmarks toolbar item placeholder icon for the palette/panel,

So this adds the WinXP icon. I'm going to leave the win8 bit for Mike de Boer in bug 859751.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8360692 [details] [diff] [review]
Australis: Need a more high-res version of the bookmarks toolbar item placeholder icon for the palette/panel,

Review of attachment 8360692 [details] [diff] [review]:
-----------------------------------------------------------------

Other than the little bug I found, I think this looks OK to me. Certainly better than what we had before.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +685,5 @@
>        toolbarItem.checked = true;
>      }
>  
> +    if (aWrapper.hasAttribute("flex") && !toolbarItem.hasAttribute("flex")) {
> +      toolbarItem.setAttribute(aWrapper.getAttribute("flex"));

This needs to be:

toolbarItem.setAttribute("flex", aWrapper.getAttribute("flex"))

Comment 12

[:Gijs (out for now; he/him)]

With my stupidity addressed (at least as far as this patch is concerned),

remote:   https://hg.mozilla.org/integration/fx-team/rev/4a5c84a4d166

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a5c84a4d166

Comment 14

[:Gijs (out for now; he/him)]

Attachment: Backed out changeset 4a5c84a4d166 ()

So this fixes the styling of the search bar and the bookmarks bar. There's still a separate issue that only occurs if moving the bookmarks toolbar items from the palette to the menu panel, but stops as soon as they've been in the toolbar. Don't think that needs to hold up this bug, though.

Comment 15

[:Gijs (out for now; he/him)]

Comment on attachment 8366666 [details] [diff] [review]
Backed out changeset 4a5c84a4d166 ()

Sigh. Wrong bug! :-)