971948 - Bookmark toolbar items icon is broken in the palette

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[u428464]

It just looks completely distorted.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Most likely broken by bug 897496.

Comment 2

[Matthew N. [:MattN]]

Attachment: Screen Shot

Comment 3

[u428464]

This is resolved at least on Windows.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

This is still looking distorted for me on fx-team 363beb46bd10 Win8.1.

Comment 5

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> This is still looking distorted for me on fx-team 363beb46bd10 Win8.1.

Same on a new profile on Win 7. Seems to be fine on OS X 10.9.

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> > This is still looking distorted for me on fx-team 363beb46bd10 Win8.1.
> 
> Same on a new profile on Win 7. Seems to be fine on OS X 10.9.

Huh, and of course straight after that I can't reproduce on Win 7 anymore. Something weird is up here. :-\

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

The stretching of the icon is no longer present, I think that got fixed recently by removing the overflow:hidden, etc.

This patch fixes the positioning of the label in the palette and menu panel.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8390801 [details] [diff] [review]
Patch

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

r=me on the min-height, but I'm confused about the label's margin...

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +133,5 @@
>  
>  .panelUI-grid .toolbarbutton-1 > .toolbarbutton-text,
>  .panelUI-grid .toolbarbutton-1 > .toolbarbutton-multiline-text {
>    text-align: center;
> +  /* Need to override toolkit theming which sets margin: 0 !important; */

On what OS do we need to override this? I'm having to modify this for the subviewbuttons already (although they might not be toolbarbutton-1, come to think of it), and I'd like to not exacerbate our use of !important too much. :-(

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

We need 'important' for Linux and Windows, not OSX:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/toolbarbutton.css#32
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/toolbarbutton.css#24
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/toolbarbutton.css#30

Would you rather that I write the following?
%ifdef XP_MACOSX
margin: 2px 0 0;
%else
margin: 2px 0 0 !important;
%endif

Comment 10

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> We need 'important' for Linux and Windows, not OSX:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/
> toolbarbutton.css#32
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> toolbarbutton.css#24
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/
> toolbarbutton.css#30
> 
> Would you rather that I write the following?
> %ifdef XP_MACOSX
> margin: 2px 0 0;
> %else
> margin: 2px 0 0 !important;
> %endif

Nah, it's ok.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/95341f53d78f

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95341f53d78f

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8390801 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 897496
User impact if declined: label for palette item is out of line with other items
Testing completed (on m-c, etc.): locally and m-c
Risk to taking this patch (and alternatives if risky): none expected
String or IDL/UUID changes made by this patch: none

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9f7b62796b0d