Bookmarks menu button dropdown icon has the wrong aspect ratio and is mis-sized when placed in the Bookmarks Toolbar

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Theme
--
minor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Alice0775 White, Assigned: mak)

Tracking

(Blocks: 1 bug)

29 Branch
Firefox 29
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P3][good first verify])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Created attachment 8358258 [details]
screenshot

STR
Move Bookmarks widget Icon to Bookmarks Toolbar

Updated

4 years ago
Component: Toolbars and Customization → Theme

Updated

4 years ago
Blocks: 939862
Whiteboard: [Australis:P3]
The problem is that while we could adjust the image to be 'properly' 16x16px, it'll still not look great because the source image is 18px. :-|

Anyway, better than the current situation, I guess.
Summary: New Bookmarks widget Icon's has wrong aspect ratio in Bookmarks Toolbar → Bookmarks menu button dropdown icon has the wrong aspect ratio and is mis-sized when placed in the Bookmarks Toolbar
(In reply to :Gijs Kruitbosch from comment #1)
> The problem is that while we could adjust the image to be 'properly'
> 16x16px, it'll still not look great because the source image is 18px. :-|
> 
> Anyway, better than the current situation, I guess.

Huh, actually, it seems like all the other icons that I move there end up being 18x18 and it looks OK, and the bookmarks toolbar doesn't resize. Marco, should we just be using 18x18 for the star icon and the menu button dropdown icon in the bookmarks toolbar, too?
Flags: needinfo?(mak77)

Comment 3

4 years ago
It doesn't just happen in the bookmarks bar, also happens in overflow panel.
(Assignee)

Comment 4

4 years ago
This is a normal menu-button where the usual dropmarker has been replaced by a 18x18 icon, so the first possibility is that there is some rule adding padding/margin on the dropmarker that was removed in other cases. DOMi may help figuring that.
Though, I'd also take a look at the rules here, that look like may be causing this:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#1922
IIRC when the button is moved to the bookmarks toolbar it gets the bookmark-item attribute (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#1054), to be styled more similarly to the other objects on the toolbar. If keeping 18px doesn't "resize" vertically the toolbar when the button is added to it we may remove the rule, provided it doesn't look like this icon is much bigger than the other, otherwise it would look alien.
Flags: needinfo?(mak77)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(In reply to Marco Bonardo [:mak] from comment #4)
> This is a normal menu-button where the usual dropmarker has been replaced by
> a 18x18 icon, so the first possibility is that there is some rule adding
> padding/margin on the dropmarker that was removed in other cases. DOMi may
> help figuring that.
> Though, I'd also take a look at the rules here, that look like may be
> causing this:
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.
> css#1922
> IIRC when the button is moved to the bookmarks toolbar it gets the
> bookmark-item attribute
> (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> places.js#1054), to be styled more similarly to the other objects on the
> toolbar. If keeping 18px doesn't "resize" vertically the toolbar when the
> button is added to it we may remove the rule, provided it doesn't look like
> this icon is much bigger than the other, otherwise it would look alien.

So the toolbar doesn't visually resize (at least on OS X) in customize mode, but if you use the context menu to remove it, it does in fact resize.

The problem is that this already happens for the other buttons (e.g. the downloads button etc.) as well.

I don't think we should try to come up with a kind of magical way to make this work. The simplest way to solve this is to just accept that adding normal toolbar buttons adds 2px to the height of the bookmarks toolbar. Maintaining alternative icons (or even just alternative CSS to crop the sides off all the icons just when they're on the bookmarks toolbar) for every single other button is going to be a nightmare.

We could mitigate this by giving the bookmarks items margin so that there's no observable resize, but that'll add the 2px permanently.

Marco, what do you think?
Flags: needinfo?(mak77)
(Assignee)

Comment 6

4 years ago
did you see this rule?

#bookmarks-menu-button[cui-areatype="toolbar"] > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon {
  padding-top: 2px;
  padding-bottom: 2px;
}

looks like exactly what is causing the icon to be distorted.

adding a :not(.bookmark-item) solves the issue (when out of customize). During customize there's still one issue, the button doesn't change class until we exit customize. That's likely fixable touching _updateToolbarStyle in browser-places.js, I suspect it's early returning cause it can't find this.button, though I didn't verify that.

I don't think we should go much further in this bug, it's true that keeping 18px would simplify a lot of stuff (the bookmarks button has duplicated images for 16 and 18px) but I feel like that discussion is for a separate bug with UX involvement. I actually would also like to change the bookmarks toolbar feature completely, so that may be a more generic discussion with a different goal than a simple style fix.
Flags: needinfo?(mak77)
(Assignee)

Comment 7

4 years ago
(In reply to Marco Bonardo [:mak] from comment #6)
> During customize there's still one issue, the button doesn't change class
> until we exit customize. That's likely fixable touching _updateToolbarStyle
> in browser-places.js, I suspect it's early returning cause it can't find
> this.button, though I didn't verify that.

It's even simpler than that
BookmarksMenuButton.customizeChange(); should be BookmarkingUI.customizeChange();
Sounds like we didn't update this code
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3293
(Assignee)

Comment 8

4 years ago
at this point I will just submit a patch for you to review
Assignee: gijskruitbosch+bugs → mak77
(Assignee)

Comment 9

4 years ago
Created attachment 8363157 [details] [diff] [review]
patch v1

there was a little bit more missing, nothing fancy
Attachment #8363157 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8363157 [details] [diff] [review]
patch v1

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

This is why I hate patches that linger. I actually fixed a bunch of the home button stuff in bug 944947, but I can't land that until I get r+ on the other patch there. Sigh. Anyway. This looks good. I'll unbitrot my own patch whenever I get the other reviews in that bug.
Attachment #8363157 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 11

4 years ago
I added [Australis] to the checkin message
https://hg.mozilla.org/integration/fx-team/rev/7a85fd2440e8
Target Milestone: --- → Firefox 29
https://hg.mozilla.org/mozilla-central/rev/7a85fd2440e8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Whiteboard: [Australis:P3] → [Australis:P3][good first verify]
You need to log in before you can comment on or make changes to this bug.