browser/extensions/newtab/css/activity-stream.css
15343	Why is this part of this patch?
browser/themes/shared/browser-shared.css
335–336	What are these fallback values needed for?
337–338	The shorthand takes an image and a color, so the comma between these two seems redundant? But also, was already set as the background color. Can you just set here and leave the color alone?
toolkit/themes/shared/design-system/dist/tokens-shared.css
1154–1155	Can these just be ?
1154–1155	I suspect we shouldn't use the gradient for either, based on https://github.com/w3c/csswg-drafts/issues/6036#issuecomment-786308318

This revision now requires changes to proceed.Tue, May 26, 5:39 PM

nsharpley requested review of this revision.Fri, May 29, 3:01 PM

nsharpley updated this revision to Diff 1286748.

phab-bot added a reviewer: Dre.Fri, May 29, 3:01 PM

phab-bot removed a reviewer: home-newtab-reviewers-rotation.

phab-bot added a subscriber: home-newtab-reviewers-rotation.

nsharpley marked 5 inline comments as done.Fri, May 29, 3:05 PM

nsharpley added a subscriber: mstriemer.

nsharpley added inline comments.

toolkit/themes/shared/design-system/dist/tokens-shared.css
1154–1155	Hmmm. These updated when I ran so I suspect this is coming from Figma. @mstriemer might know better. I've wrapped the actual style rules that use these values in a media query for not .

sclements mentioned this in D303366: Bug 2042447 - Revert support for translucency with nova enabled r=jswinarton,nsharpley.Fri, May 29, 3:06 PM

Harbormaster completed remote builds in B977603: Diff 1286748.Fri, May 29, 3:26 PM

dao requested changes to this revision.Mon, Jun 1, 9:06 AM

dao added inline comments.

toolkit/themes/shared/design-system/dist/nova/tokens-figma-theme.json
490	Do we need here given that the usage of the gradient is gated by ? If we do want to set this to be safe, should it use instead of to mirror how it's used? Also, would instead of also do the trick? I had already asked about the latter in https://phabricator.services.mozilla.com/D302057#inline-1635642.

This revision now requires changes to proceed.Mon, Jun 1, 9:06 AM

mkennedy removed a reviewer: mkennedy. mkennedy added 1 blocking reviewer(s): dao.Mon, Jun 1, 1:14 PM

nsharpley requested review of this revision.Mon, Jun 1, 7:53 PM

nsharpley updated this revision to Diff 1288664.

nsharpley marked an inline comment as done.

Herald added a reviewer: tabbrowser-reviewers. · View Herald TranscriptMon, Jun 1, 7:53 PM

Herald added a reviewer: urlbar-reviewers. · View Herald Transcript

nsharpley planned changes to this revision.Mon, Jun 1, 7:53 PM

nsharpley removed reviewers: tabbrowser-reviewers, urlbar-reviewers.

Harbormaster failed remote builds in B979013: Diff 1288664!Mon, Jun 1, 7:54 PM

nsharpley added inline comments.Mon, Jun 1, 7:56 PM

browser/themes/shared/browser-shared.css
337–338	Reverted this requested change due to D301786 landing ahead.

nsharpley added a parent revision: D301786: Bug 2034863 - Adjust background for web extension theme compatability r=emilio.Mon, Jun 1, 8:05 PM

nsharpley requested review of this revision.Tue, Jun 2, 2:30 PM

nsharpley updated this revision to Diff 1289482.

nsharpley marked an inline comment as done.Tue, Jun 2, 2:33 PM

nsharpley added inline comments.

toolkit/themes/shared/design-system/dist/nova/tokens-figma-theme.json
490	See below?
toolkit/themes/shared/design-system/dist/tokens-shared.css
1154–1155	@dao Restructured things a bit, so we do use transparent now for . Does this clean things up a bit?

Harbormaster completed remote builds in B979553: Diff 1289482.Tue, Jun 2, 3:11 PM

nsharpley updated this revision to Diff 1290725.Wed, Jun 3, 2:40 PM

nsharpley retitled this revision from Bug 2027859 - Nova toolbar visual updates r=sthompson to Bug 2027859 - Nova toolbar visual updates r=emilio.

nsharpley marked an inline comment as done.

Herald added a subscriber: Itiel. · View Herald TranscriptWed, Jun 3, 2:40 PM

nsharpley edited reviewers, added: emilio; removed: sthompson, Dre.Wed, Jun 3, 2:41 PM

nsharpley added inline comments.

browser/themes/shared/browser-shared.css
238–241	This somehow snuck in on a rebase and messed up the body background. Removing this fixes bug 2044533.

emilio added inline comments.Wed, Jun 3, 2:55 PM

browser/extensions/newtab/css/activity-stream.css
15536	Probably avoid calling it ? or something more nova-specific? Also do we need the / tokens since their only use is the gradient and there's only one reference?
browser/themes/shared/browser-shared.css
282	Maybe reword a bit? Something like: Toolbox background images go either on the `<body>` or on the toolbox itself. For most themes and pre-nova default theme, it goes on the `<body>`. For themes that align the image in the y axis, and the nova default theme, they go on the toolbox. FIXME(bug XXX): Clean this up by using a more specific attribute or so. Please file bug XXX to do so and ni? me, I can take a look :)
toolkit/themes/shared/design-system/src/tokens/components/toolbox.nova.tokens.json
14	Do you need this tho? here will be completely unused.
toolkit/themes/shared/design-system/src/tokens/components/toolbox.tokens.json
15	This should either be or or so, so it is also an image. But also maybe you can remove it and just not set if ?

