#59694 closed defect (bug) (fixed)
Regression: Fix duotone support to be compatible with enhanced pagination
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Editor | Keywords: | has-patch has-unit-tests gutenberg-merge dev-reviewed |
Focuses: | Cc: |
Description (last modified by )
Reference: Gutenberg PR 55415
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
.
With the current implementation of enhanced pagination, the CSS corresponding to each block is still not detected. 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 https://github.com/WordPress/gutenberg/issues/55230).
Change History (29)
This ticket was mentioned in PR #5540 on WordPress/wordpress-develop by @cbravobernal.
16 months ago
#2
- Keywords has-patch has-unit-tests added
@cbravobernal commented on PR #5540:
16 months ago
#3
I'm not finding an easy way to test that in render_duotone_support()
function with an empty $block_content
, we are not returning early like we did before.
I tried checking that get_all_global_style_block_names()
is called, but, as is a private static function, I couldn't make it work with getMockBuilder
or ReflectionMethod
Any help would be kindly appreciated.
@cbravobernal commented on PR #5540:
16 months ago
#4
Hi there!
Pinging @tellthemachines for a review 😅
Thanks a lot!!
#6
@
16 months ago
- Component changed from General to Editor
- Description modified (diff)
- Keywords gutenberg-merge commit added
- Owner set to hellofromTonya
- Status changed from new to reviewing
- Summary changed from Backport Gutenberg #55415 - Make duotone support compatible with enhanced pagination to Merge Gutenberg #55415 - Make duotone support compatible with enhanced pagination
Thanks for opening the ticket to merge the changes from Gutenberg into Core.
Marking it for commit
to Core's trunk
.
What about backporting the change to the 6.4 branch?
@mikachan @cbravobernal Is this bugfix resolving a regression or break / error introduced during the 6.4 cycle?
See Make/Core post "WordPress 6.4 Release Candidate Phase":
Regressions: bugs that have been introduced during the WordPress 6.4 development cycle, either to existing or new features.
Exploring the fix:
- The change removes a conditional introduced in [56226] / #58673, i.e. during 6.3 cycle.
- [56101] first introduced the
WP_Duotone
class during the 6.3 cycle. - The issue in Gutenberg seems to be a bugfix to further address issues introduced in 6.3. It was opened 2 weeks ago and punted to 6.4.1.
In reviewing, it does not seem to fit within the RC guidelines. Awaiting more information before punting from 6.4.0 milestone.
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
16 months ago
#8
@
16 months ago
This change is a partial fix for the Gutenberg issue 55230. The other half of the fix is in ticket #59681. Both tickets should be considered together for whether they should or shouldn't be backported to the 6.4 branch, as together they solve the issue with enhanced pagination in the Query block.
#9
@
16 months ago
Sharing more information on whether this should or shouldn't be backported to the 6.4-branch.
As @cbravobernal noted in Make/Core slack core channel, the bug did happen in 6.4, as the enhanced pagination is being introduced in 6.4.
the layout support was working fine in 6.3. This is a compatibility fix with the new enhanced pagination of the Query block, which is being introduced in 6.4...
..this fixes an edge case only affecting sites using the new enhanced pagination in conjunction with featured images that are styled with a doutone
@cbravobernal commented on PR #5540:
16 months ago
#10
Patch Report
Patch tested: https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/5540.diff
How to read this report:
🐞 <= Bug occurs.
✅ <= Behavior is expected.
❌ <= Behavior is NOT expected.
Environment
- OS: macOS 14.0
- Web Server: Nginx
- macOS 14.0
- PHP 8.2.11
- 6.4-RC1-56475-src
- Firefox 118.0.2
Steps to Reproduce
- With WordPress 6.4 installed
- Create the three posts with featured image, and another three without featured image.
- On the site editor, in home page, TT4, add pagination to the Query Loop, set it to inherit the template settings, activate "Enhanced Pagination feature)
- Add duotone to the feature image inside the Query Loop.
- Make sure the three posts shown in the home page don't have featured image. Use pagination.
- 🐞 Check that on page two, duotone filter is not applied for next posts.
Steps to Test Patch
- 🛠 Apply the patch.
- Refresh the home page.
- ✅ Check that the duotone filter is applied after navigating to the second page.
- ✅ Check that the navigation happened without a page reload.
Test Results
- ✅ Issue is reproducible.
- ✅ Issue resolved with patch.
Screenshots
#11
@
16 months ago
In discussions and reviews, this is fixing a 6.4 regression introduced with the new enhanced pagination feature. Thus it is okay to be fixed in the 6.4 RC cycle.
#12
@
16 months ago
- Summary changed from Merge Gutenberg #55415 - Make duotone support compatible with enhanced pagination to Regression: Fix duotone support to be compatible with enhanced pagination
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
16 months ago
#14
@
16 months ago
In general, this looks good. I left one comment but I don't think it needs to block merge.
I do think there is further opportunity to improve the automated tests. For example, all the tests right now assume that the DuoTone should be added. This feels like a 6.5 improvement thought.
@hellofromTonya commented on PR #5540:
16 months ago
#15
For commit prep, I'll be pushing a few commits to this PR to address the failing PHP 8.3 test, review notes from @aaronjorbin, resetting the reflected property's visibility in the test, and a few other minor formatting things.
@hellofromTonya commented on PR #5540:
16 months ago
#18
Committed via https://core.trac.wordpress.org/changeset/56991.
#19
@
16 months ago
- Keywords dev-feedback added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for 2nd committer sign-off to backport [56991] to the 6.4 branch.
This ticket was mentioned in PR #5559 on WordPress/wordpress-develop by @hellofromTonya.
16 months ago
#20
@costdev found the test in r56991 did not properly handle the private static property:
- It set the object using
WP_Block
🤦♀️ - It needs to also reset to the original value when done.
As this property is static
and the class is also static by design, an object cannot be passed when setting the value. Instead, ReflectionClass
is needed, which exposes a getter (getStaticPropertyValue()
) and setter (setStaticPropertyValue()
) for static properties.
Trac ticket: https://core.trac.wordpress.org/ticket/59694
#21
@
16 months ago
- Keywords commit removed
Resetting the commit
keyword as @costdev found the test in [56991] was incorrectly handling the static property. https://github.com/WordPress/wordpress-develop/pull/5561 is a follow-up to resolve the reflection needs in the test.
This ticket was mentioned in PR #5561 on WordPress/wordpress-develop by @hellofromTonya.
16 months ago
#22
@costdev found the test in r56991 did not properly handle the private static property:
- It set the object using
WP_Block
🤦♀️ - It needs to also reset to the original value when done.
This PR fixes both of these issues.
It also addresses handling issues.
It uses ReflectionClass
which works well with static classes, i.e. a class that only contains static properties and methods. It then uses ReflectionClass::getProperty()
to get an instance of ReflectionClass
, which it then uses for changing the property's visibility, getting its value, and setting its value.
It also adds an inline comment to explain why an instance of WP_Duotone
is needed. IMO having an instance is confusing.
Why?
WP_Duotone
is a static class by design (as noted above), meaning it's not intended to be an object. But as of PHP 8.3, not passing an instance toReflectionProperty::setValue()
is deprecated. Therefore, an instance is needed to run in all Core supported PHP versions.
Trac ticket: https://core.trac.wordpress.org/ticket/59694
@hellofromTonya commented on PR #5559:
16 months ago
#23
Follow-up: This approach works awesome in PHP 7.4 and up, but it does not with < PHP 7.4 as shown by the failing tests for PHP 7.0, 7.1, and 7.3. Closing this PR in favor of https://github.com/WordPress/wordpress-develop/pull/5561.
@hellofromTonya commented on PR #5561:
16 months ago
#28
Committed into trunk
via https://core.trac.wordpress.org/changeset/56996 and to 6.4 branch via https://core.trac.wordpress.org/changeset/56997.
#29
@
5 months ago
Thanks, for help.https://pearllemonhomerepairs.co.uk/
Trac ticket: https://core.trac.wordpress.org/ticket/59694
This PR backports #55415 that fixes an issue with duotone and enhanced pagination. All the context is inside Gutenberg PR.