Too little padding on left & right side of bookmarks toolbar

ASSIGNED
Assigned to

Status

()

Firefox
Theme
P2
minor
ASSIGNED
7 years ago
7 months ago

People

(Reporter: Theodore, Assigned: Nikhil Johny)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
Created attachment 510411 [details]
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.

Updated

6 years ago
Depends on: 673695
(Reporter)

Updated

6 years ago
Blocks: 544820
Severity: trivial → minor
Priority: -- → P2
(Assignee)

Comment 3

5 years ago
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

5 years ago
(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
(Assignee)

Comment 6

5 years ago
Created attachment 729079 [details] [diff] [review]
Simple solution
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.
(Assignee)

Comment 10

5 years ago
Created attachment 729619 [details] [diff] [review]
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
Attachment #729079 - Attachment is obsolete: true
Attachment #729619 - Flags: review?
(Assignee)

Comment 11

5 years ago
Created attachment 729622 [details] [diff] [review]
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
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-
(Assignee)

Comment 13

5 years ago
Created attachment 730792 [details] [diff] [review]
patch
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

Updated

4 years ago
Depends on: 767319
You need to log in before you can comment on or make changes to this bug.