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

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Theme
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

unspecified
Firefox 29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P3])

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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)
Flags: needinfo?(shorlander)
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M?][Australis:P3]
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?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 2

4 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Created attachment 8346074 [details]
bookmarksToolbar@2x.png
Attachment #8345462 - Attachment is obsolete: true
Flags: needinfo?(shorlander)
Created attachment 8346173 [details]
bookmarkToolbar_menuPanel.zip
Attachment #8346074 - Attachment is obsolete: true
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
Created attachment 8360417 [details] [diff] [review]
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.
Attachment #8360417 - Flags: review?(mconley)
Flags: needinfo?(mmaslaney)
Created attachment 8360481 [details]
bookmarkToolbar-menuPanel.zip

Updated with all platform themes.
Flags: needinfo?(mmaslaney)
(Assignee)

Comment 9

4 years ago
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.
Attachment #8360417 - Flags: review?(mconley)
(Assignee)

Comment 10

4 years ago
Created attachment 8360692 [details] [diff] [review]
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.
Attachment #8360692 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Attachment #8360417 - Attachment is obsolete: true
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"))
Attachment #8360692 - Flags: review?(mconley) → review+
(Assignee)

Comment 12

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

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

Updated

4 years ago
Whiteboard: [Australis:M?][Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4a5c84a4d166
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 29
(Assignee)

Updated

4 years ago
Depends on: 964217
(Assignee)

Comment 14

4 years ago
Created attachment 8366666 [details] [diff] [review]
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.
Attachment #8366666 - Flags: review?(mdeboer)
(Assignee)

Comment 15

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

Sigh. Wrong bug! :-)
Attachment #8366666 - Flags: review?(mdeboer)
You need to log in before you can comment on or make changes to this bug.