1000739 - Bookmark toolbar items icon is too close to its label in customization mode

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

Attachment: Glass beta-3437e5663d9e_126_tabsOutsideTitlebar_twoPinnedWithOverflow_maximized_allToolbars_noLWT_customizing.png

Happens at least on Windows 7 and Linux. This worked fine in 28 on Win7.

Comment 1

[Matthew N. [:MattN]]

Attachment: Ubuntu 38_fourPinned_normal_allToolbars_noLWT_customizing.png

Comment 2

[:Gijs (he/him)]

The icon is supposed to look open on the right hand side, so that's not a bug.

To fix the margin, we should add styling for:

#personal-bookmarks[cui-areatype="toolbar"]:not([overflowedItem=true]) > #bookmarks-toolbar-placeholder > .toolbarbutton-icon

setting -moz-margin-end to 5px instead of the default 2px (linux) and 0px (Windows).

Here would probably be a good place:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/customizeMode.inc.css#273

You could then remove the last selector from:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#357

because it does the same thing but just for OS X.

Comment 3

[rn10950]

I'm interested in taking this bug.

Comment 4

[:Gijs (he/him)]

(In reply to rn10950 from comment #3)
> I'm interested in taking this bug.

Great! The steps in comment #2 should be able to let you fix this. You can get more general info on how to build Firefox at https://developer.mozilla.org/en-US/docs/Introduction and  https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

For questions, unfortunately I am a little less available the next 3 days (ie until Monday) than usual, although I'll still try to reply quickly here. If you need an answer faster you might try joining #introduction on IRC ( https://chat.mibbit.com/?url=irc%3A%2F%2Firc.mozilla.org%2F%23introduction ).

Please let me know if you have any further questions - we don't normally change the assignee field for the first bug you're fixing immediately, but don't let that stop you. :-)

Comment 5

[rn10950]

Attachment: bug1000739_marginfix.diff

Here is the patch for browser/themes/shared/customizableui/customizeMode.inc.css.

Comment 6

[rn10950]

Attachment: bug1000739_OSXfix.diff

Here is the OSX browser.css patch

Comment 7

[:Gijs (he/him)]

Awesome work, these patches look great!

Just a few more things to make them ready for check-in. Could you please:

- combine the two patches into one
- update the commit summary/header to be of the form:

"Bug 1000739 - fix the bookmarks toolbar placeholder icon's margin in toolbars, r=gijs"

- fix the indentation of the -moz-margin-end property to use spaces instead of tabs (please use 2 so it lines up with the other CSS in that file)

Then attach the result and request review from me? You can do this by setting the "review" dropdown box to "?" when you add the patch, and filling in my email address in the textbox next to it that then appears - you can use any uniquely matching description of my user id, so ":gijs" will be enough for bugzilla to find me. This is the case for most reviewers on bugzilla, ie ":theirnameornickname" usually finds them.

Thanks!

Comment 8

[rn10950]

(In reply to :Gijs Kruitbosch from comment #7)
> Awesome work, these patches look great!
> 
> Just a few more things to make them ready for check-in. Could you please:
> 
> - combine the two patches into one
> - update the commit summary/header to be of the form:
> 
> "Bug 1000739 - fix the bookmarks toolbar placeholder icon's margin in
> toolbars, r=gijs"
> 
> - fix the indentation of the -moz-margin-end property to use spaces instead
> of tabs (please use 2 so it lines up with the other CSS in that file)
> 
> Then attach the result and request review from me? You can do this by
> setting the "review" dropdown box to "?" when you add the patch, and filling
> in my email address in the textbox next to it that then appears - you can
> use any uniquely matching description of my user id, so ":gijs" will be
> enough for bugzilla to find me. This is the case for most reviewers on
> bugzilla, ie ":theirnameornickname" usually finds them.
> 
> Thanks!

How do I combine these patch files?

Comment 9

[:Gijs (he/him)]

(In reply to rn10950 from comment #8)
> How do I combine these patch files?

Depends how you generated them... Assuming that you used mercurial queues, you can apply the two patches one after the other (with hg qpush), then hg qpop the second one, then use "hg qfold -e name-of-second-patch" which will combine the two patches.

Does that help?

Comment 10

[rn10950]

Attachment: This is the updated combined patch with the spacing issues fixed

Comment 11

[:Gijs (he/him)]

Thanks! I've gone and pushed this to the fx-team integration branch for you; assuming nothing breaks, it'll be merged to mozilla-central sometime this weekend or on Monday, after which it'll make it into a nightly near you (maybe on Sunday, maybe on Monday, maybe on Tuesday - depends on merge times), which you can download at https://nightly.mozilla.org/ .

remote:   https://hg.mozilla.org/integration/fx-team/rev/bc93dbdb7a80


Congrats on your first patch! Do you want to look at fixing something else next? Maybe bug 983828, or bug 996562? :-)

Comment 12

[rn10950]

Ill try both of them. I have some time over the next few weeks. Regarding 1000739, no extra steps are necessary on my part to get the patch integrated in the tree?

Comment 13

[:Gijs (he/him)]

(In reply to rn10950 from comment #12)
> Ill try both of them. I have some time over the next few weeks. Regarding
> 1000739, no extra steps are necessary on my part to get the patch integrated
> in the tree?

Nope! In fact, it was merged to m-c already, but someone forgot to run the tool that then closes all the bugs...

Comment 14

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/bc93dbdb7a80