Details

Reviewers

karlt
whimboo
tgiles
dao

Group Reviewers

webdriver-reviewers
desktop-theme-reviewers
media-playback-reviewers
devtools-reviewers

Commits

rMOZILLACENTRAL598874fd4b8e: Bug 1845150 - Use moz-message-bar instead of message-bar in notificationbox.js…
rMOZILLACENTRAL683745289588: Bug 1845150 - Use moz-message-bar instead of message-bar in notificationbox.js…
rMOZILLACENTRAL7477fd1c48fa: Bug 1845150 - Use moz-message-bar instead of message-bar in notificationbox.js…

Bugzilla Bug ID

1845150

Summary

This patch updates the NotificationMessage element in notificationbox.js so that it extends our newer moz-message-bar component instead of the deprecated message-bar component. Many of the changes are just dealing with the implications of making things async (so that we can ensure moz-message-bar.mjs gets imported). I tried to break out places where I modified related code and tests into separate patches to mitigate some of the review pain here.

This patch solves a longstanding issue where we were loading in-content/common-shared.css in the chrome since it gets used by the message-bar element. It also makes some small visual changes to our infobars (slight outline, icon colors, adds a bit of spacing).

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Build Status

Buildable 615005
Build 713221: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

The reusable component changes look good to me, just have one question and a nit. Thanks for putting all this together, great to see so much code get reduced/simplified!

toolkit/content/widgets/infobar.css
78–79	Why do we need to use `!important` here? Is this to deal with XUL links?
toolkit/content/widgets/message-bar.css
126	I think we need to use `margin-block: 4px` and `margin-inline: 8px` here since the margin shorthand maps to the margin physical properties instead of the logical properties as far as I understand it.

This revision now requires changes to proceed.Dec 5 2023, 10:13 PM

Itiel added a subscriber: Itiel.Dec 5 2023, 10:31 PM

Itiel added inline comments.

toolkit/content/widgets/message-bar.css
126	This would have been relevant if any of the Firefox UI supported vertical reading mode. Shorthand usecases like this is fine also for RTL, as long as you stick to 1-3 values and not 4 (because e.g. `1px 2px 3px 4px` isn't RTL friendly (and `1px 2px 1px 2px` would be equivalent to `1px 2px` anyway), `1px 2px 3px` would mean the margin on both inline sides would be 2px, etc).

flod removed a subscriber: flod.Dec 6 2023, 6:23 AM

hjones requested review of this revision.Dec 6 2023, 4:15 PM

hjones marked 3 inline comments as done.

hjones added inline comments.

toolkit/content/widgets/infobar.css
78–79	Yeah I think this is kind of an edge case because the vast majority of links are support links, but you can check it out with a snippet like this: let buttons = [ { label: "Link 1", link: "https://www.mozilla.org" }, { label: "Button 2", callback: () => console.log("hey"), } ] gNotificationBox.appendNotification( "testing", { label: { "l10n-id": "quickactions-logins2" }, }, buttons ); Unfortunately with the `::slotted` selector any global styles still take precedence without the `!important`. I think this is the relevant part of the spec that explains why: https://drafts.csswg.org/css-cascade-4/#cascade-context Basically it comes down to cascade order rather than specificity, and `!important` is kind of our only option.

dao requested changes to this revision.Dec 6 2023, 4:42 PM

dao added a subscriber: dao.

dao added inline comments.

toolkit/content/widgets/notificationbox.js
630	`MozMessageBar` adds the `El` suffix to all element properties; we should probably be consistent one way or another? Fwiw, I prefer the pattern without that suffix but I suppose that ship may have sailed?
653	ditto :\|
791	Hmm, this seems like a misuse of the `footer-button` class? Should we rename the class? Could be a followup.
toolkit/themes/shared/global-shared.css
238	Setting the line-height to a px value seems a bit messy. Instead of doing that, could we just reduce the vertical padding?
280	Can you move this into the `&.small-button` rule as `&xul\|button` (if it's still needed after addressing my previous comment)?

This revision now requires changes to proceed.Dec 6 2023, 4:42 PM

dao mentioned this in D189004: Bug 1825032 - How to guide for design tokens r?#reusable-components-reviewers,#desktop-theme-reviewers,morgan.Dec 6 2023, 5:51 PM

rebase, address review comments

Harbormaster completed remote builds in B605175: Diff 796765.Dec 7 2023, 9:20 PM

hjones added inline comments.Dec 7 2023, 9:21 PM

toolkit/content/widgets/notificationbox.js
630	Chatted with other recomp folks about this - I believe we initially introduced that `El` suffix for cases where we had potential property naming collisions between data and elements (like in moz-toggle where we pass in `label` data but also wanted to expose the internal `label` element for testing). It seems alright not to enforce this as a convention, especially in cases like this where the names are already pretty clear. @tgiles filed a bug to document this. Also happy to discuss more though!
791	Agreed this is a pretty awkward fit and could probably use a rename. This is kind of meant to be a temporary solution / intended to be replaced by `moz-button` once the component is available. Alternatively we might want to create a single source of truth in the CSS for these kinds of buttons, along the lines of what's proposed here: https://bugzilla.mozilla.org/show_bug.cgi?id=1858814
toolkit/themes/shared/global-shared.css
238	Removed the line-height hack and it didn't seem to change the vertical alignment at all 🤔 Unless I'm missing something we might be alright just setting `min-height`...

Thanks for all the work on this! toolkit/content changes look good to me, as well as the tokens changes. I'll defer to Dao on the final say on the tokens and global-shared changes though.

toolkit/content/widgets/infobar.css
6	@hjones do you think it's worth filing a good-first-bug to add some spaces between the `rgb` values here?

dao accepted this revision.Dec 8 2023, 10:44 AM

dao added inline comments.

toolkit/content/widgets/infobar.css
90–91	Should probably use `var(--font-weight-bold)` here
toolkit/content/widgets/moz-message-bar/moz-message-bar.css
198–200	Nest this in the `:host([type=error]), :host([type=critical])` rule? That should work I think now that bug 1850974 is fixed. The other two `.icon` rules above this should probably be nested too for consistency. Looking through this file, there are more opportunities for nesting, so perhaps just file a followup on that...
toolkit/content/widgets/notificationbox.js
630	Sounds good, but it seems like some of `MozMessageBar`'s property names should be revisited then.
toolkit/themes/shared/global-shared.css
228	update this to using `var(--size-item-large)` while you're at it?

This revision is now accepted and ready to land.Dec 8 2023, 10:44 AM

hjones updated this revision to Diff 797387.Dec 8 2023, 10:24 PM

hjones marked 7 inline comments as done.

Harbormaster completed remote builds in B605739: Diff 797387.Dec 8 2023, 10:24 PM

hjones added inline comments.Dec 8 2023, 10:24 PM

toolkit/content/widgets/infobar.css
6	It probably would be, but now that I've seen it I can't not fix it hah 😂
toolkit/content/widgets/moz-message-bar/moz-message-bar.css
198–200	Nice! I was confused as to why nesting wasn't working with `:host` rules. I fixed this and also nested some rules in `infobar.css`. Was going to file a follow-up for additional nesting, but the only rules I see are the ones related to ghost buttons which will get removed when D187931 lands.
toolkit/content/widgets/notificationbox.js
630	Good call, I filed Bug 1869065 which should be a simple bit of cleanup.
toolkit/themes/shared/global-shared.css
238	I found the case where things looked off - it seems if we create and HTML button rather than a XUL button things get wonky: I ended up adding more padding and specifying a font-size for the `.small-button` class and it looks alright now.