1324172 - Indented descriptions should use a lighter font color

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

From slide 12 and 13 of https://bugzilla.mozilla.org/attachment.cgi?id=8819509

Comment 1

[John Drinkwater (:beta)]

Thought most of the feedback in that guideline was fair, but found little to justify this change - Font is smaller & already indented, however robbing it of the contrast to the background makes it harder to read.
Hopefully if this change does happen it wont be as light as their example.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Using the DevTools I checked the font-sizes and in what we are shipping the font is not smaller.

For the gray text we will use 'color: GrayText', which on my machine results in rgb(109,109,109). The background-color of the page is rgb(251,251,251). Using WCAG Contrast formula (simple web form available at http://webaim.org/resources/contrastchecker/), there is a 5:1 contrast ratio between the two colors. This contrast passes the Normal Text WCAG AA test and fails the WCAG AAA test. I haven't seen us require WCAG AAA in other parts of our UI.

I measured the font-size difference in the spec. The indented text is one pixel shorter in height from the screenshots, so it would be a subtle difference but shouldn't affect readability.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

The spec says:
font-size: 13px
font-color: #858585
line-height: 21px

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Hey Jared, are we okay to proceed with the spec's colours, or should we go back to UX (or a11y) and have them evaluate?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

I think the colors here will be okay as far as contrast goes, after the examination I did in comment #2, but I am a bit worried that users may think this is disabled text and not supplemental text. Let's bring that up with UX.

Tina, what are your thoughts about people confusing the text for being disabled or not being applicable because they think it is disabled?

Comment 6

[Tina Hsieh[:Tina_Hsieh] UX]

Attachment: Proposed font

Hey Jared,
I won't see it as a possibility to confuse people since the description has no checkbox in the front. The font color #858585 is accessible and can easily be distinguished from the option text.

I had a discussion with our visual designer Helen. She made an example that shows the comparison of the normal state, current disabled state, and the disabled state with lower opacity (30%). Making the opacity lower to show the differentiation might be a choice, but it's not accessible and different from the opacity defined in the current Firefox styleguide. 

Since I don't think the proposed font will confuse people and we need the option text to stand out from lines, I'll suggest us to use the proposed description font :)

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Thank you, the lower opacity (30%) looks to have far too low contrast to be usable. We can use the "proposed font" color, #858585 and leave the font-size and line-height unchanged.

Comment 8

[chsiang]

Hi Helen,
Kindly provide the final specs for this after your call tonight. 
Thanks!

Comment 9

[Helen Huang [:HHuang]]

Sorry for the late reply. 
Base on the latest color palette, I will change the font color to #737373, please help to fix this, thanks!

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This is a stlying follow up for re-org, not visual update for 57.

Comment 12

[Francis Lee [:frlee]]

remove whiteboard tag due to its duplicate

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Are you sure comment 10 is incorrect?

Comment 14

[Ricky Chien [:rickychien] (inactive)]

Sorry I didn't look through this issue carefully! Let's ask Tina for confirmation.

Reorg v2 (bug 1365133) is ready to land in 56. This change mentioned in description was targeting on Reorg v1 in 55. 

Tina, can you confirm that this change will need to be shipped in Reorg v1 (55) or Reorg v2 (56) or both? Otherwise, does it cover by visual refresh spec in 57 so that we can do it with a sweep in 57 implementation?

Comment 15

[Tina Hsieh[:Tina_Hsieh] UX]

Hey Ricky,
The description font style is very important for visual hierarchy. The style we use in 57 is the same us what we proposed before, so there won't be a repetitive work. Let's implement in 56 or even earlier :)

Comment 16

[Ricky Chien [:rickychien] (inactive)]

Hi Helen,

We'd like to get double confirmation here. Can you tell us that the proposed font for descriptions is same with the style we use in 57 and even it won't be a repetitive work as well, is that still recommended to land in 56?

Thanks

Comment 17

[Helen Huang [:HHuang]]

Yes, the font style for descriptions in 57 is same as what we proposed before, and that will be great if it could land in 56.

font-size: 13px
font-color: #737373

Comment 19

[Evan Tseng [:evanxd]]

Attachment: Bug 1324172 - Add css styles for indented descriptions.

Review commit: https://reviewboard.mozilla.org/r/155642/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155642/

Comment 20

[Evan Tseng [:evanxd]]

Hi Jared,

Could you help review the patch?

Thank you.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8884756 [details]
Bug 1324172 - Add css styles for indented descriptions.

https://reviewboard.mozilla.org/r/155642/#review161434

::: browser/components/preferences/in-content-new/main.xul:908
(Diff revision 1)
> -    <description id="contentProcessCountEnabledDescription">&limitContentProcessOption.description;</description>
> -    <description id="contentProcessCountDisabledDescription">&limitContentProcessOption.disabledDescription;<label class="text-link" href="https://wiki.mozilla.org/Electrolysis">&limitContentProcessOption.disabledDescriptionLink;</label></description>
> +    <description id="contentProcessCountEnabledDescription" class="light">&limitContentProcessOption.description;</description>
> +    <description id="contentProcessCountDisabledDescription" class="light">&limitContentProcessOption.disabledDescription;<label class="text-link" href="https://wiki.mozilla.org/Electrolysis">&limitContentProcessOption.disabledDescriptionLink;</label></description>

Please undo these changes, see the other review comment for why.

::: browser/themes/shared/incontentprefs/preferences.inc.css:15
(Diff revision 1)
> +description.indent,
> +description.light {
> +  font-size: 1.18rem;
> +  color: #737373;

Please just use:

description.indent,
.indent > description {
  font-size: 1.18rem;
  color: #737373;
}

Comment 22

[Evan Tseng [:evanxd]]

Comment on attachment 8884756 [details]
Bug 1324172 - Add css styles for indented descriptions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155642/diff/1-2/

Comment 23

[Evan Tseng [:evanxd]]

Comment on attachment 8884756 [details]
Bug 1324172 - Add css styles for indented descriptions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155642/diff/2-3/

Comment 24

[Evan Tseng [:evanxd]]

Updated the patch for the review comments. Thank you for review, Jared.

Let's land the patch after the try[1] is good.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a049fe4d7e1e

Comment 25

[Evan Tseng [:evanxd]]

Looks like the try is good. Land it.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b940bc3aab48
Add css styles for indented descriptions. r=jaws

Comment 27

[Mozilla Autoland]

We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b940bc3aab48

Comment 29

[Hani Yacoub, Desktop QA]

Build ID: 20170817100132
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Verified as fixed on Firefox Nightly 57.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.