Closed Bug 986970 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

All
macOS
defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 37
Iteration:
37.3 - 12 Jan

People

(Reporter: pretzer, Assigned: davezatch, Mentored)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [Australis:P4-] [diamond] [qx:link:spec])

Attachments

(4 files, 2 obsolete files)

Attached image 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.
Whiteboard: [Australis:P?]
Whiteboard: [Australis:P?] → [Australis:P4-]
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Whiteboard: [Australis:P4-] → [Australis:P4-] p=2
Points: --- → 2
Whiteboard: [Australis:P4-] p=2 → [Australis:P4-]
Peter, can you mentor a contributor through this?
Flags: needinfo?(peter.retzer)
Whiteboard: [Australis:P4-] → [Australis:P4-] [diamond]
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?
Flags: needinfo?(peter.retzer) → needinfo?(gijskruitbosch+bugs)
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.
Mentor: gijskruitbosch+bugs
Flags: needinfo?(gijskruitbosch+bugs)
OS: Windows 7 → All
Summary: [Australis] Bookmark widget label is not aligned correctly in palette → Bookmark widget label is not aligned correctly in palette
(marking all/all because I can reproduce on OS X)
Whiteboard: [Australis:P4-] [diamond] → [Australis:P4-] [diamond] [qx]
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.
Attachment #8541941 - Flags: review?(gijskruitbosch+bugs)
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 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?
Attachment #8541941 - Flags: review?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(davezatch)
Attached image 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.
Flags: needinfo?(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 :)
(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...
Attached image screenshots.png
... this. Note how there is now wonky padding on the dropmarker, and the border between the two parts shifts on hover.
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 .
Attached patch A more targeted padding fix (obsolete) — Splinter Review
Ok, here's another attempt, following your advice. Not sure on the etiquette here, so I included the revert of my previous patch.
Attachment #8545495 - Flags: review?(gijskruitbosch+bugs)
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 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
Attachment #8545903 - Flags: review+
Assignee: nobody → davezatch
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Flags: qe-verify-
Flags: in-testsuite-
OS: All → Mac OS X
Summary: Bookmark widget label is not aligned correctly in palette → [OSX] Bookmark widget label is not aligned correctly in palette
Attachment #8545495 - Attachment is obsolete: true
Attachment #8545495 - Flags: review?(gijskruitbosch+bugs)
Attachment #8541941 - Attachment is obsolete: true
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 .
Cool. First FF bug :) Thanks Gijs!

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

Cheers!
https://hg.mozilla.org/mozilla-central/rev/cd908a8fb224
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Iteration: 37.2 → 37.3 - 12 Jan
Whiteboard: [Australis:P4-] [diamond] [qx] → [Australis:P4-] [diamond] [qx:link:spec]
You need to log in before you can comment on or make changes to this bug.