Closed Bug 1478156 Opened Last year Closed 2 months ago

Adding color contrast information to the color picker

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox70 fixed)

RESOLVED FIXED
Firefox 70
Tracking Status
firefox70 --- fixed

People

(Reporter: mtigley, Assigned: mislam, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(19 files, 1 obsolete file)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
146.13 KB, image/png
Details
174.95 KB, image/png
Details
139.24 KB, image/png
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
162.31 KB, image/png
Details
129.90 KB, image/png
Details
189.89 KB, image/png
Details
106.66 KB, image/png
Details
118.61 KB, image/png
Details
188.55 KB, image/png
Details
136.89 KB, image/png
Details
274.31 KB, image/png
Details
170.86 KB, image/png
Details
220.24 KB, image/png
Details
Contrast ratio information of the selected color will be added in this task according to the new design: https://mozilla.invisionapp.com/share/CGMV7JMRV36#/screens

Much of the groundwork for this component has already been developed in Bug1332090 and all that needs to be done is porting this code over to the current picker and polished.

Hi Victoria, my new intern, Maliha, will be picking this up starting next week and I was wondering if you had any comments/updates to the wireframes that were originally considered for this bug? Thanks

Flags: needinfo?(victoria)
Mentor: yzenevich
Assignee: nobody → maliha.rh
Status: NEW → ASSIGNED
Attachment #9065154 - Attachment is obsolete: true
See Also: → 1003289

So excited to see this work happening! The original mockups should still be accurate :) but let me know if any new questions come up.

Flags: needinfo?(victoria)
Attached image Annotation Collapsed

Hi Victoria,

We decided to do an expandable annotation section instead of showing it on hover. I attached some screenshots. Please let us know what you think!

Maliha

Flags: needinfo?(victoria)
Attached image Annotation Expanded
Attached image Error: No Annotation

(In reply to Maliha Islam [:mislam] from comment #8)

Created attachment 9070032 [details]
Annotation Collapsed

Hi Victoria,

We decided to do an expandable annotation section instead of showing it on hover. I attached some screenshots. Please let us know what you think!

Maliha

To add more context, we are considering the expandable section as it is more inclusive than hover activatable tooltip + leaves an option for potentially add an eye-dropper button for the background colour.

Attachment #9067377 - Attachment description: Bug 1478156 - Move shared methods, styles and constants to shared files, r=yzen → Bug 1478156 - Move shared code to be used by color contrast feature, r=yzen

This looks great! And that reasoning for the expandable UI totally makes sense.

Minor visual tweak: would you mind adding a little extra bottom padding to the whole tooltip? (4px extra space between the contrast info and the bottom of the tooltip, so that the contrast section looks more vertically centered in that space).

Related good news: All the tooltips are being updated to a design that is no longer translucent, so everything will be easier to read https://bugzilla.mozilla.org/show_bug.cgi?id=1553472

Flags: needinfo?(victoria)

Will do! Thanks, Victoria :) It's great that we are planning to make the tooltip backgrounds a solid color, I always found it a little distracting to see the devtools body through it.

Attachment #9067378 - Attachment description: Bug 1478156 - Add color contrast span to color picker, r=yzen → Bug 1478156 - Add color contrast span to color picker, r=yzen,gl
Attachment #9067378 - Attachment description: Bug 1478156 - Add color contrast span to color picker, r=yzen,gl → Bug 1478156 - Add color contrast span to color picker, r=yzen, gl
Attachment #9068056 - Attachment description: Bug 1478156 - Add collapsible contrast ratio annotation section to color picker, r=yzen → Bug 1478156 - Add collapsible contrast ratio annotation section to color picker, r=yzen, gl
Attachment #9070124 - Attachment description: Bug 1478156 - Move shareable focus methods to be used for color picker accessibility, r=yzen → Bug 1478156 - Move shareable focus methods to be used for color picker accessibility, r=yzen, gl
Attachment #9067378 - Attachment description: Bug 1478156 - Add color contrast span to color picker, r=yzen, gl → Bug 1478156 - Add color contrast span to color picker, r=yzen
Attachment #9068056 - Attachment description: Bug 1478156 - Add collapsible contrast ratio annotation section to color picker, r=yzen, gl → Bug 1478156 - Add collapsible contrast ratio annotation section to color picker, r=yzen
Attachment #9070124 - Attachment description: Bug 1478156 - Move shareable focus methods to be used for color picker accessibility, r=yzen, gl → Bug 1478156 - Move shareable focus methods to be used for color picker accessibility, r=yzen
Attachment #9067378 - Attachment description: Bug 1478156 - Add color contrast span to color picker, r=yzen → Bug 1478156 - Add color contrast span to color picker, r=yzen,gl
Attachment #9067378 - Attachment description: Bug 1478156 - Add color contrast span to color picker, r=yzen,gl → Bug 1478156 - Add color contrast span to color picker, r=yzen
Attachment #9068056 - Attachment description: Bug 1478156 - Add collapsible contrast ratio annotation section to color picker, r=yzen → Bug 1478156 - Add contrast ratio annotation section to color picker, r=yzen
Attachment #9069062 - Attachment description: Bug 1478156 - Make color picker tooltip accessible, r=yzen → Bug 1478156 - Make color picker tooltip keyboard accessible, r=yzen

Hi Victoria,

We decided to make some changes to the color picker I showed you last time (please check the last few attachments). There is no expandable section anymore and we decided to add a color name (or "closest to" color name) title when the user hovers over the spectrum dragger or the color preview circle. I think it would also be nice to make this color name more visible in the color picker as a span, but not sure how this will fit in with the current design. What are your thoughts?

Flags: needinfo?(victoria)

Hi Maliha!

I talked to Yura a bit to learn more about the reasoning behind the changes, and here's what I've come up with. See this new mockup for a visual reference.

  1. I think the info text is a bit too much to have always showing, since it's more of a notice that most people won't need after seeing it for the first time. Since we can't put the info text in a expandable section anymore (without a lot of extra work, sounds like) I'd suggest we instead move it back into the tooltip. However, we can also add an info icon which the user can click to learn more about color contrast.

The simplest link to use is this MDN page, though its tutorial-like format doesn't really fit our use case. A better approach could be to make a new section in the DevTools color picker page that gives a compact overview of the WCAG specs. I'll ping some folks from MDN to see if they have thoughts on this. For reference, Chrome's color contrast page which is linked to from their color picker seems really nice.

  1. I think it would be best to show the color name tooltip only in the color preview circle, and not over the whole spectrum rectangle, as it seems a little distracting in the spectrum since it's such a large target.

  2. Here are a few small visual tweaks:

  • A little more left margin on the Contrast text would be great to line up with the eyedropper icon (about 12px looks like)
  • Move the first of the horizontal sliders up a bit so it lines up with the color preview circle. The extra spacing between the two sliders should help usability as well.

Thanks - let me know if anything is unclear!

Flags: needinfo?(victoria)
Attached image image.png

Hi Irene and Chris! For this new color picker, we want to add a link to an MDN page that explains color contrast for accessibility, including the WCAG ratio numbers and "AA/AAA" terminology.

This MDN page on color contrast would be the easiest solution, though its tutorial-like format doesn't really fit our need, which is for a more compact informational page. A better approach could be to make a new section in the DevTools color picker page. For reference, Chrome's color contrast page which is linked to from their color picker seems very nice. Would love to hear your thoughts on how to proceed.

Flags: needinfo?(irenesmith13)
Flags: needinfo?(cmills)

Hi Victoria!

The color contrast page I wrote is designed to be linked to from the color contrast accessibility check results in the a11y inspector. It is not really a tutorial, but more of an informational page. I think I'd prefer to keep information on how to fix color contrast issues and explanation of the related WCAG criteria in that page, and link to it from the color picker page, which should be more focused on just covering the tool features.

I do think that https://developer.mozilla.org/en-US/docs/Web/Accessibility/Understanding_WCAG/Perceivable/Color_contrast#Solution could be expanded to include better information on what the contrast ratios mean, and the WCAG criteria, in the same sort of way as the Google article does.

Would this work for you?

Flags: needinfo?(victoria)
Flags: needinfo?(irenesmith13)
Flags: needinfo?(cmills)
Attachment #9068056 - Attachment description: Bug 1478156 - Add contrast ratio annotation section to color picker, r=yzen → Bug 1478156 - Add contrast ratio annotation title and info link to color picker, r=yzen

:victoria I have attached a few screenshots of the updated tooltip. Let me know if you have any questions :)

Flags: needinfo?(victoria)
Flags: needinfo?(victoria)

Hi Maliha, these look beautiful! Thanks so much for making the changes.

One little tweak: Right side of the spectrum rectangle looks like it needs a bit more margin to match the left side.

And one slightly bigger question about title attr usability for both you and Yura:
In the mockup I had a light gray underline under the contrast ratio/icon to hint that there's extra info to be found there (since it's not just alt text). It's inspired by the traditional dotted-underline styling for abbreviations. We could add that in, but now that I'm looking at your screenshots, I wonder if we should combine the two title tags into just one, which would be found by hovering over the (?). It could say (assuming ability to use new lines):

Does not meet WCAG standards for accessible text
Calculated against background: #ffffff
Learn more on MDN

What do you think?

Flags: needinfo?(yzenevich)
Flags: needinfo?(malihaislam81)

Hi Chris!

The color contrast page I wrote is designed to be linked to from the color contrast accessibility check results in the a11y inspector. It is not really a tutorial, but more of an informational page. I think I'd prefer to keep information on how to fix color contrast issues and explanation of the related WCAG criteria in that page, and link to it from the color picker page, which should be more focused on just covering the tool features.

I do think that https://developer.mozilla.org/en-US/docs/Web/Accessibility/Understanding_WCAG/Perceivable/Color_contrast#Solution could be expanded to include better information on what the contrast ratios mean, and the WCAG criteria, in the same sort of way as the Google article does.

