Remove legacy transparent borders from toolbarbuttons on OS X

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dao, Assigned: batmanav10, Mentored)

Tracking

Trunk
mozilla44
Unspecified
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox43 affected, firefox44 fixed)

Details

(Whiteboard: [good first bug][lang=css])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

3 years ago
We set border-right and border-left to "1px solid transparent" here:
http://hg.mozilla.org/mozilla-central/annotate/5fe9ed3edd68/toolkit/themes/osx/global/toolbarbutton.css#l7

We never make these borders visible, so we should just remove them and increase the padding instead.

Updated

3 years ago
Flags: needinfo?

Updated

3 years ago
Flags: needinfo?

Comment 1

3 years ago
Can it be assigned to me?
(Reporter)

Comment 2

3 years ago
sure
Assignee: nobody → leo_2b2006

Comment 3

3 years ago
Because I am new. May you tell me what I should do?
(Reporter)

Comment 4

3 years ago
The basic instructions are in comment 0. Is there some part you don't understand or is there some point where you got stuck? If so I can go into detail there.

Comment 5

3 years ago
How do I run the code to test it after I finish it?
(Reporter)

Comment 6

3 years ago
Are you on OS X?

Comment 7

3 years ago
I have windows and os x.
(Reporter)

Comment 8

3 years ago
Okay, build instructions are here: https://developer.mozilla.org/en-US/docs/Simple_Firefox_build#Building

I think it's sufficient if you just open Firefox and check that toolbar buttons in the navigation toolbar, the bookmarks toolbar and the find bar look fine.

Comment 9

3 years ago
If I want to solve it in windows, is it possible like using virtual os?
(Reporter)

Comment 10

3 years ago
This bug is only present on OS X, you won't be able to test the fix on Windows.
(Assignee)

Comment 11

3 years ago
Created attachment 8666910 [details] [diff] [review]
toolbarbutton.css

Hi, I think I solved this bug. 
Could you please have a look?
Attachment #8666910 - Flags: review?(dao)
(Assignee)

Comment 12

3 years ago
Also, could you assign it to me, if it isn't ready to be merged? Or even if it is.
(Reporter)

Comment 13

3 years ago
Comment on attachment 8666910 [details] [diff] [review]
toolbarbutton.css

Hi. Could you please generate a patch using hg diff and attach that for review? Thanks!
Attachment #8666910 - Flags: review?(dao)
(Assignee)

Comment 14

3 years ago
Created attachment 8667267 [details] [diff] [review]
Removed transparent borders and added padding from toolbarbuttons on OSX

Hey, I figured I attached the wrong file earlier. Please find the diff attached for review. Thanks!
Attachment #8667267 - Flags: review?(dao)
(Assignee)

Updated

3 years ago
Attachment #8666910 - Attachment is obsolete: true
(Assignee)

Comment 15

3 years ago
Created attachment 8667272 [details] [diff] [review]
Removed transparent borders and added padding from toolbarbuttons on OSX

Hopefully, this is the correct one. I think I was unable to attach a patch via bzexport because the bug wasn't assigned to me. Anyway, please find the diff for review.
Attachment #8667272 - Flags: review?(dao)
(Reporter)

Updated

3 years ago
Attachment #8667267 - Attachment is obsolete: true
Attachment #8667267 - Flags: review?(dao)
(Reporter)

Comment 16

3 years ago
Comment on attachment 8667272 [details] [diff] [review]
Removed transparent borders and added padding from toolbarbuttons on OSX

> toolbarbutton {
>   -moz-box-align: center;
>   -moz-box-pack: center;
>   margin: 0px 2px 0px 2px;
>-  padding: 3px 1px 3px 1px;
>-  border-right: 1px solid transparent;
>-  border-left: 1px solid transparent;
>+  padding: 3px 11px 3px 11px;

To compensate the 1px border, you need to increase the left and right padding by one pixel (rather than by 10 pixels as you did in this patch).

Looks good otherwise!
Attachment #8667272 - Flags: review?(dao) → review-
(Reporter)

Updated

3 years ago
Assignee: leo_2b2006 → batmanav10
(Assignee)

Comment 17

3 years ago
Created attachment 8667303 [details] [diff] [review]
Removed transparent borders and added padding from toolbarbuttons on OSX

I'm really really sorry! I had set the border as 10 for testing on my machine, and forgot to change it back to 1 before changing the padding, hence mixing up :/

Please find the new patch! I really really hope this one is fine! Thanks a ton!
Attachment #8667303 - Flags: review?(dao)
(Reporter)

Comment 18

3 years ago
Comment on attachment 8667303 [details] [diff] [review]
Removed transparent borders and added padding from toolbarbuttons on OSX

Perfect! Oh, while you're already touching that line, you can set padding to "3px 2px" instead of "3px 2px 3px 2px", and the margin to "0 2px" instead of "0px 2px 0px 2px". Don't need to re-request review for that change. Thanks!
Attachment #8667303 - Flags: review?(dao) → review+
(Assignee)

Comment 19

3 years ago
Created attachment 8667545 [details] [diff] [review]
emoved transparent borders and added padding to tolbarbuttons on OS X

Final Patch for the bug with all the recommended fixes.
Attachment #8667272 - Attachment is obsolete: true
Attachment #8667303 - Attachment is obsolete: true
Attachment #8667545 - Flags: review+
(Reporter)

Comment 20

3 years ago
(In reply to Manav Batra from comment #19)
> Created attachment 8667545 [details] [diff] [review]
> emoved transparent borders and added padding to tolbarbuttons on OS X
> 
> Final Patch for the bug with all the recommended fixes.

This seems to be a diff against a local revision based on your previous patch (attachment 8667303 [details] [diff] [review]). We'll need a diff against remote tip, i.e. both patches merged into one. Could you do that?
(Assignee)

Comment 21

3 years ago
Created attachment 8669008 [details] [diff] [review]
Removed transparent borders from toolbarbuttons on OSX theme

Hi!

Sorry for the delay! Was busy with schoolwork.

Here, find the patch attached with all the recommended patches.
Thanks.
Attachment #8667545 - Attachment is obsolete: true
Attachment #8669008 - Flags: review?(dao)
(Reporter)

Comment 22

3 years ago
Comment on attachment 8669008 [details] [diff] [review]
Removed transparent borders from toolbarbuttons on OSX theme

Perfect, thanks.
Attachment #8669008 - Flags: review?(dao) → review+
(Assignee)

Comment 24

3 years ago
Can you help me proceed further?
Is there any bug you'd like to point me to?
https://hg.mozilla.org/mozilla-central/rev/f7103240cb30
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Reporter)

Updated

3 years ago
Blocks: 1212280
(Reporter)

Comment 26

3 years ago
(In reply to Manav Batra from comment #24)
> Can you help me proceed further?
> Is there any bug you'd like to point me to?

Bug 1212280 would be a good followup to this bug.
You need to log in before you can comment on or make changes to this bug.