Closed Bug 1396975 Opened 2 years ago Closed 2 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)

defect

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- verified

People

(Reporter: asa, Assigned: vedantc98, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file, 2 obsolete files)

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.
Whiteboard: [photon-visual] → [photon-visual] [triage]
Whiteboard: [photon-visual] [triage]
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
Flags: qe-verify+
Keywords: good-first-bug
Priority: -- → P5
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?
(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
Status: NEW → ASSIGNED
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?
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 :)
Thanks :)
Attachment #8905445 - Flags: review?(jhofmann)
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-
Attached patch bug_1396975.patch (obsolete) — Splinter Review
Okay, I've added the changes.
Please tell me if I need to change anything more.
Thanks. :)
Attachment #8905456 - Flags: review?(jhofmann)
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+
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 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+
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
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.
(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! :)
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
https://hg.mozilla.org/mozilla-central/rev/563cbdd108d6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1398549
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]
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.
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.