label text and checkbox for Title Bar are black while the label text and widgets for other options are gray

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Toolbars and Customization
P5
normal
VERIFIED FIXED
2 months ago
2 months ago

People

(Reporter: asa, Assigned: vedantc98, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 57
good-first-bug
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox55 wontfix, firefox56 wontfix, firefox57 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 months ago
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

2 months ago
Whiteboard: [photon-visual] → [photon-visual] [triage]

Updated

2 months ago
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@mozilla.com
status-firefox55: --- → wontfix
status-firefox56: --- → fix-optional
Flags: qe-verify+
Keywords: good-first-bug
Priority: -- → P5
(Assignee)

Comment 2

2 months 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?
(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
(Assignee)

Comment 4

2 months 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?
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 :)
(Assignee)

Comment 6

2 months ago
Created attachment 8905445 [details] [diff] [review]
bug_1396975.patch, Patch for the bug 1396795.

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-
(Assignee)

Comment 8

2 months ago
Created attachment 8905456 [details] [diff] [review]
bug_1396975.patch

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+
(Assignee)

Comment 10

2 months ago
Created attachment 8905833 [details] [diff] [review]
bug_1396975.patch

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
(Assignee)

Comment 13

2 months 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.
(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

2 months 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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/563cbdd108d6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

2 months ago
Depends on: 1398549

Comment 17

2 months 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]
status-firefox56: fix-optional → wontfix
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
status-firefox57: fixed → verified
You need to log in before you can comment on or make changes to this bug.