Closed Bug 1794369 Opened 2 years ago Closed 2 years ago

[Colorway Closet] “Phrase not found” warning is not visible with Playmaker theme, Balanced intensity

Categories

(Firefox :: Theme, defect, P3)

Product:
Firefox
Component:
Theme
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
108 Branch
Tracking Flags:
Tracking Status
firefox106 --- wontfix
firefox107 --- wontfix
firefox108 --- verified

People

(Reporter: asoncutean, Assigned: amy)

References

Details

(Whiteboard: [fidefe-colorway-closet])

Attachments

(3 files)

screenshoot issue.png
2.67 KB, image/png
Details
error in light and balanced intensities + figma design system treatment of errored text input field
87.63 KB, image/png
Details
Bug 1794369 - Change findbar “phrase not found” error message to text color. r=bigiri
48 bytes, text/x-phabricator-request
Details | Review
Attached image screenshoot issue.png —

Found in

  • 107.0a1

Affected versions

  • 107.0a1
  • 106.0b9

Affected platforms

  • Windows 10
  • Ubuntu 20.4
  • macOS 12

Steps to reproduce

  1. Make sure “browser.theme.colorway-closet" is set to true in about:config
  2. Open Colorways modal from about:addons
  3. Set Playmaker theme, Balanced intensity
  4. Go to any webpage
  5. Open Find bar (CTRL + F)
  6. Enter any input that will not generate a matching result
  7. Observe the “Phrase not found” on the Find bar

Expected result

  • The warning phrase is visible.

Actual result

  • The contrast is low, the warning phrase can barely be seen.

Regression range

  • Not a regression, I can see this issue way back to 103.0a1.

Additional notes

  • The contrast is not ideal for other themes that uses red or yellow, but they are fairly visible.
Has STR: --- → yes
Severity: S4 → S3
Priority: -- → P3
Whiteboard: [fidefe-colorway-closet]

Misha -- Not pressing now, but we'll need design direction on this one eventually.

Flags: needinfo?(mbruk)

heard! looks like this might have gotten overlooked as a use case in the original designs, i will provide mocks this week :)

Flags: needinfo?(mbruk)

(In reply to misha [:mbruk] from comment #2)

heard! looks like this might have gotten overlooked as a use case in the original designs, i will provide mocks this week :)

Thanks Misha! One thought I had that may be lower lift than considering error color(s) that work for every theme is having this message in the text color (white, in this case) with an error icon next to it -- (!) Phrase not found.

I remember reading accessibility-wise it is good to pair text with imagery to communicate error versus color, but I'd look to your best judgement in this department. :)