Thanks for the info! I see how that page makes sense when linked to from the a11y inspector. For linking from the color picker, I feel it would help to have maybe 1-2 sentences on contrast ratios and WCAG criteria near the top of the page, as a quick summary, since I feel there's a more time-sensitive need to know what the UI means rather than just the concept of fixing contrast issues. (I guess we could also link to the Solution section)

Here's a quick attempt at the sort of thing I'm thinking of:
"When designing readable interfaces for different vision capabilities, a good rule of thumb is the WCAG guidelines which recommend an AA contrast ratio of 4.5:1 or higher for text. You can increase the contrast to an AAA rating of 7:1 to cover a greater range of deficiencies. Very large text (120-150% larger than the body text) can go down to 3:1."

Flags: needinfo?(victoria)

I thought a little more about the combined title attributes and now think it’s a bad idea 😅. Poor title tooltip would be doing too much work. I think if we keep it as is but just add a subtle underline indicator to the contrast info, that will be fine for now.

Flags: needinfo?(yzenevich)
Flags: needinfo?(malihaislam81)

Victoria, thanks a lot for your feedback! I have made the changes you suggested. We have decided to shorten the title attr for the info button to "Learn more" to reuse localization string and be consistent with the accessibility panel. Let me know if there is any final thing I need to update :)

Flags: needinfo?(victoria)

These screenshots look great and that sounds good re: the alt text! Thank you Maliha!

Flags: needinfo?(victoria)
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/041f51c9d4d1
Move shared code to be used by color contrast feature, r=yzen,gl
https://hg.mozilla.org/integration/autoland/rev/314855eab887
Add color contrast span to color picker, r=yzen,gl
https://hg.mozilla.org/integration/autoland/rev/a363971fffb0
Add contrast ratio annotation title and info link to color picker, r=yzen,gl
https://hg.mozilla.org/integration/autoland/rev/83a10c635fe9
Move shareable focus methods to be used for color picker accessibility, r=yzen,gl
https://hg.mozilla.org/integration/autoland/rev/fcc9123589fd
Make color picker tooltip keyboard accessible, r=yzen,gl
https://hg.mozilla.org/integration/autoland/rev/5ca694230ffa
Make color picker tooltip screen reader accessible, r=yzen,gl
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e16036e3719c
Move shared code to be used by color contrast feature, r=yzen,gl
https://hg.mozilla.org/integration/autoland/rev/1a57ac83b59c
Add color contrast span to color picker, r=yzen,gl
https://hg.mozilla.org/integration/autoland/rev/2bfcab598ff4
Add contrast ratio annotation title and info link to color picker, r=yzen,gl
https://hg.mozilla.org/integration/autoland/rev/71c74b7d0e45
Move shareable focus methods to be used for color picker accessibility, r=yzen,gl
https://hg.mozilla.org/integration/autoland/rev/c3bb4cfd04dc
Make color picker tooltip keyboard accessible, r=yzen,gl
https://hg.mozilla.org/integration/autoland/rev/83881815f007
Make color picker tooltip screen reader accessible, r=yzen,gl

Comment on attachment 9067377 [details]
Bug 1478156 - Move shared code to be used by color contrast feature, r=yzen

Beta/Release Uplift Approval Request

  • User impact if declined: Users will not have a color contrast information available for text color pickers which would help accessibility effort.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): These patches include improves the way keyboard and screen reader users can use the color picker , some of the color picker components had to be substantially updated. This is all front end code within the devtools inspector panel (no platform changes)
  • String changes made/needed: 6 strings added to devtools/client/locales/en-US/inspector.properties and 1 string added to devtools/client/locales/en-US/accessibility.properties in total.
Attachment #9067377 - Flags: approval-mozilla-beta?
Attachment #9067378 - Flags: approval-mozilla-beta?
Attachment #9068056 - Flags: approval-mozilla-beta?
Attachment #9069062 - Flags: approval-mozilla-beta?
Attachment #9070124 - Flags: approval-mozilla-beta?
Attachment #9074094 - Flags: approval-mozilla-beta?

Hi flod, are we OK taking this patch with string changes?

Flags: needinfo?(francesco.lodolo)

I think at this point it's a question for Release Management to answer, more than l10n.

We keep getting in this situation (DevTools patches with strings in need of uplift, IIRC a few of them from accessibility), and I wonder what's the point of having a rule if we keep making exception to it. This looks like feature development (and the bug is over a year old), not bug fixing.

Last note: these strings landed less than 48h ago, they're not even exposed to localization yet, let alone localized.

Flags: needinfo?(francesco.lodolo)

Comment on attachment 9067377 [details]
Bug 1478156 - Move shared code to be used by color contrast feature, r=yzen

I agree that this is pushing it, especially when we're already nearly halfway into the Beta cycle. Let's let this ride the trains.

Attachment #9067377 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9067378 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9068056 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9069062 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9070124 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9074094 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Regressions: 1568250
Regressions: 1571237
You need to log in before you can comment on or make changes to this bug.