Harbormaster completed remote builds in B980490: Diff 1290725.Wed, Jun 3, 3:11 PM

emilio requested changes to this revision.Wed, Jun 3, 3:15 PM

Comment Actions

Tagging request changes so that I know I need to take another look as soon as it's back on the review queue :)

This revision now requires changes to proceed.Wed, Jun 3, 3:15 PM

nsharpley requested review of this revision.Wed, Jun 3, 3:29 PM

nsharpley updated this revision to Diff 1290789.

nsharpley marked 3 inline comments as done.

nsharpley marked an inline comment as done.Wed, Jun 3, 3:29 PM

nsharpley added inline comments.

browser/extensions/newtab/css/activity-stream.css
15536	Also do we need the -leading / -trailing tokens since their only use is the gradient and there's only one reference? We have to keep these variables, named the same, as they are pulled in from Figma. That way if colors are changed in Figma they get updated here too :)
toolkit/themes/shared/design-system/src/tokens/components/toolbox.nova.tokens.json
14	This is imported from Figma. I'll let the Acorn folks know we maybe don't need it and they can update their end / pull those changes in later.

emilio accepted this revision.Wed, Jun 3, 3:42 PM

emilio added a project: testing-exception-ui (Only changes ui styling, images, or localized strings).

Comment Actions

r=me with those fixes, can look at the follow-up once this is on main. See below, the default gradient variable seems easy to misuse.

browser/extensions/newtab/content-src/styles/nova/_tokens.scss
303	If we could call this or or so it'd be good, to make sure it's not misused.
browser/themes/shared/browser-shared.css
571	I don't think you need this anymore (\o/)
576	too please

nsharpley marked 3 inline comments as done.Wed, Jun 3, 3:45 PM

nsharpley added inline comments.

browser/extensions/newtab/content-src/styles/nova/_tokens.scss

303

Hmmm. I'd need to do this in the tokens json, but the script trims "nova" so it doesn't work 😞

"nova-nova-gradient": {
 "comment": "This gradient is only used for Nova",
 "value": {
 "default": "linear-gradient(96deg, color-mix({toolbox.background.color.gradient-leading} 50%, transparent) 39.84%, color-mix({toolbox.background.color.gradient-trailing} 50%, transparent) 101.72%)",
 "forcedColors": "image(transparent)"
 }
 }

And I know we are steering away from using "default"

browser/themes/shared/browser-shared.css

571

🎉

nsharpley updated this revision to Diff 1290828.Wed, Jun 3, 3:46 PM

nsharpley marked 2 inline comments as done.

nsharpley removed a reviewer: dao.Wed, Jun 3, 3:59 PM

This revision is now accepted and ready to land.Wed, Jun 3, 3:59 PM

nsharpley added a comment.Wed, Jun 3, 3:59 PM

Comment Actions

Removing Dao as a reviewer since feedback has been addressed and Emilio has approved for desktop theme reviewers.

Harbormaster failed remote builds in B980536: Diff 1290789!Wed, Jun 3, 4:02 PM

This revision was landed with ongoing or failed builds.Wed, Jun 3, 4:12 PM

Closed by commit rFIREFOXAUTOLANDae933c480f42: Bug 2027859 - Nova toolbar visual updates r=desktop-theme-reviewers,emilio (authored by nsharpley). · Explain Why

This revision was automatically updated to reflect the committed changes.

nsharpley added a commit: rFIREFOXAUTOLANDae933c480f42: Bug 2027859 - Nova toolbar visual updates r=desktop-theme-reviewers,emilio.

Harbormaster completed remote builds in B980552: Diff 1290828.Wed, Jun 3, 4:16 PM

nsharpley mentioned this in rFIREFOXBETAae933c480f42: Bug 2027859 - Nova toolbar visual updates r=desktop-theme-reviewers,emilio.Mon, Jun 15, 12:33 PM

Revision Contents
Changeset List

Path

Size

browser/

extensions/

newtab/

content-src/

styles/

nova/

_tokens.scss

12 lines

css/

activity-stream.css

9 lines

nova/

activity-stream.css

9 lines

themes/

shared/

browser-shared.css

73 lines

tabbrowser/

tab.tokens.css

1 line

urlbar/

urlbarview.tokens.css

1 line

toolkit/

content/

widgets/

moz-badge/

moz-badge.tokens.css

1 line

themes/

shared/

design-system/

dist/

nova/

tokens-figma-theme.json

10 lines

tokens-figma-theme.json

5 lines

tokens-shared.css

14 lines

src/

tokens/

components/

toolbox.nova.tokens.json

14 lines

toolbox.tokens.json

15 lines

Commit	Tree	Parents	Author	Summary	Date
ae933c480f42	25c0c948f67a	Nikki Sharpley	Bug 2027859 - Nova toolbar visual updates r=desktop-theme-reviewers,emilio (Show More…)

	Status	Author	Revision
Closed	nsharpley	D302057 Bug 2027859 - Nova toolbar visual updates r=emilio
Closed	nsharpley	D301786 Bug 2034863 - Adjust background for web extension theme compatability r=emilio