(In reply to Amy Churchwell [:amy] from comment #3)

(In reply to misha [:mbruk] from comment #2)

heard! looks like this might have gotten overlooked as a use case in the original designs, i will provide mocks this week :)

Thanks Misha! One thought I had that may be lower lift than considering error color(s) that work for every theme is having this message in the text color (white, in this case) with an error icon next to it -- (!) Phrase not found.

I agree with that, but one thing to consider is whether to apply that for all themes (light and dark themes included), or just to Colorway themes.
This issue can be reproduced in webextension themes as well.

Adding a related contrast issue here for reference as well:
https://bugzilla.mozilla.org/show_bug.cgi?id=1784439

Additionally, the search field color in this scenario may also benefit from some consistency as it also has some contrast concerns depending on which Colorway is active.

(In reply to Amy Churchwell [:amy] from comment #3)

(In reply to misha [:mbruk] from comment #2)

heard! looks like this might have gotten overlooked as a use case in the original designs, i will provide mocks this week :)

Thanks Misha! One thought I had that may be lower lift than considering error color(s) that work for every theme is having this message in the text color (white, in this case) with an error icon next to it -- (!) Phrase not found.

I remember reading accessibility-wise it is good to pair text with imagery to communicate error versus color, but I'd look to your best judgement in this department. :)

that is literally what i was going to propose :) that we match whatever the text color is in the rest of the find in page (sometimes white sometimes dark depending on the colorway selected) with the addition of a warning icon

(In reply to misha [:mbruk] from comment #6)

(In reply to Amy Churchwell [:amy] from comment #3)

(In reply to misha [:mbruk] from comment #2)

heard! looks like this might have gotten overlooked as a use case in the original designs, i will provide mocks this week :)

Thanks Misha! One thought I had that may be lower lift than considering error color(s) that work for every theme is having this message in the text color (white, in this case) with an error icon next to it -- (!) Phrase not found.

I remember reading accessibility-wise it is good to pair text with imagery to communicate error versus color, but I'd look to your best judgement in this department. :)

that is literally what i was going to propose :) that we match whatever the text color is in the rest of the find in page (sometimes white sometimes dark depending on the colorway selected) with the addition of a warning icon

haha, amazing! :D
Do you think this icon https://searchfox.org/mozilla-central/source/toolkit/themes/shared/icons/warning.svg works for this?

here is my rec: let's match the color of the 'phrase not found' message to the rest of the text and icons in that bar (in a given colorway sometimes it'll be dark and sometimes light)

the last bar in that screenshot is how we handle an error state in the figma ds - which is different from what we do live, i'm going to check with design system team about this but in the meantime, for the sake of moving things along, let's treat the background of the input the same as if there wasn't an error (for colorways)

thanks! and plz reach out if i can help provide more clarity

Assignee: nobody → achurchwell

FYI: we also seem to be using this error color-mix in a couple of other places such as, panel.css (identity panel) and print.css --
https://searchfox.org/mozilla-central/search?q=color-mix(in+srgb%2C+currentColor+40%25%2C+%23C50042)&path=&case=false&regexp=false

(In reply to Amy Churchwell [:amy] from comment #9)

FYI: we also seem to be using this error color-mix in a couple of other places such as, panel.css (identity panel) and print.css --
https://searchfox.org/mozilla-central/search?q=color-mix(in+srgb%2C+currentColor+40%25%2C+%23C50042)&path=&case=false&regexp=false

btw specifically for the print popup it doesn't matter as we use common-shared.css there, and webext themes can't affect how it'd look. We cater only for light and dark mode so this isn't an issue there.

(In reply to Itiel from comment #11)

(In reply to Amy Churchwell [:amy] from comment #9)

FYI: we also seem to be using this error color-mix in a couple of other places such as, panel.css (identity panel) and print.css --
https://searchfox.org/mozilla-central/search?q=color-mix(in+srgb%2C+currentColor+40%25%2C+%23C50042)&path=&case=false&regexp=false

btw specifically for the print popup it doesn't matter as we use common-shared.css there, and webext themes can't affect how it'd look. We cater only for light and dark mode so this isn't an issue there.

How about drop the color and make text bold instead to keep the emphasis for the find bar and the site identity panel? I feel like just dropping the color without a replacement isn't that great as the emphasis serves a purpose in both cases.

Flags: needinfo?(mbruk)
Flags: needinfo?(achurchwell)
Pushed by achurchwell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea71acdaaf17
Change findbar “phrase not found” error message to text color. r=desktop-theme-reviewers,Itiel

(In reply to Dão Gottwald [::dao] from comment #12)

(In reply to Itiel from comment #11)

(In reply to Amy Churchwell [:amy] from comment #9)

FYI: we also seem to be using this error color-mix in a couple of other places such as, panel.css (identity panel) and print.css --
https://searchfox.org/mozilla-central/search?q=color-mix(in+srgb%2C+currentColor+40%25%2C+%23C50042)&path=&case=false&regexp=false

btw specifically for the print popup it doesn't matter as we use common-shared.css there, and webext themes can't affect how it'd look. We cater only for light and dark mode so this isn't an issue there.

How about drop the color and make text bold instead to keep the emphasis for the find bar and the site identity panel? I feel like just dropping the color without a replacement isn't that great as the emphasis serves a purpose in both cases.

This makes sense to me. I can send a patch in a followup if there's an agreement on this.

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

Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox108: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 108 Branch

Verified fixed with 108.0a1 (2022-10-23) on Windows 10, Ubuntu 20.04 and macOS 11.

Status: RESOLVED → VERIFIED
status-firefox108: fixed → verified
Blocks: 1797058

(In reply to Itiel from comment #14)

(In reply to Dão Gottwald [::dao] from comment #12)

(In reply to Itiel from comment #11)

(In reply to Amy Churchwell [:amy] from comment #9)

FYI: we also seem to be using this error color-mix in a couple of other places such as, panel.css (identity panel) and print.css --
https://searchfox.org/mozilla-central/search?q=color-mix(in+srgb%2C+currentColor+40%25%2C+%23C50042)&path=&case=false&regexp=false

btw specifically for the print popup it doesn't matter as we use common-shared.css there, and webext themes can't affect how it'd look. We cater only for light and dark mode so this isn't an issue there.

How about drop the color and make text bold instead to keep the emphasis for the find bar and the site identity panel? I feel like just dropping the color without a replacement isn't that great as the emphasis serves a purpose in both cases.

This makes sense to me. I can send a patch in a followup if there's an agreement on this.

Filed bug 1797058.

Flags: needinfo?(mbruk)
Flags: needinfo?(achurchwell)

The patch landed in nightly and beta is affected.
:amy, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox107 to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(achurchwell)
Flags: needinfo?(achurchwell)

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #18)

The patch landed in nightly and beta is affected.
:amy, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox107 to wontfix.

For more information, please visit auto_nag documentation.

I don't believe that this bug requires an uplift, thank you.

status-firefox107: affected → wontfix
Regressions: 1806826
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: