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

Parse (area-specific) context in template-part block. #47203

Merged
merged 4 commits into from
Jan 23, 2023

Conversation

felixarntz
Copy link
Member

What?

Similar to #45534, this PR is part of the solution from WordPress/wordpress-develop#3560, which addresses https://core.trac.wordpress.org/ticket/56930. We need to pass the $context parameter to wp_filter_content_tags() in the template-part block.

Why?

We should always pass a context parameter to wp_filter_content_tags(), except when there is a current filter running. This is not the case when parsing the block template though, which is why WordPress/wordpress-develop#3560 already takes care of it for the overall template. It also does so for the template-part block, however since that can only be edited within upstream Gutenberg, I have opened this PR.

Including the template part area identifier is useful context for that function, as it can make different decisions based on that. For example, images in the header of a site are extremely likely to appear above the fold, while for images in the footer the opposite applies.

How?

The implementation here goes a bit beyond the core PR WordPress/wordpress-develop#3560, using a cleaner implementation: We should ensure that the $area string is one of the allowed area identifiers before passing it through in the context.

Testing Instructions

Test this as part of WordPress/wordpress-develop#3560:

  1. Add a header template part with an image in it.
  2. Add a footer template part with an image in it.
  3. See how the image in the header is not lazy-loaded, while the one in the footer is lazy-loaded.

@felixarntz felixarntz added [Type] Bug An existing feature does not function as intended [Type] WP Core Ticket Requires an upstream change from WordPress. Core Trac ticket should be linked. [Type] Performance Related to performance efforts [Feature] Full Site Editing [Block] Template Part Affects the Template Parts Block labels Jan 16, 2023
@felixarntz
Copy link
Member Author

@Mamaduka Can you take a look at this?

Apologies for asking that again (especially after we now already backported a few package updates in https://core.trac.wordpress.org/changeset/55079 🤦‍♂️), but if this is good to go, is there any chance that we can backport it soon? Unfortunately it blocks the same core PR WordPress/wordpress-develop#3560. To be fair, since we're not far from 6.2 beta, can you tell me when the regular package update would happen for core? Maybe that would be enough.

Anyway, first of all please take a look at the PR :)

@github-actions
Copy link

github-actions bot commented Jan 16, 2023

Flaky tests detected in 55abfe5.
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/3946945642
📝 Reported issues:

Copy link
Member

@Mamaduka Mamaduka 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 ping, @felixarntz.

To be fair, since we're not far from 6.2 beta, can you tell me when the regular package update would happen for core? Maybe that would be enough.

Technically we can start doing regular package releases for the 6.2 beta after Gutenberg 15.1 RC is released. It's scheduled for next week. I can ensure we merge this update before RC so it's included in the initial package updates PR.

We also have a similar issue with the Cover block when it's used as a featured image. It might be a good idea to patch it as well.

@felixarntz
Copy link
Member Author

@Mamaduka I reviewed the issue, tested it, and left a reply. Please take a look.

For context, the core ticket https://core.trac.wordpress.org/ticket/56930 already covers the part of the issues reported that is realistic to cover at this point. There is one issue there that remains (core/cover usage in block templates), however fixing that is out of scope for that ticket, since that usage would be outside of "the loop". Any usage outside of the loop is also a problem for classic themes FWIW. It is something that members of the performance team including myself are actively looking into fixing, however it will require another separate workstream and ticket. 56930 will at least fix the most prevalent problems, for when such images are used within the loop.

In other words, this PR should be good as is. It would be great if we could get it into the Gutenberg 15.1 RC release.

@Mamaduka
Copy link
Member

Thank you, @felixarntz!

I'll make sure to get this merged before 15.1 RC, more likely before the end of the week.

@felixarntz felixarntz requested review from Mamaduka and carlomanf and removed request for carlomanf January 18, 2023 09:58
@felixarntz
Copy link
Member Author

@Mamaduka Friendly ping here. Just want to make sure this isn't forgotten :)

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

This looks great.

I believe WordPress/wordpress-develop#3560 will need a refresh to use the updated context name.

@Mamaduka Mamaduka merged commit ea4af80 into trunk Jan 23, 2023
@Mamaduka Mamaduka deleted the fix/pass-context-in-template-part-block branch January 23, 2023 12:21
@github-actions github-actions bot added this to the Gutenberg 15.1 milestone Jan 23, 2023
felixarntz added a commit to felixarntz/wordpress-develop that referenced this pull request Jan 27, 2023
felixarntz added a commit to felixarntz/wordpress-develop that referenced this pull request Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Template Part Affects the Template Parts Block [Type] Bug An existing feature does not function as intended [Type] Performance Related to performance efforts [Type] WP Core Ticket Requires an upstream change from WordPress. Core Trac ticket should be linked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants