986970 - [OSX] Bookmark widget label is not aligned correctly in palette

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Peter Retzer (:pretzer)]

Attachment: Screenshot of the issue

When the bookmark widget is in the palette its label is not vertically aligned with the other labels. See the screenshot. Reproduced on Windows 7 with the latest Nightly.

Comment 1

[Mike Hoye [:mhoye]]

Peter, can you mentor a contributor through this?

Comment 2

[Peter Retzer (:pretzer)]

Sorry for the late reply. I've never fixed a code bug myself yet (just a simple image swap one) so unfortunately I cannot mentor this bug (I'd rather be the mentee ;-)). Maybe Gijs can?

Comment 3

[:Gijs (he/him)]

I can mentor this, but it seems to be tricky. It's a challenging button to fix correctly. DOM Inspector ( https://addons.mozilla.org/en-US/firefox/addon/dom-inspector-6622/ ) will come in handy, but I'm not even 100% sure what's going on yet. At the very least, the padding/margin is different between menu buttons and "normal" buttons. Not 100% sure that that by itself will be enough to fix this issue, but it'll be a good start.

Comment 4

[:Gijs (he/him)]

(marking all/all because I can reproduce on OS X)

Comment 5

[davezatch]

I would like to take this as my first bug. I've found the offending piece of CSS, and disabling padding on the element does the trick, at least on OS X. But I'm not sure about a couple things:

1. Is it better to add a specific rule on this element specifying the padding, or am I OK with disabling the padding here? (more specifically, how would I go about figuring out what other DOM elements this CSS effects?)

2. I don't see the offending CSS rule in the browser.css files for Windows or Linux. I assume that means they don't exist there, but since I'm developing on OS X, I can't test that.

I've attached a patch file.

Comment 6

[davezatch]

Attachment: First draft attempt at normalizing padding

Comment 7

[:Gijs (he/him)]

Hey Dave, thanks for taking a look. Sorry for the late reply - I'm on vacation and away from my computer until Monday. I'll look at the review straight away when I get back.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8541941 [details] [diff] [review]
First draft attempt at normalizing padding

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

Did you try this patch?

Looking on Windows, the issue is definitely still there, despite the padding on the dropmarker not being reset anywhere, but also, the dropmarker is not visible when the button is in the palette (The 'Additional Tools and Features' area in customize mode). It's hidden using display: none, so it'd be surprising that the padding on the (hidden and therefore frameless) dropmarker would influence the sizing of the other bits of the button...

I also note that this patch would (if it worked) impact the 'ordinary' state of the button - you're changing the padding on the dropmarker icon when it is in the toolbar, whereas AFAIK the padding is currently correct there (on both Windows and OS X). Of course, this is talking about the bookmarks button, and the rule you're adjusting would equally apply to other buttons with dropmarkers...

Can you clarify what you're seeing before/after your patch?

Comment 9

[:Gijs (he/him)]

(In reply to Gijs Kruitbosch from comment #8)
> Comment on attachment 8541941 [details] [diff] [review]
> First draft attempt at normalizing padding
> 
> Review of attachment 8541941 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did you try this patch?
> 
> Looking on Windows, the issue is definitely still there, despite the padding
> on the dropmarker not being reset anywhere, but also, the dropmarker is not
> visible when the button is in the palette (The 'Additional Tools and
> Features' area in customize mode). It's hidden using display: none, so it'd
> be surprising that the padding on the (hidden and therefore frameless)
> dropmarker would influence the sizing of the other bits of the button...
> 
> I also note that this patch would (if it worked) impact the 'ordinary' state
> of the button - you're changing the padding on the dropmarker icon when it
> is in the toolbar, whereas AFAIK the padding is currently correct there (on
> both Windows and OS X). Of course, this is talking about the bookmarks
> button, and the rule you're adjusting would equally apply to other buttons
> with dropmarkers...
> 
> Can you clarify what you're seeing before/after your patch?

Comment 10

[davezatch]

Attachment: Screenshot after patch

This is what I'm seeing after my patch, with an on-screen ruler to show that the labels are now lined up.

Comment 11

[davezatch]

As I said before, I don't have a Windows box to work with, though could set up a VM I suppose. So on OS X, after my patch, the labels are lined up. That rule was affecting the 'xul:toolbarbutton', which the other items don't have. Sorry if I'm not explaining this well.

Sorry if my patch is way off target, I don't see any regressions here and on the windows.css file (mozilla-central/browser/themes/windows/browser.css) I couldn't find a similar CSS rule, so I'm not sure where to look. 

Would it be better to try and target the bookmarks toolbar item explicitly? Sorry, this is my first time digging through the FF source and I'm trying to wrap my head around it all :)

Comment 12

[:Gijs (he/him)]

(In reply to davezatch from comment #11)
> As I said before, I don't have a Windows box to work with, though could set
> up a VM I suppose. So on OS X, after my patch, the labels are lined up. That
> rule was affecting the 'xul:toolbarbutton', which the other items don't
> have. Sorry if I'm not explaining this well.
> 
> Sorry if my patch is way off target, I don't see any regressions here and on
> the windows.css file (mozilla-central/browser/themes/windows/browser.css) I
> couldn't find a similar CSS rule, so I'm not sure where to look. 
> 
> Would it be better to try and target the bookmarks toolbar item explicitly?
> Sorry, this is my first time digging through the FF source and I'm trying to
> wrap my head around it all :)

Three sorries in one comment - first off, no worries. All of these are totally legit questions. No need to be sorry about anything. :-)

So let's focus on OS X. Windows/Linux will probably need its own special treatment after we're done here (sad as that might be).

In terms of regressions, I just tested your patch on OS X, and you're quite right that this fixes the problem there. It does introduce a regression, however, that I picked up on here:

(In reply to Gijs Kruitbosch from comment #8)
> I also note that this patch would impact the 'ordinary' state
> of the button - you're changing the padding on the dropmarker icon when it
> is in the toolbar, whereas AFAIK the padding is currently correct there (on
> both Windows and OS X). Of course, this is talking about the bookmarks
> button, and the rule you're adjusting would equally apply to other buttons
> with dropmarkers...

So with the patch, and the button in the toolbar, I see...

Comment 13

[:Gijs (he/him)]

Attachment: screenshots.png

... this. Note how there is now wonky padding on the dropmarker, and the border between the two parts shifts on hover.

Comment 14

[:Gijs (he/him)]

Of course, the million dollar question is: what to do instead.

I *think* that the simplest thing required to make this work would be for the buttons where we hide the dropmarker in the palette to have the same padding as without the rule that resets it.

You can add a rule strictly for OS X and the customization palette / panel at the bottom of:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/customizableui/panelUIOverlay.css

The best would be to set the padding for the .toolbarbutton-menubutton-button to 3px 1px *when it is in the palette*, so using two child selectors like the first part of this selector:

https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#974

I *think* we should do this for all 'menu button' type buttons, so also things like adblockplus or other add-on provided buttons.


In fact, it's possible that doing this would also fix the issue on Windows or Linux - although we might need different values of padding... :-\

Can you try if you can get this solution to work on OS X, and if so, I can look if it works on Windows? In that case, the rule could live in browser/themes/shared/customizableui/panelUIOverlay.inc.css .

Comment 15

[davezatch]

Attachment: A more targeted padding fix

Ok, here's another attempt, following your advice. Not sure on the etiquette here, so I included the revert of my previous patch.

Comment 16

[:Gijs (he/him)]

Attachment: Fix for padding alignment issue for bookmarks label in customize->additional tools and features on OS X,

Great! There's no need to revert the other change. I've removed that in this patch, and updated the summary to describe exactly what is being fixed. Windows seems to be more complicated... I will file a follow-up bug for that. I'll r+ and land this patch in a second.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8545903 [details] [diff] [review]
Fix for padding alignment issue for bookmarks label in customize->additional tools and features on OS X,

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

Comment 18

[:Gijs (he/him)]

So, first patch done! Thanks for the patch, and for persisting beyond my saying it didn't work on Windows. :-)

Maybe you'd like to look at something else next? If you can create a Windows/Linux VM, bug 1119219 for the Windows/Linux parts of this bug would probably not be too hard (I fear that we will need per-OS rules about different bits of padding/border...), or otherwise perhaps something like bug 1000700?

If you're also interested in stuff requiring a bit of JS knowledge, there's also things like bug 1013234 or bug 900763 - a longer list is on http://www.joshmatthews.net/bugsahoy/?ff=1&js=1&unowned=1 .

Comment 19

[davezatch]

Cool. First FF bug :) Thanks Gijs!

Will take a look at those bugs after work and try to grab one.

Cheers!

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cd908a8fb224