The solution we came up with was to include both images when required and render both if they are available (one could have a .light- another a .dark- selector). We can then use CSS attribute selectors lwt-newtab-brighttext to conditionally hide the one we don't want to show.

Hey Andrei - can you expand on the problem this bug is designed to solve? And what the current behavior is? (with screenshots)

Trying to get a sense of if we need UX advice on this, and if it needs to be fixed and uplifted into Beta 66.

Ah! Andrei is out this week. Kate, can you chime in?

The snippet theme will change to match Activity Stream colors. Currently we do not have a way to update the images that come with a snippet message. For example a logo inside a snippet message might need a dark and a light variant.

Hey. Is the proposed solution (including both images in the payload and having AS selecting the correct one) possible on your end as well? How would the payload look like? We could move forward on this before the endpoint is updated.

A few questions about this:

• Which templates are going to be affected by this change?
• And also which images in the templates?

About the implementation, yes we could extended the schemas to add "-dark-theme" variables for images. I guess you could code it in a way to allow all variables of type "image" to potentially have a "<name>-dark-theme" variable in the payload and use that when needed.

The AS code must be able to default to the original variable if the "-dark-theme" variable is not present.

From the snippets-server side I expect this to get implemented after we move away from using base64 for the images, or we'll end up doubling the side of the payload with the known indexdb consequences.

(In reply to Giorgos Logiotatidis [:giorgos] from comment #5)

A few questions about this:

• Which templates are going to be affected by this change?

All templates extend some base snippet implementation. This is where we would check if an alternative -dark-theme variant is present and if dark theme is on. In other words we show the dark theme variant image every time it's present and we're in dark theme mode for all templates.

• And also which images in the templates?

I think this would be up to the person adding the snippet content and doing verifying.

The AS code must be able to default to the original variable if the "-dark-theme" variable is not present.

Yes.

From the snippets-server side I expect this to get implemented after we move away from using base64 for the images, or we'll end up doubling the side of the payload with the known indexdb consequences.

Yes this sounds best.

(In reply to Andrei Oprea [:andreio] from comment #6)

So tl;dr the code

• will support dark variants for all images
• will default to the light variant if dark is not present. Note that for a snippet with multiple images only some may have dark variants

@andreio please make sure to update the schemas and your tests.

Thanks!

Also please fix https://bugzilla.mozilla.org/show_bug.cgi?id=1510335 otherwise we won't be able to preview and thus we won't take advantage of this.

I'll go ahead and mark 1510335 a blocker.

Thanks!

(In reply to Andrei Oprea [:andreio] from comment #6)

From the snippets-server side I expect this to get implemented after we move away from using base64 for the images, or we'll end up doubling the side of the payload with the known indexdb consequences.

Yes this sounds best.

The snippets-service has moved away from using base64 images. The payloads are now much smaller and we can easily support this change.

Could we have an ETA on this to plan snippets work accordingly?

I've got a PR up, it can be reviewed and landed ASAP.
Can we confirm if the variant name *_dark_theme is ok? (example of expected payload) this can easily be adjusted if we decide for something else we just need to decide beforehand.
Which fields are expected to have a dark variant? icon, title_icon, scene1_title_icon, scene2_icon and section_title_icon any others? If the dark variant doesn't exist we just fallback to light but we need to know which to check.

Just to clarify: the *_dark_theme variant will be loaded by default if dark theme is enabled, no other changes are required.

Also scene1_icon

*_dark_theme sounds ok.

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

I have verified this issue with the latest Firefox Nightly (68.0a1 Build ID - 20190520095418) installed, on Windows 10 x64, Arch Linux and Mac 10.14.4. Now, the "icon-dark-theme" is displayed if the browser's "Dark" theme is active and the "icon-light-theme" variant is displayed if the "Light" theme is active.