632207 - Too little padding on left & right side of bookmarks toolbar

Status: RESOLVED WORKSFORME

(Firefox :: Theme, defect, P3)

Description

[Theodore]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110207 Firefox/4.0b12pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110207 Firefox/4.0b12pre

There is 2px padding below (and I assume above) the bookmarks in the bookmarks toolbar. However, there is no padding on the right and left sides, which looks somewhat ugly.

Reproducible: Always

Steps to Reproduce:
1. Make sure "View Bookmarks Toolbar" is toggled on.
2. Hover or click on the leftmost bookmark, or the bookmarks button on the right side of the toolbar.
Actual Results:  
There is no padding on the left and right sides of the toolbar

Expected Results:  
There should be 2px padding, just like there is below the bookmarks.

See screenshot

Comment 1

[Theodore]

Attachment: Screenshot of problem

Comment 2

[Alex Lakatos[:AlexLakatos]]

I belive this is a good catch, even if it's trivial.

Comment 3

[Nikhil Johny]

hi, i have been trying to solve this bug and it seem to me that by coding -moz-padding-start: 2px; in the selector #personal-bookmarks does the job. is there something that i am missing?

Comment 4

[:Margaret Leibovic]

(In reply to Nikhil Johny from comment #3)
> hi, i have been trying to solve this bug and it seem to me that by coding
> -moz-padding-start: 2px; in the selector #personal-bookmarks does the job.
> is there something that i am missing?

If this issue still exists, a patch is definitely welcome! I don't see this issue on OSX, but I don't have a Windows machine to test.

Jared would probably be a good person to test this out and review a patch if you write one.

Comment 5

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

Thanks Margaret. Yeah this issue still exists on Windows.

Setting -moz-padding-start:2px on #personal-bookmarks is a very simple and effective solution. The downside to this approach is that it doesn't allow clicks on the edge of the window (when maximized) to activate the button.

A lot of work went in to bug 571454 to get the left-most pixel clickable for the back button. I don't think the level of effort that went in to bug 571454 is necessary here, as the bookmarks button is not as major of a UI element as the back button.

In this situation, I think the simplicity of the code is better than a highly complex solution.

Can you please upload your patch for review? Thanks!

Comment 6

[Nikhil Johny]

Attachment: Simple solution

Comment 7

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

Comment on attachment 729079 [details] [diff] [review]
Simple solution

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

This looks good. Can you create a new file in /browser/themes/shared/ named browserShared.inc.css and place the rule in there? After creating the file, you'll need to include it within the browser.css files, preferably right below all the %define's at the top of the file.

You can look at http://mxr.mozilla.org/mozilla-central/search?string=chat.inc.css to see how the chat.inc.css file is included.

Comment 8

[Marco Bonardo [:mak]]

The bug is also about the end side, according to the title but the patch seems only to handle the start side?
Did you test that with this padding the drag&drop indicator is still properly positioned (between items borders)? I wonder if a margin would be safer for that.
And the spacing should be the same that toolbarbutton-1 items get on the #nav-bar, so that the first and last items are aligned on the 2 toolbars, guessing 2px may not be good for any platform. 

There is also another fact, if you try to drag the home button (for example) to the #PersonalToolbar, you'll see the issue is not solved for it since the #nav-bar adds padding to toolbarbutton-1, but not the #PersonalToolbar.  You may consider adding #PersonalToolbar to the moz-any here (http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#801 and probably something similar on other platforms) I suppose.

Comment 9

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #7)
> This looks good. Can you create a new file in /browser/themes/shared/ named
> browserShared.inc.css and place the rule in there?

As we discussed on IRC, I think we should avoid creating catch-all files in themes/shared/ that end up being a 3000 line dumping ground like browser.css currently is.

Moving all of the common bookmark toolbar styles to its own file can be handled in a separate bug as I don't think it's worth creating a new file just for one ruleset. Having one ruleset separated from the rest of the styles also worse for maintenance IMO.

Comment 10

[Nikhil Johny]

Attachment: second patch

i dont think the drag/drop indicator seems to be a problem after adding the padding, i think it is properly positioned. 

please let me know if i missed something.

Thank you

Comment 11

[Nikhil Johny]

Attachment: second patch

i dont think the drag/drop indicator seems to be a problem after adding the padding, i think it is properly positioned. 

please let me know if i missed something.

Thank you

Comment 12

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

Comment on attachment 729622 [details] [diff] [review]
second patch

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

The new :-moz-any(#PersonalBookmarks) that this patch added should be removed. Please follow the recommendations of comment #8.

You can ask for help on #introduction or #fx-team on irc.mozilla.org if you have any questions.

Comment 13

[Nikhil Johny]

Attachment: patch

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 730792 [details] [diff] [review]
patch

Sorry for the delay.  The problem isn't bookmark toolbar specific so the fix should be more general IMO.
It's also debatable whether we would want this on the right side or not since the new menu button and the new tab button are both regularly used and would benefit from easy access at the edge of the screen.
I also think we should wait for the new Australis toolbar button styling (e.g. bug 856665) before fixing this since they change how the implementation of this fix.
I think we can revisit this after Australis. Thanks.

Comment 15

[u428464]

(In reply to Matthew N. [:MattN] from comment #14)
> I also think we should wait for the new Australis toolbar button styling
> (e.g. bug 856665) before fixing this since they change how the
> implementation of this fix.
> I think we can revisit this after Australis. Thanks.

And bug 734326.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 18

[Gregory Pappas [:gregp]]

Unable to reproduce