Closed Bug 1202618 Opened 9 years ago Closed 9 years ago

Remove legacy transparent borders from toolbarbuttons on OS X

Categories

(Toolkit :: Themes, defect)

Product:
Toolkit
Component:
Themes
Platform:
Unspecified
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: dao, Assigned: batmanav10, Mentored)

References

Details

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

Attachments

(1 file, 5 obsolete files)

Removed transparent borders from toolbarbuttons on OSX theme
1.05 KB, patch
dao
: review+
Details | Diff | Splinter Review
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.
Flags: needinfo?
Flags: needinfo?
Can it be assigned to me?
sure
Assignee: nobody → leo_2b2006
Because I am new. May you tell me what I should do?
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.
How do I run the code to test it after I finish it?
Are you on OS X?
I have windows and os x.
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.
If I want to solve it in windows, is it possible like using virtual os?
This bug is only present on OS X, you won't be able to test the fix on Windows.
Attached patch toolbarbutton.css (obsolete) — — Splinter Review
Hi, I think I solved this bug. Could you please have a look?
Attachment #8666910 - Flags: review?(dao)
Also, could you assign it to me, if it isn't ready to be merged? Or even if it is.
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)
Hey, I figured I attached the wrong file earlier. Please find the diff attached for review. Thanks!
Attachment #8667267 - Flags: review?(dao)
Attachment #8666910 - Attachment is obsolete: true
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)
Attachment #8667267 - Attachment is obsolete: true
Attachment #8667267 - Flags: review?(dao)
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-
Assignee: leo_2b2006 → batmanav10
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)
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+
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+
(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?
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)
Comment on attachment 8669008 [details] [diff] [review] Removed transparent borders from toolbarbuttons on OSX theme Perfect, thanks.
Attachment #8669008 - Flags: review?(dao) → review+
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
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1212280
(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.

Attachment

General

Creator:
Created:
Updated:
Size: