1770534 - Only use the zap gradient on the setup card and colourway closet expiry blurb in Firefox View with default themes

Status: RESOLVED FIXED

(Firefox :: Firefox View, defect, P2)

Description

[Sam Foster [:sfoster] (he/him)]

The "zap-gradient" (which gets added to the sync setup card in bug 1770499 - its the purple -> pink -> orange gradient border) clashes terribly with a lot of non-built-in themes. We use a check for a :-moz-lwtheme pseudo to fall back to a normal solid color in places like the separator under the FxA main menu item. AFAICT this approach is not currently available for in-content stylesheets & elements however, so we need another solution.

Comment 1

[Sarah Clements [:sclements]]

Dao, Yasmin mentioned that colorways might have hit this same problem. If so, did you come up with a solution?

Comment 2

[Dão Gottwald [:dao]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #0)

We use a check for a :-moz-lwtheme pseudo to fall back to a normal solid color in places like the separator under the FxA main menu item. AFAICT this approach is not currently available for in-content stylesheets & elements however, so we need another solution.

You can use the lwt-newtab attribute.

(In reply to Sarah Clements [:sclements]|away till 30th from comment #1)

Dao, Yasmin mentioned that colorways might have hit this same problem. If so, did you come up with a solution?

No, the colorways modal only supports light and dark mode where the gradient sort of works, and it also only uses it for the expiry pill where the border isn't hugely important in terms of marking a section (e.g. a card) that needs visual separation from the rest of the page.

Comment 3

[:Gijs (he/him)]

Morphing this to include the same issue for the other use of the zap, namely the colourway collection expiry blob, as noted in bug 1788274.

Comment 4

[Dão Gottwald [:dao]]

Not a performance issue.

Comment 5

[Sarah Clements [:sclements]]

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

(In reply to Sam Foster [:sfoster] (he/him) from comment #0)

We use a check for a :-moz-lwtheme pseudo to fall back to a normal solid color in places like the separator under the FxA main menu item. AFAICT this approach is not currently available for in-content stylesheets & elements however, so we need another solution.

You can use the lwt-newtab attribute.

That doesn't work. All of the in-content themes have the lwt-newtab attribute on body thanks to the contentTheme.js script.

Emilio, I see that the moz-lwtheme selector was removed from in-content pages last year in bug 1740230. Would it be possible to get that selector to work in in-content pages and give those pages access to it again?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I removed it from content but it never matched before, so it's not particularly simple. Also, do you really want moz-lwtheme? It seems to me that'd match on the default light and dark themes, which probably is not what you want for comment 0?

Comment 7

[Shane Hughes [:aminomancer]]

Maybe we should just add an attribute in contentTheme.js that reflects the theme's computed frame saturation. Since the zap doesn't necessarily look bad on all custom themes, it mostly seems to look bad on colorways and other themes that have very bold colors. Something like :is(:root, body)[lwt-newtab-vibrant] {...}

Otherwise, aren't we using theme experiment properties to control the appearance of the zap gradient in the chrome now? I think it should be possible to add that to the content styles with contentTheme.js.

Comment 8

[Sarah Clements [:sclements]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

I removed it from content but it never matched before, so it's not particularly simple. Also, do you really want moz-lwtheme? It seems to me that'd match on the default light and dark themes, which probably is not what you want for comment 0?

Well, we want to have the zap gradient for all Mozilla themes - default, light, dark, alphenglow and colorways. But not the third-party/user created themes (since we have no idea if the clashing would occur).

Maybe we should just add an attribute in contentTheme.js that reflects the theme's computed frame saturation. Since the zap doesn't necessarily look bad on all custom themes, it mostly seems to look bad on colorways and other themes that have very bold colors. Something like :is(:root, body)[lwt-newtab-vibrant] {...}

I don't think we want to start tweaking the colors like that for colorways (if we ever needed to, maybe the zap_gradient property at the manifest level is the way to go).

Otherwise, aren't we using theme experiment properties to control the appearance of the zap gradient in the chrome now? I think it should be possible to add that to the content styles with contentTheme.js.

This would work. The default-theme, light, dark and alphenglow theme manifests all have a zap_gradient property that's exactly the one we're using for Firefox View but colorways themes do not. So I could modify contentTheme.js to utilize that zap_gradient property if it exists and if it doesn't, then use the lwt-newtab attr in order to add the zap-gradient for colorways themes, leaving third party themes without it (just a default border color).

Comment 9

[Shane Hughes [:aminomancer]]

(In reply to Sarah Clements [:sclements] from comment #8)

This would work. The default-theme, light, dark and alphenglow theme manifests all have a zap_gradient property that's exactly the one we're using for Firefox View but colorways themes do not. So I could modify contentTheme.js to utilize that zap_gradient property if it exists and if it doesn't, then use the lwt-newtab attr in order to add the zap-gradient for colorways themes, leaving third party themes without it (just a default border color).

Hi Sarah, I actually submitted a patch to expose that property to Fx View. Let me know if this works for you, thanks!

Comment 10

[Dão Gottwald [:dao]]

(In reply to Shane Hughes [:aminomancer] from comment #9)

(In reply to Sarah Clements [:sclements] from comment #8)

This would work. The default-theme, light, dark and alphenglow theme manifests all have a zap_gradient property that's exactly the one we're using for Firefox View but colorways themes do not. So I could modify contentTheme.js to utilize that zap_gradient property if it exists and if it doesn't, then use the lwt-newtab attr in order to add the zap-gradient for colorways themes, leaving third party themes without it (just a default border color).

Hi Sarah, I actually submitted a patch to expose that property to Fx View. Let me know if this works for you, thanks!

From what I can tell, that completely fixed this bug.

Comment 11

[Sarah Clements [:sclements]]

(In reply to Shane Hughes [:aminomancer] from comment #9)

(In reply to Sarah Clements [:sclements] from comment #8)

This would work. The default-theme, light, dark and alphenglow theme manifests all have a zap_gradient property that's exactly the one we're using for Firefox View but colorways themes do not. So I could modify contentTheme.js to utilize that zap_gradient property if it exists and if it doesn't, then use the lwt-newtab attr in order to add the zap-gradient for colorways themes, leaving third party themes without it (just a default border color).

Hi Sarah, I actually submitted a patch to expose that property to Fx View. Let me know if this works for you, thanks!

Ok, thanks but... I'm confused why you submitted a patch when I was assigned to this bug?

Edit: I see that was a patch for bug 1792736, but included also changes to Firefox View...

Comment 12

[Shane Hughes [:aminomancer]]

(In reply to Sarah Clements [:sclements] from comment #11)

Ok, thanks but... I'm confused why you submitted a patch when I was assigned to this bug?

Edit: I see that was a patch for bug 1792736, but included also changes to Firefox View...

Sorry about that, it was just supposed to illustrate my previous suggestion for your feedback. I think Dão thought it was just part of the needed changes, so went ahead and landed it.