Open Bug 632207 Opened 12 years ago Updated 5 months ago

Too little padding on left & right side of bookmarks toolbar

Categories

(Firefox :: Theme, defect, P3)

defect

Tracking

()

People

(Reporter: theodorejb, Unassigned)

References

Details

Attachments

(2 files, 3 obsolete files)

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
Attached image Screenshot of problem
Component: General → Theme
QA Contact: general → theme
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
I belive this is a good catch, even if it's trivial.
Depends on: 673695
Blocks: 544820
Severity: trivial → minor
Priority: -- → P2
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?
(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.
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!
Assignee: nobody → nikhiljohny
Status: NEW → ASSIGNED
Attached patch Simple solution (obsolete) — Splinter Review
Attachment #729079 - Flags: review?(jAwS)
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.
Attachment #729079 - Flags: review?(jAwS) → feedback+
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.
(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.
Attached patch second patch (obsolete) — Splinter Review
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
Attachment #729079 - Attachment is obsolete: true
Attachment #729619 - Flags: review?
Attached patch second patch (obsolete) — Splinter Review
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
Attachment #729619 - Attachment is obsolete: true
Attachment #729619 - Flags: review?
Attachment #729622 - Flags: review?(jAwS)
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.
Attachment #729622 - Flags: review?(jAwS) → review-
Attached patch patchSplinter Review
Attachment #729622 - Attachment is obsolete: true
Attachment #730792 - Flags: review?(mnoorenberghe+bmo)
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.
Attachment #730792 - Flags: review?(mnoorenberghe+bmo) → feedback-
(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.
Duplicate of this bug: 896754
Depends on: australis-buttons
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
Priority: P2 → P3
Assignee: nikhiljohny → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.