Reader mode: Generate styles for color palette, gradient presets, and font sizes from the theme support features of the primary theme #6042
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #6042 +/- ##
=============================================
+ Coverage 74.89% 75.03% +0.13%
- Complexity 5775 5829 +54
=============================================
Files 231 232 +1
Lines 17491 17618 +127
=============================================
+ Hits 13100 13219 +119
- Misses 4391 4399 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
I've also tried creating a cover block in Twenty Twenty and giving its background color the accent color:
The color chosen for Twenty Twenty-One doesn't seem right? I then made Twenty Twenty-One the active theme, and I opened the editor and I was surprised to see that the Cover block prompted me to select a new color. Without doing so, I and looking at the non-AMP frontend, I then saw the same dark background color as above as when Twenty Twenty-One was the Reader theme and Twenty Twenty was the active theme. In Legacy AMP, no background color is shown at all, whereas when Twenty Twenty is the Reader theme, the pink background shows up again as expected:
The black background in Twenty Twenty-One is coming from the default style rule for the Cover block, whereas in Twenty Twenty there is a style rule for
If I go ahead and make Twenty Twenty-One the active theme and set the new background color according to the theme, the results are as follows:
(Note the Legacy AMP text color is white when it should be black.)
This is a challenge! The lack of standardization across themes poses a key problem. This problem is acknowledged even in the block editor after switching a theme, since the user has to re-pick a theme color if the new theme lacks the same color name from the previous theme. |
|
The issue is not limited to the background color either. If I select one theme color “Red” for the background and another theme color “Dark Gray” as the text color, in a Reader theme I get no styling:
The cover block has no styling because Twenty Twenty has no style rules for |
|
I have what may be an acceptable fallback for the Reader theme case in 2347340, where if a Reader theme is being served it will give Cover blocks a default background color of black and text color of white:
|
westonruter
force-pushed
the
fix/5229-theme-color-not-avail-in-legacy-reader-template
branch
from
2347340
to
a500206
Compare
|
I have an idea for how to solve the Reader theme problem. We can store the primary theme's |
…fault text color for an assigned background color
|
@pierlon Here's what it looks like now with my changes:
Two observations:
|
…theme-color-not-avail-in-legacy-reader-template * 'develop' of github.com:ampproject/amp-wp: (35 commits) Bump `codecov/codecov-action` to v1.4.1 Fix chevron for "Context" column not expanding Ensure the `source` class is always used Remove legacy styles for single error detail summary Extract details column to separate SCSS partial Extract error details toggle button to separate SCSS partial Position stylesheets expand arrow correctly and clean up CSS Keep DETAILS closed initially so that expand arrows stay in sync Remove obsolete Icon::get_color() Preserve hover color for row-title Use SCSS stylelint preset and fix SCSS lint issues Convert `amp-icons` to SCSS and use alt icons in admin bar Ensure expand arrow is positioned relative to stylesheet details button Ensure rows with unreviewed errors are styled on AMP Validated URLs page Reverse boldness of new-vs-reviewed error rows in block editor Fix stylelint error Ensure SUMMARY marker is rendered correctly on Safari Prevent minor bottom border gap in error details cell Revers boldness of new-vs-reviewed error rows Be more specific in jsdoc param type definition ...
…theme-color-not-avail-in-legacy-reader-template * 'develop' of github.com:ampproject/amp-wp: Make expression reduction clearer Ignore directories in CSS source directory Map expression in single pass Add rel="nofollow noreferrer noopener" to amp-wp.org link in form submission message Define a logger for WP CLI in PHPUnit bootstrap Scope CSS entries to current Webpack compilation Add Webpack plugin to ignore JS and PHP asset files when stylesheet used as entrypoint Bump @wordpress/scripts from 14.0.1 to 14.1.0 Bump @wordpress/escape-html from 1.12.0 to 1.12.1 Bump @wordpress/is-shallow-equal from 3.1.0 to 3.1.1 Bump @wordpress/autop from 2.12.0 to 2.12.1 Bump @wordpress/data from 4.27.0 to 4.27.1 Bump @wordpress/compose from 3.25.0 to 3.25.1
|
Plugin builds for 8462653 are ready 🛎️!
|
westonruter
changed the title
src/ReaderThemeSupportFeatures.php
Outdated
| private function get_theme_support_features() { | ||
| $features = []; | ||
| foreach ( self::SUPPORTED_FEATURES as $feature_key ) { | ||
| $features[ $feature_key ] = current( (array) get_theme_support( $feature_key ) ); |
There was a problem hiding this comment.
We can limit the keys that we store to only include slug, size, color, and gradient.
src/ReaderThemeSupportFeatures.php
Outdated
| } | ||
| foreach ( $theme_support_features as $support => $feature ) { | ||
| if ( is_array( $feature ) ) { | ||
| add_theme_support( $support, $feature ); // @todo Problem: $feature is now a subset. |
There was a problem hiding this comment.
I thought this was a good idea at first, but now it may be problematic. Consider the case where a user has Twenty Thirteen as the primary theme and they select Twenty Twenty-One as the Reader theme. The TT1 theme actually reads from the editor-color-palette to populate a Customizer control for the background color, and the result here is that the color palette from Twenty Thirteen is showing up in the Customizer for Twenty Twenty-One:
On one hand that seems kinda cool, but it seems eventually it could cause problems, especially here in the case where I've reduced the theme support props to omit anything but the slug and the other prop that is needed to generate the styles. It could be that a theme wants to use the color values in the Customizer or some other UI as well, and they'd be missing here.
I think we should consider not adding the theme support features from the primary theme. They're not actually used in the generation of the page styles since they're (currently) defined in the theme's style.css. But that could change in the future. There's nothing stopping someone from trying to be be more DRY and configurable and only define the color palette in PHP to then render out the style rules like we're doing here.
The bottom line is that we are printing out the styles from the primary theme which are needed for rendering the blocks. Actually copying the theme support features from the primary theme would extend the scope to beyond the block styles to then affect other aspects of the theme design. As such we can revisit that later.
src/ReaderThemeSupportFeatures.php
Outdated
| */ | ||
| public function register() { | ||
| add_filter( 'amp_options_updating', [ $this, 'filter_amp_options_updating' ] ); | ||
| add_action( 'after_switch_theme', [ $this, 'update_cached_theme_support' ] ); |
There was a problem hiding this comment.
This doesn't work when switching the theme with WP-CLI.
There was a problem hiding this comment.
Note that when you switch a theme with WP-CLI, the after_switch_theme action will only fire the next time that a user accesses the site.
…idden at after_switch_theme
…theme-color-not-avail-in-legacy-reader-template * 'develop' of github.com:ampproject/amp-wp: Improve JS test coverage Remove code incorrectly added in merge conflict resolution Pass missing backend props to settings page JS app Improve the notice microcopy Use info notice and toggle for showing unavailable reader themes Ensure carousel nav state gets updated when items count changes Add unit test to increase coverage Fix indentation issues Hide carousel nav if there is just one page Remove unused namespace imports Remove unused namespace import Categorize themes by availability in ReaderThemes context provider
There was a problem hiding this comment.
@westonruter @pierlon I've reviewed the code and tested it out locally. Everything seems to be working as expected.
westonruter
deleted the
fix/5229-theme-color-not-avail-in-legacy-reader-template
branch
Summary
Fixes #5229
There are various theme support features that impact how blocks are rendered on the frontend, including:
editor-color-palette: “A default set of colors is provided, but themes can register their own and optionally lock users into picking from the defined palette.”editor-gradient-presets: Default set of gradients for block backgrounds, e.g. Cover block.editor-font-sizes: “A default set of sizes is provided, but themes can register their own and optionally lock users into picking from preselected sizes.”When editing content in the block editor and you select one of the colors from the theme's color palette, the slug of that color is used in a class name on the block. In order for the color to then show up, the stylesheet on the page must have a style rule for that color. This was broken for AMP pages in in Reader mode:
The issue is the same for the theme's font sizes and gradient presets: the style rules are defined in the primary theme's stylesheet which is not loaded in Reader mode.
This issue is fixed by ensuring that the required styles are included in Reader mode:
The cache of the primary theme's theme support features are updated whenever:
Demo 1
With the Twenty Twenty theme active and the following post content:
Here's the before and after with the changes from this PR applied:
Demo 2
Given this block markup with the Twenty Twenty-One theme active:
Checklist