Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make duotone support compatible with enhanced pagination #55415

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

luisherranz
Copy link
Member

What?

Partial fix for #55230.

This fix makes the duotone compatible with the enhanced pagination by making sure that the CSS is always on the page, even when the posts have no featured image. It also prevents the duotone from interfering with other blocks using wp_unique_id.

Why?

With the current implementation of enhanced pagination, we are still not detecting the CSS corresponding to each block. Therefore, for the enhanced pagination to work correctly, the CSS of the blocks present in the Post Template must be stable on all pages.

The featured image block can contain a duotone, but the duotone CSS is only inlined when the block is not empty. As posts can have or not have a featured image, if all posts on one page do not have a featured image, the duotone CSS will not load, and when you paginate, the CSS will not be applied to the posts that do have a featured image.

In addition, some duotone settings use the wp_unique_id, and as it is a conditional use, this can break the stability of the rest of the blocks that use wp_unique_id causing another series of problems like those described in #55230.

How?

Simply by making the duotone CSS render even when the block is empty. This seems to be the normal behavior of other types of block supports, because the duotone, according to my tests, is the only one that has this problem. This means that on some occasions, we will inline the CSS unnecessarily, for example, in a template that renders a post without a featured image. But we can add this optimization back (and even add it in other block supports) once the enhanced pagination is able to detect the CSS corresponding to each block.

Testing Instructions

  • Create multiple posts so you can paginate with the Query block.
  • Make it so that on the first page of the Query block, none of the posts have featured images.
  • Make it so that on the second page of the Query block, at least one post has a featured image.
  • Add a duotone to the featured image.
  • Enable the enhanced pagination setting.
  • Check that the duotone works fine when you paginate.
  • Repeat with different types of duotone: presets, CSS, custom, global styles…

Screenshots or screencast

Before:

Screen.Capture.on.2023-10-17.at.12-15-05.mov

After:

Screen.Capture.on.2023-10-17.at.12-14-03.mp4

@luisherranz luisherranz added [Type] Bug An existing feature does not function as intended [Feature] Colors Color management labels Oct 17, 2023
@luisherranz luisherranz self-assigned this Oct 17, 2023
@github-actions
Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/class-wp-duotone-gutenberg.php

@github-actions
Copy link

Flaky tests detected in dd5efc19ec6d6a136c790eb00ab2082f60a6e547.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6548240211
📝 Reported issues:

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I'm afraid this fix isn't quite working for me. Not quite sure why, but it seems the duotone classnames can still vary across pages. In my local, I have pagination set to 4 items per page; on the first page there's only one featured image, and on the second page all 4 posts have it. One of the posts on the second page has a classname for which the style isn't being output:

Screenshot 2023-10-18 at 4 05 11 pm

It might be that we need a similar logic to the layout fix here?

@luisherranz
Copy link
Member Author

luisherranz commented Oct 18, 2023

Can you share the exact steps to reproduce that issue? Like the exact configuration of blocks, their hierarchy and the type of duotone (preset, css, custom or global).

@luisherranz luisherranz added Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta Needs PHP backport Needs PHP backport to Core labels Oct 18, 2023
@tellthemachines
Copy link
Contributor

Can you share the exact steps to reproduce that issue? Like the exact configuration of blocks, their hierarchy and the type of duotone (preset, css, custom or global).

Sure!

Here's the template markup
<!-- wp:query {"queryId":20,"query":{"perPage":4,"pages":0,"offset":0,"postType":"post","order":"desc","orderBy":"date","author":"","search":"","exclude":[],"sticky":"","inherit":true},"enhancedPagination":true,"layout":{"type":"constrained"}} -->
<div class="wp-block-query"><!-- wp:post-template {"layout":{"type":"grid","columnCount":3}} -->
<!-- wp:group {"align":"wide","layout":{"type":"constrained"}} -->
<div class="wp-block-group alignwide"><!-- wp:post-title {"isLink":true} /-->

