Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.43k stars 1.99k forks source link

Livro: New Post Email Link Color is grey #70360

Open devNigel opened 1 year ago

devNigel commented 1 year ago

Quick summary

With Livro theme activated, both in AT and simple sites, the link color is always grey despite the settings in Global Styles.

This probably also happens with Lettre, they're the two themes that have accent colour slugs defined (see wpcom_get_site_accent_color_palette_item_slug).

Steps to reproduce

  1. Activate Livro on an AT or Simple site.
  2. Go to Appearance > Editor > Global Styles and change Link color to anything other than grey.
  3. Subscriber to the blog by email.
  4. Publish a new post with an hyperlink.
  5. Check the email notification.

What you expected to happen

The link color should be what is set on the Global Styles.

What actually happened

The link color was grey despite having a different color in the Global Style settings.

image image

Context

User report: 5737636-zd-woothemes

Platform (Simple, Atomic, or both?)

Simple, Atomic

Theme-specific issue?

Livro + Lettre

Browser, operating system and other notes

No response

Reproducibility

Consistent

Severity

All

Available workarounds?

No but the platform is still usable

Workaround details

No response

github-actions[bot] commented 1 year ago

Support References

This comment is automatically generated. Please do not edit it.

kavyagokul commented 1 year ago

📌 SCRUBBING : RESULT

📌 FINDINGS/SCREENSHOTS/VIDEO

📌 ACTIONS

arinoch commented 1 year ago

User replied indicating this should be considered an a11y issue for people, the user specifically mentioned elderly, with vision difficulties. I've labeled the issue accordingly.

Copons commented 1 year ago

Adding this to the T-Rex backlog as per triage, but we won't get to it before next year. cc-ing @Automattic/theam if they have bandwidth in the meantime

dsas commented 1 year ago

Colours are picked out by Wpcom_Notifications_Theme_Helper and will always get the primary site colour (as the -ACCENT_COLOR variable). This is done via this stacktrace:

Wpcom_Notifications_Theme_Helper::create_html_email_args Subscription_Mailer->get_post_as_html Subscription_Mailer->get_posts Subscription_Mailer->send_post

It looks as though it was introduced via this diff: D86747-code

Options to address:

  1. Remove automatic colour customisation - probably make too many people sad, particularly with lettre and newsletters
  2. Make the emails really customisable as per popular jetpack request - too much work & too many unknowns for a bugfix
  3. Apply the theme background colour to the whole email
  4. Make it cleverer - try link colour, then primary colour then a known good colour. Fall down the chain if the colour isn't set or doesn't provide enough contrast on a white background.
dsas commented 1 year ago

The link colour can be retrieved by wp_get_global_styles(array('elements', 'link', 'color','text')), if the user hasn't changed the link colour then it will probably be something like var(--wp--preset--color--foreground), if they have customised it then it might be a value or a variable, depending on whether it was saved after this PR.

The background colour is presumably something similar

dsas commented 1 year ago

@Automattic/apex Looks like you worked on this originally. Interested to hear if you have any thoughts on the best approach here? I'm leaning towards the fourth option.

ivan-ottinger commented 1 year ago

@Automattic/apex Looks like you worked on this originally. Interested to hear if you have any thoughts on the best approach here? I'm leaning towards the fourth option.

Option four sounds best to me as well. I think that the Link color set in the Global Styles could be used if it was customized by the user, otherwise, the accent color could be used instead (the current behavior).

I would rule out the option one - as it would basically put us back where we were - in the other direction where we wanted to get with newsletters.

Pinging @mashikag if you would like to add your point of view. 🙂

dsas commented 1 year ago

I ran out of time to actually implement this, but here's what I've found.

The accent colour is used mainly for links, but also for blockquote borders and a couple of other things. Arguably we could introduce a new variable for link colour in addition to accent colour, but I think it will look visually better to just make everything use a consistent colour instead i.e. just set a different accent color value.

There's some really helpful email testing guides on PdDOJh-9q-p2

Finding contrast

Jetpack_Colour can help determine sufficient contrast. The email is always on a white background.

Available colours

The link colour is stored in wp_get_global_styles(['elements', 'link', 'color', 'text']); The format of actually depends on where the color is chosen from.

Default colour

Will be a value like var(--wp--preset--color--primary), but wpcom_get_site_accent_color_palette_item_slug has the values for each theme. Given the theme stylesheet isn't loaded by the notification then this css variable needs converting to a value.

I think this converting is the same as Palette colour below, except for how you get the slug. Currently it's always in the theme part of the array and never in the default part, which means wpcom_get_global_theme_color_palette_item($slug)['color']; will do the trick.

Currently the 'default' colour is always used - setting an accent colour changes the default colour in the palette (see wpcom-site-accent-color).

Custom colour

Will just be a value like #abcdef that can be used directly

Palette colour

Will be a value like var:preset|color|vivid-red.

The colour slug vivid-red can be turned into a hex color by looking them up from global styles. This can be done by:

  1. Use wp_get_global_settings(['color', 'palette']) to get arrays of palette colour sources (e.g. for theme and default)
  2. Iterate over each of these sources, they contain arrays of colours. The array that has a slug key which matches the last part of the value (e.g. vivid-red will also have a color key which is the hex value
$needle = 'vivid-red';
foreach (wp_get_global_settings(['color', 'palette']) as $source ) {
    foreach ($source as $color) {
        if ($needle === $color['slug']) {
            $color_value = $color['color'];
            break 2;
        }
    }
}

Add a function to wpcom-global-styles-color-palette to do something like that.

dsas commented 1 year ago

I've ran out of time on my maintenance rotation to implement this :(

Marvel will be very short-staffed for the foreseeable so I don't think we'll get around to this normal priority issue anytime soon. I think it's really a better fit for Apex's usual areas of expertise, so if @Automattic/apex could pick this up and prioritise appropriately that'd be great.

ktyfuller604 commented 1 year ago

The original reporter of this issue has returned for an update. I've suggested switching to Erma after testing how the new subscriber email appears in that theme, but this may not be accepted as a solution.

mashikag commented 1 year ago

This issue should be fixed now: D119415-code