Closed
Bug 1396975
Opened 7 years ago
Closed 7 years ago
label text and checkbox for Title Bar are black while the label text and widgets for other options are gray
Categories
(Firefox :: Toolbars and Customization, defect, P5)
Firefox
Toolbars and Customization
Tracking
()
VERIFIED
FIXED
Firefox 57
People
(Reporter: asa, Assigned: vedantc98, Mentored)
References
Details
(Keywords: good-first-bug)
Attachments
(1 file, 2 obsolete files)
1000 bytes,
patch
|
johannh
:
review+
|
Details | Diff | Splinter Review |
In the customize window, at the bottom, the checkbox and label text for the Title Bar option are black. That looks a bit odd next to the gray label text and outlines of the select widgets to the right. This may be Windows only.
Updated•7 years ago
|
Whiteboard: [photon-visual] → [photon-visual] [triage]
Updated•7 years ago
|
Whiteboard: [photon-visual] [triage]
Comment 1•7 years ago
|
||
This is not a recent regression (can reproduce in release) but it would probably still be nice to fix it. To solve this bug, we have to share the color attribute between .customizationmode-button and .customizationmode-checkbox in https://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/browser/themes/shared/customizableui/customizeMode.inc.css#58,63,67. Bonus points if you share the margin and padding attributes as well.
Mentor: jhofmann
status-firefox55:
--- → wontfix
status-firefox56:
--- → fix-optional
Flags: qe-verify+
Keywords: good-first-bug
Priority: -- → P5
Assignee | ||
Comment 2•7 years ago
|
||
Can I take up this issue? I'm new to this community. As I see it, I need to add three shared attributes for color, margin and padding for .customizationmode-button and .customizationmode-checkbox. Anything else I would need to do?
Comment 3•7 years ago
|
||
(In reply to vedantc98 from comment #2) > Can I take up this issue? I'm new to this community. Absolutely. Thank you. > As I see it, I need to add three shared attributes for color, margin and > padding for .customizationmode-button and .customizationmode-checkbox. > Anything else I would need to do? I think that's all. You should verify that your fix worked by building Firefox locally. You can find instructions here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions
Assignee: nobody → vedantc98
Updated•7 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•7 years ago
|
||
I've built Firefox locally, and made the changes necessary. But the changes aren't too prominent, maybe because I'm running MacOS? Should I push my changes anyway?
Comment 5•7 years ago
|
||
I wouldn't expect this change to be very prominent, it changes the text color from black to gray. So please do submit your patch and I'll give it a look :)
Comment 7•7 years ago
|
||
Comment on attachment 8905445 [details] [diff] [review] bug_1396975.patch, Patch for the bug 1396795. Review of attachment 8905445 [details] [diff] [review]: ----------------------------------------------------------------- This looks good and works for me, just two minor things to fix :) Thanks! ::: browser/themes/shared/customizableui/customizeMode.inc.css @@ +60,2 @@ > margin: 6px 10px; > padding: 2px 5px; You can remove the margin and padding here. @@ +63,4 @@ > -moz-appearance: none; > } > > +.customizationmode-checkbox, .customizationmode-button { Nit: we usually put each selector on a separate row, so .customizationmode-checkbox, .customizationmode-button {
Attachment #8905445 -
Flags: review?(jhofmann) → review-
Assignee | ||
Comment 8•7 years ago
|
||
Okay, I've added the changes. Please tell me if I need to change anything more. Thanks. :)
Attachment #8905456 -
Flags: review?(jhofmann)
Comment 9•7 years ago
|
||
Comment on attachment 8905456 [details] [diff] [review] bug_1396975.patch Review of attachment 8905456 [details] [diff] [review]: ----------------------------------------------------------------- Hey, this patch combined with the other patch looks good to me. Please merge the two matches using something like hg histedit (https://www.mercurial-scm.org/wiki/HisteditExtension, you need to mark the second patch with "r" to roll it into the first) and re-submit the result. Thank you. ::: browser/themes/shared/customizableui/customizeMode.inc.css @@ +60,5 @@ > background-color: #fcfcfd; > -moz-appearance: none; > } > > +.customizationmode-checkbox, Nit: please remove the trailing whitespace.
Attachment #8905456 -
Flags: review?(jhofmann) → feedback+
Assignee | ||
Comment 10•7 years ago
|
||
I've removed the unnecessary whitespace and rolled back the second commit, so the bug fix is now in one commit. Thank you :)
Attachment #8905445 -
Attachment is obsolete: true
Attachment #8905456 -
Attachment is obsolete: true
Attachment #8905833 -
Flags: review?(jhofmann)
Comment 11•7 years ago
|
||
Comment on attachment 8905833 [details] [diff] [review] bug_1396975.patch Review of attachment 8905833 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thank you!
Attachment #8905833 -
Flags: review?(jhofmann) → review+
Comment 12•7 years ago
|
||
I'll set the checkin-needed flag so that someone lands this patch for us. Normally we'd also do a try run (https://wiki.mozilla.org/ReleaseEngineering/TryServer) to make sure no automated tests break, but let's skip it this time since we only touched CSS (famous last words). If you're looking for another good starter bug, I need someone to take over bug 1359289. Let me know if you're interested :)
Keywords: checkin-needed
Assignee | ||
Comment 13•7 years ago
|
||
Thanks for guiding me through the process I'm looking to actively contribute to Mozilla, so I'd love to take up another bug. Should I ask to be assigned on bug 1359289's comments? Thanks again.
Comment 14•7 years ago
|
||
(In reply to Vedant Chakravadhanula from comment #13) > Thanks for guiding me through the process > I'm looking to actively contribute to Mozilla, so I'd love to take up > another bug. > Should I ask to be assigned on bug 1359289's comments? > > Thanks again. I'll just assign you, thanks! :)
Comment 15•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/563cbdd108d6 Change the title bar checkbox and button in the customization screen to look similar to the rest of the options. r=johannh
Keywords: checkin-needed
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/563cbdd108d6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 17•7 years ago
|
||
I have reproduced this bug with Nightly 57.0a1 (2017-09-05) on Windows 8.1 , 64 Bit ! This bug's fix is Verified with latest Nightly 57.0a1 ! Build ID 20170913100125 User Agent Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170913]
Updated•7 years ago
|
Comment 18•7 years ago
|
||
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID: 20170918100059 I have reproduced this bug on Nightly 57.0a1 (Build ID: 20170901100059 ) on Windows 8.1 x64. This issue has been verified on Mac OS 10.12, Windows 8.1 x64 and Ubuntu 14.04 with latest Nightly 57.0a1 and I cannot reproduce it. The text color for the buttons from the bottom of the nightly customization page have been changed to gray.
Comment 19•7 years ago
|
||
Based on comments 17 and 18, mark this bug verified as fixed.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•