Closed Bug 1845151 Opened 10 months ago Closed 25 days ago

Remove all code associated with the message-bar component

Categories

(Toolkit :: UI Widgets, task)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
127 Branch
Tracking Flags:
Tracking Status
firefox127 --- fixed

People

(Reporter: hjones, Assigned: annhermy)

References

(Blocks 1 open bug)

Details

(Whiteboard: [recomp])

Attachments

(2 files)

Bug 1845151 - Remove all code associated with the message-bar component r=hjones
48 bytes, text/x-phabricator-request
Details | Review
before and after color change
351.80 KB, image/png
Details

Once all existing uses of message-bar have been replaced with moz-message-bar we should remove all the code associated with the older element as it will no longer be needed. It's likely we'll have to do this when we remove the last usage, as I expect we'll get a failure in the browser_all_files_referenced.js, but I'm filing this to make sure we do all the necessary cleanup. Specifically we should:

Depends on: 1844783
Whiteboard: [fidefe-reusable-components]
Depends on: 1855471
Depends on: 1851877
Whiteboard: [fidefe-reusable-components] → [recomp]
Assignee: nobody → annhermy
Status: NEW → ASSIGNED
Pushed by hjones@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d137dadab8f
Remove all code associated with the message-bar component r=hjones,desktop-theme-reviewers,reusable-components-reviewers,extension-reviewers,robwu

Backed out for causing bc failures in browser_parsable_css.js

Flags: needinfo?(annhermy)

(In reply to Cristian Tuns from comment #3)

Backed out for causing bc failures in browser_parsable_css.js

I wonder why we get this error when we use this variable here: https://searchfox.org/mozilla-central/source/toolkit/components/aboutwebauthn/content/aboutWebauthn.css#35

Flags: needinfo?(jules)
Flags: needinfo?(hjones)
Flags: needinfo?(annhermy)

(In reply to Ganna from comment #4)

(In reply to Cristian Tuns from comment #3)

Backed out for causing bc failures in browser_parsable_css.js

I wonder why we get this error when we use this variable here: https://searchfox.org/mozilla-central/source/toolkit/components/aboutwebauthn/content/aboutWebauthn.css#35

Dug into the code a bit and I'm pretty sure the issue is that the aboutWebauthn code is Mac/Linux only, so it doesn't get loaded on Windows - see here

That tracks with what we're seeing, since the test is only failing for Windows builds.

I think we have three options here:

  • Replace the --green-50 usage with one of our design tokens - we don't have anything super close, but maybe --color-green-30 would be alright (ideally mapped to a more semantic variable based on how it's being used). It's possible we have a green variable that is closer that we just haven't had a reason to use yet, in which case we could add that value to tokens-shared.css then use it here; :jules probably has more info about our colors. This will add a bit of scope, but is my personal preferred fix.
  • Wrap the --green-50 definition in a platform specific media query - something like @media not (-moz-platform: win) (ideally with a comment to explain why)
  • Add a rule to the ignoreList in the test file
Flags: needinfo?(hjones)

(In reply to Hanna Jones [:hjones] from comment #5)
I tried second option (wrapping the variable in a platform specific media query), but the test still fails. So I feel like the first option is the best. But I am not sure which semantic variable name we can choose for this color. --green-50 (#30e60b) is used as a success background-color in aboutWebauthn.css. We have a --background-color-success variable in https://searchfox.org/mozilla-central/source/toolkit/themes/shared/design-system/tokens-shared.css#17, but that's a very different shade of green.

Hey y'all. So the existing green 50 variable is a rather bright lime green that our branding has moved away from. It'd be totally fine to go with our new token for success, in my personal opinion. Thanks for working this out Anna and for suggesting the token replacement Hanna.

Flags: needinfo?(jules)

(In reply to Jules Simplicio [:jules] from comment #7)

Hey y'all. So the existing green 50 variable is a rather bright lime green that our branding has moved away from. It'd be totally fine to go with our new token for success, in my personal opinion. Thanks for working this out Anna and for suggesting the token replacement Hanna.

Thank you for the reply. But in this case, should we also replace error bacground color here https://searchfox.org/mozilla-central/source/toolkit/components/aboutwebauthn/content/aboutWebauthn.css#39 with --background-color-critical token?

Flags: needinfo?(jules)

That's a great point. I'd probably file a separate bug for that since it's unrelated to this problem we're having. What do you think? But also...I wonder what this UI is and if it could use a message bar instead? ha

Flags: needinfo?(jules)

(In reply to Jules Simplicio [:jules] from comment #9)

That's a great point. I'd probably file a separate bug for that since it's unrelated to this problem we're having. What do you think? But also...I wonder what this UI is and if it could use a message bar instead? ha

That's a great question. I can't eccess about:webauthn page to check that, as it is for Linux and Mac, if I am not mistaken. But it definately looks like some sort of message for a user, which is the main purpose of the message bar)

What do you think, Hanna?

Flags: needinfo?(hjones)

(In reply to Jules Simplicio [:jules] from comment #11)

What do you think, Hanna?

I did a little before/after test by forcing the message to show. I think the color change is definitely an improvement, and we should also get a bug on file to use moz-message-bar there. I imagine this isn't a super high traffic page, but it should be an easy improvement to make :)

Flags: needinfo?(hjones)
Pushed by hjones@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8d490edd8c3
Remove all code associated with the message-bar component r=hjones,desktop-theme-reviewers,reusable-components-reviewers,extension-reviewers,robwu

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

Status: ASSIGNED → RESOLVED
Closed: 25 days ago
status-firefox127: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 127 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: