775191 - bookmarks toolbar items oversized

Status: RESOLVED DUPLICATE of bug 734326

(Firefox :: Theme, defect)

Description

[bugz]

Attachment: oversized bookmark items

User Agent: Mozilla/5.0 (Windows NT 6.0; WOW64; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713134347

Steps to reproduce:

moved the "bookmarks toolbar items" from the "bookmarks toolbar" up into the main toolbar where the location bar is.


Actual results:

all the items have no top or bottom margins, so they become oversized, please see screenshot for illustration


Expected results:

they should not stretch to the edge of the toolbar

Comment 1

[Alice0775 White]

Confirmed on
http://hg.mozilla.org/releases/mozilla-release/rev/e5728a4e106c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20100101 Firefox/14.0.1 ID:20120713134347

Comment 2

[Seetu]

I would like to work on this bug, Can you please assign this?

Comment 3

[Matthew N. [:MattN]]

(In reply to Seetu from comment #2)
> I would like to work on this bug, Can you please assign this?

Sure thing. 

First check if you can still reproduce this with the Australis redesign on Nightly.

Comment 4

[Seetu]

I could reproduce it, Can you give me some more details about this bug, In which file will be the bug present?. Can you please help me with this.

Comment 5

[Sunny [:darkowlzz]]

Hi Seetu,
After our irc discussion and some searching, I found that you need to make changes to browser.css [1][2][3] of the respective platforms (which is I guess, all the platforms).

I am on OS X, I added some values to the margin-top and the buttons appeared to become a bit small. I am adding 2 attachments, which would perhaps help you see the difference.

[1]: http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#169 (for OS X)
[2]: http://mxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#78 (for Linux)
[3]: http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#284 (for Windows)

I have tested [1]. [2] & [3] should be right for respective platforms too.

Comment 6

[Sunny [:darkowlzz]]

Attachment: Before adding margin.

Comment 7

[Sunny [:darkowlzz]]

Attachment: After adding margin.

Comment 8

[Seetu]

Attachment: patch.patch

Comment 9

[Sunny [:darkowlzz]]

(In reply to Seetu from comment #8)
> Created attachment 8360437 [details] [diff] [review]
> patch.patch

The patch doesn't contains your changes.
Read this [1] to learn how to create a proper patch.

Also, I haven't given a r+ (review+) yet. Once the patch is ready for checkin, you could add `r` to it, not now.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 10

[Seetu]

Attachment: bugpatch.patch

Comment 11

[Sunny [:darkowlzz]]

Comment on attachment 8361114 [details] [diff] [review]
bugpatch.patch

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

Good start.

This patch tries to fix the oversized bookmarks toolbar items in Linux.
Could you try for Windows and OS X too?

Also, I haven't given r+ yet. You can have r=me once you get a r+.

::: browser/themes/linux/browser.css
@@ +77,5 @@
>  #personal-bookmarks[cui-areatype="toolbar"] > #bookmarks-toolbar-placeholder {
> +  margin-left: 0;
> +  margin-right:10px;
> +  margin-top:10px;
> +  margin-bottom:10px;

We only need to set margin for top and bottom, keeping left and right 0 as before.
I would like you to use Two-value syntax of margin, like
margin: 4px 0px;

After fiddling a bit on my machine and comparing various margin sizes, I found that 4px for top and bottom is good enough.

Comment 12

[Seetu]

Attachment: LinuxFix.patch

Comment 13

[Sunny [:darkowlzz]]

Comment on attachment 8361744 [details] [diff] [review]
LinuxFix.patch

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

Thanks! That does the job for linux.

I am giving you r+ on this patch. Now you can submit this patch again with r=me :)
No need to ask for review on resubmission of this patch again.

I would suggest you to submit separate patches for Windows and OS X.

Good Luck!

Comment 14

[Seetu]

Attachment: Linux.patch

Comment 15

[Seetu]

Attachment: windows.patch

Comment 16

[Seetu]

Attachment: Osx.patch

Comment 17

[Sunny [:darkowlzz]]

Comment on attachment 8362232 [details] [diff] [review]
Osx.patch

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

Looks fine on OS X.

Comment 18

[Seetu]

Attachment: Osx.patch

Comment 19

[Seetu]

Attachment: windows.patch

Comment 20

[Sunny [:darkowlzz]]

Comment on attachment 8362522 [details] [diff] [review]
windows.patch

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

This looks fine :)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Doesn't have review from a theme peer. Also, can you please fold these patches together when this is ready to land?

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8362236 [details] [diff] [review]
Osx.patch

I'll review the patches. 

Sunny, thanks for your help but in the future, please don't mark r+ if you aren't a peer for the module in question. Feel free to use feedback+ though.

Also, we should indicate in the commit message that this conflicts with Australis so it gets backed out of Holly to avoid merge issues.

Comment 23

[Matthew N. [:MattN]]

Sunny, it was helpful for you to mentor the bug and there was nothing wrong with that. Patches still have to follow the Firefox Code Review[1] rules though.

[1] https://wiki.mozilla.org/Firefox/Code_Review

Comment 24

[Sunny [:darkowlzz]]

Thanks Matthew.

Seetu, fold the 3 patches into a single patch, removing `r=me`. Once done, ask MattN for review.

I won't be available for a week. Hope to see your patch land soon. :)

Comment 25

[Seetu]

Attachment: Bugfix.patch

Comment 26

[Matthew N. [:MattN]]

Comment on attachment 8363621 [details] [diff] [review]
Bugfix.patch

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

All of the changes you made also affect when the bookmarks items are in their default toolbar but the bug seems to be about when they're moved outside of it e.g. to the nav-bar.

I don't think we should bother fixing this before implementing bug 734326 since it will change how this works anyways and I believe will make this case look much better. Thanks for your time on this. Hopefully you can find another bug of interest to you. (bug 734326 is not so small).

::: browser/themes/windows/browser.css
@@ +274,5 @@
>  /* ::::: bookmark buttons ::::: */
>  
>  toolbarbutton.bookmark-item,
>  #personal-bookmarks[cui-areatype="toolbar"] > #bookmarks-toolbar-placeholder {
> +  margin: 7px 0px;

This increases the height of the Bookmark toolbar and adds too much margin when the items are there (in their default position)