<!-- wp:post-featured-image {"isLink":true,"align":"wide","style":{"color":{"duotone":["#5b0d73","rgb(252, 254, 124)"]}}} /-->

<!-- wp:post-excerpt {"moreText":"\u003cbr\u003e\u003cbr\u003e\u003cbr\u003e"} /--></div>
<!-- /wp:group -->

<!-- wp:separator {"opacity":"css"} -->
<hr class="wp-block-separator has-css-opacity"/>
<!-- /wp:separator -->

<!-- wp:post-date /-->
<!-- /wp:post-template -->

<!-- wp:query-pagination {"layout":{"type":"flex","justifyContent":"center"}} -->
<!-- wp:query-pagination-previous /-->

<!-- wp:query-pagination-numbers /-->

<!-- wp:query-pagination-next /-->
<!-- /wp:query-pagination --></div>
<!-- /wp:query -->

<!-- wp:gallery {"linkTo":"none"} -->
<figure class="wp-block-gallery has-nested-images columns-default is-cropped"><!-- wp:image {"id":160,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large"><img src="http://localhost:8888/wp-content/uploads/2023/10/PXL_20230428_121033487-1024x768.jpg" alt="" class="wp-image-160"/></figure>
<!-- /wp:image -->

<!-- wp:image {"id":75,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large"><img src="http://localhost:8888/wp-content/uploads/2023/09/tumblr_ef4d6c9edbff0723397fd2bf21103b2d_c3a297fd_1280-1024x768.jpg" alt="" class="wp-image-75"/></figure>
<!-- /wp:image -->

<!-- wp:image {"id":52,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large"><img src="http://localhost:8888/wp-content/uploads/2023/09/tumblr_13f0f7a77e8cf2cb694fc5f3e38543c9_8cd79083_540.jpg" alt="" class="wp-image-52"/></figure>
<!-- /wp:image --></figure>
<!-- /wp:gallery -->

I'm using TT3 theme with the Pilgrimage style variation, that uses a default duotone filter for all images. On top of that, I customised the Featured Image block to use one of the preset duotone filters.

Note that the template only contains the Query and a Gallery block. If I remove the Gallery the bug persists, except that funnily enough, on the second posts page the image that doesn't get the correct duotone preset now shows with no duotone filter at all (instead of showing with the default duotone from the theme variation).

Hope this helps!

@luisherranz luisherranz force-pushed the fix/make-duotonoe-work-with-enhanced-pagination branch from dd5efc1 to 32935e3 Compare October 19, 2023 10:59
@luisherranz
Copy link
Member Author

Thanks for the details, but I'm still not able to reproduce it.

I've made a video just in case it helps to understand if I'm missing something.

https://www.loom.com/share/6778b34d888547a482d3bb2bbc7e3310

@tellthemachines
Copy link
Contributor

Oh whoops sorry, turns out I wasn't using a preset filter! As the markup for Featured Image shows: <!-- wp:post-featured-image {"isLink":true,"align":"wide","style":{"color":{"duotone":["#5b0d73","rgb(252, 254, 124)"]}}} /-->. The rest of the steps on your video are correct.

I reset styles on my theme and tried to follow the same steps and am no longer able to reproduce the issue so I guess let's go with this solution and revisit later if the bug pops up again 🤷

@luisherranz
Copy link
Member Author

Ok, thanks. I'll merge it then, and I'll do the backport 🙂

@luisherranz luisherranz merged commit 23c6a71 into trunk Oct 20, 2023
49 checks passed
@luisherranz luisherranz deleted the fix/make-duotonoe-work-with-enhanced-pagination branch October 20, 2023 06:20
@github-actions github-actions bot added this to the Gutenberg 17.0 milestone Oct 20, 2023
@SiobhyB
Copy link
Contributor

SiobhyB commented Oct 20, 2023

Cherry-picked to the 6.4 branch in 687cb22.

@SiobhyB SiobhyB removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 20, 2023
@cbravobernal
Copy link
Contributor

Backport: WordPress/wordpress-develop#5540

SiobhyB pushed a commit that referenced this pull request Oct 23, 2023
* Focus submenu button when clicked (#55198)

* Focus element manually when open submenu on click

* Try using `tabindex="-1"`

* Use `tabindex="-1"` also in body when a submenu is opened

* Replace tabindex with event listener

* Explain the tabindex on <li>

* Don't store the element on hover to restore the focus later

* Improve explanations

* Add tests to cover webkit frontend menu interactions

Safari doesn't place focus on a clicked button as expected. These tests verify that when a submenu chevron button is clicked, focus is correctly placed on that button. It also verifies that the click on the body correctly closes any open submenus, which was failing in Safari.

* Focus clicked button on Safari

Combining the tabindex -1 on the parent li and focusing the button on Safari, and also checking that the relatedTarget is null inside the handleMenuFocusout seems to contain the focus within the menu to not fire the handleMenuFocusout as often, and still works to click on the body to close the menu.

* Added the document.addEventListener body click back in

Authored by Luis Herranz <luisherranz@gmail.com>. I'm just re-applying the change.

* Remove tab keypresses from webkit menu interaction tests

Tab keypreses on webkit playwright are really flakey (or it's something in our code that we haven't isolated) so I've split out the webkit tests to test everything I can without using a tab keypress.

* Use body click instead for consistency across environments

---------

Co-authored-by: Luis Herranz <luisherranz@gmail.com>
Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>

* Make layout support compatible with enhanced pagination (#55416)

* Make layout supports compatible with enhanced pagination

* Use sanitize_title and add `layout` to the class name

* Update default fullscreen icon for lightbox trigger (#55463)

* Make duotone compatible with enhanced pagination (#55415)

* Patterns: fix capabilities settings for pattern categories (#55379)

Co-authored-by: Daniel Richards <daniel.richards@automattic.com>

* Revert "Patterns: fix capabilities settings for pattern categories (#55379)"

This reverts commit 6f83c92.

* Image block: wrap images with hrefs in an A tag (#55470)

* This commit sees what happens when we wrap the image element in an A tag in the editor.
This is to ensure any inherited styles from the link element, such as border color, apply to the image.

* Removing duplicate <a href /> wrapper
Adding disabled onClick and aria attribute

* Image: Improve focus management in lightbox (#55428)

* Improve focus management

This commit removes the logic to set focus differently
based on event.pointerType and instead sets focus on the
dialog itself when the lightbox opens, and on the lightbox
trigger when the lightbox closes.

* Add delay before focusing when closing lightbox

* Put focus back on close button when opening lightbox

It turns out that placing focus on the modal was causing
inconsistent behavior in Safari, so I've put the focus back
on the close button when the lightbox opens, which
performs predictably.

I've also added a tabindex to the image, which prevents
the focus ring from erroneously showing when opening the lightbox
with a mouse in Chrome and Firefox.

* Move focus to the dialog when opening the lightbox.

* Fix SVG markup.

* Consistent indentation with spaces.

* Remove unnecessary tabindex

---------

Co-authored-by: Andrea Fercia <a.fercia@gmail.com>

* Fix: - Update the title when using enhanced pagination

---------

Co-authored-by: Mario Santos <34552881+SantosGuillamot@users.noreply.github.com>
Co-authored-by: Luis Herranz <luisherranz@gmail.com>
Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>
Co-authored-by: Artemio Morales <artemio.morales@a8c.com>
Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com>
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
Co-authored-by: Andrea Fercia <a.fercia@gmail.com>
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Oct 23, 2023
…agination.

Some blocks do not have content. For duotone support, blocks without content still need to run through the `render_duotone_support()` to render their duotone CSS.

This fix makes the duotone compatible with the enhanced pagination (introduced in 6.4.0) by making sure that the CSS is always on the page, even when the posts have no featured image. It also prevents the duotone from interfering with other blocks using `wp_unique_id()`.


References:
* [WordPress/gutenberg#55415 Gutenberg PR 55415]

Follow-up to [56226].

Props cbravobernal, luisherranz, hellofromTonya, isabel_brison, jorbin.
Fixes #59694.

git-svn-id: https://develop.svn.wordpress.org/trunk@56991 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Oct 23, 2023
…agination.

Some blocks do not have content. For duotone support, blocks without content still need to run through the `render_duotone_support()` to render their duotone CSS.

This fix makes the duotone compatible with the enhanced pagination (introduced in 6.4.0) by making sure that the CSS is always on the page, even when the posts have no featured image. It also prevents the duotone from interfering with other blocks using `wp_unique_id()`.


References:
* [WordPress/gutenberg#55415 Gutenberg PR 55415]

Follow-up to [56226].

Props cbravobernal, luisherranz, hellofromTonya, isabel_brison, jorbin.
Fixes #59694.
Built from https://develop.svn.wordpress.org/trunk@56991


git-svn-id: http://core.svn.wordpress.org/trunk@56502 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to gilzow/wordpress-performance that referenced this pull request Oct 23, 2023
…agination.

Some blocks do not have content. For duotone support, blocks without content still need to run through the `render_duotone_support()` to render their duotone CSS.

This fix makes the duotone compatible with the enhanced pagination (introduced in 6.4.0) by making sure that the CSS is always on the page, even when the posts have no featured image. It also prevents the duotone from interfering with other blocks using `wp_unique_id()`.


References:
* [WordPress/gutenberg#55415 Gutenberg PR 55415]

Follow-up to [56226].

Props cbravobernal, luisherranz, hellofromTonya, isabel_brison, jorbin.
Fixes #59694.
Built from https://develop.svn.wordpress.org/trunk@56991


git-svn-id: https://core.svn.wordpress.org/trunk@56502 1a063a9b-81f0-0310-95a4-ce76da25c4cd
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Oct 24, 2023
…agination.

Some blocks do not have content. For duotone support, blocks without content still need to run through the `render_duotone_support()` to render their duotone CSS.

This fix makes the duotone compatible with the enhanced pagination (introduced in 6.4.0) by making sure that the CSS is always on the page, even when the posts have no featured image. It also prevents the duotone from interfering with other blocks using `wp_unique_id()`.

References:
* [WordPress/gutenberg#55415 Gutenberg PR 55415]

Follow-up to [56226].

Props cbravobernal, luisherranz, hellofromTonya, isabel_brison, jorbin.
Reviewed by costdev.
Merges [56991] and [56996] to the 6.4 branch.
Fixes #59694.

git-svn-id: https://develop.svn.wordpress.org/branches/6.4@56997 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to WordPress/WordPress that referenced this pull request Oct 24, 2023
…agination.

Some blocks do not have content. For duotone support, blocks without content still need to run through the `render_duotone_support()` to render their duotone CSS.

This fix makes the duotone compatible with the enhanced pagination (introduced in 6.4.0) by making sure that the CSS is always on the page, even when the posts have no featured image. It also prevents the duotone from interfering with other blocks using `wp_unique_id()`.

References:
* [WordPress/gutenberg#55415 Gutenberg PR 55415]

Follow-up to [56226].

Props cbravobernal, luisherranz, hellofromTonya, isabel_brison, jorbin.
Reviewed by costdev.
Merges [56991] and [56996] to the 6.4 branch.
Fixes #59694.
Built from https://develop.svn.wordpress.org/branches/6.4@56997


git-svn-id: http://core.svn.wordpress.org/branches/6.4@56508 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@tellthemachines tellthemachines removed the Needs PHP backport Needs PHP backport to Core label Jan 21, 2024
@getdave
Copy link
Contributor

getdave commented Jan 22, 2024

✅ I updated the PHP Sync Tracking Issue for WP 6.5 to note this PR does not require a backport for WP 6.5.

@luisherranz luisherranz added the Backported to WP Core Pull request that has been successfully merged into WP Core label Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Feature] Colors Color management [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants