Make WordPress Core

Opened 8 months ago

Closed 8 months ago

#59466 closed defect (bug) (fixed)

Twenty Twenty-Four custom button block style path incorrectly registered

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile flixos90
Milestone: 6.4 Priority: normal
Severity: normal Version: 6.4
Component: Bundled Theme Keywords: has-patch has-testing-info
Focuses: performance Cc:

Description

As noted in #59465, a render-blocking button-outline.css file from TT4 is enqueued, even though that CSS file is so small it should be inlined.

Looking at the TT4 code, it does intend to pass the stylesheet's path to WordPress core, so that core can inline the file contents, however the way that happens contains a bug.

Given the block style relies on wp_enqueue_block_style() (not wp_enqueue_style()), wp_style_add_data() cannot be used with it, since the actual stylesheet won't be registered yet. In other words, the wp_style_add_data() call there doesn't do anything.

This is a bit confusing, particularly since wp_enqueue_block_style() lacks documentation that the $path can actually be provided there as an argument, which only becomes clear when looking at the code of that function. This should be updated here as well.

Change History (14)

This ticket was mentioned in PR #5321 on WordPress/wordpress-develop by @flixos90.


8 months ago
#1

  • Keywords has-patch added; needs-patch removed
  • The 'path' argument of wp_enqueue_block_style() needs to be used instead of wp_style_add_data(). The latter only works when using wp_register_style() or wp_enqueue_style().
  • The URL and path need to be provided using the same set of functions - preferably get_theme_file_uri() and get_theme_file_path(), so that they can be optionally overwritten in a child theme.
  • The wp_enqueue_block_style() function has its documentation improved to add the missing parameter.
  • A few empty lines were modified to no longer include tabs, which shouldn't be the case per WPCS.

Trac ticket: https://core.trac.wordpress.org/ticket/59466

#2 @flixos90
8 months ago

  • Keywords has-testing-info added

To test the PR https://github.com/WordPress/wordpress-develop/pull/5321:

  1. Load the regular TT4 home page (which includes buttons).
  2. Without the PR, the wp-content/themes/twentytwentyfour/assets/css/button-outline.css file should be loaded (you should also see it in the source code).
  3. With the PR, that file should no longer be loaded. Instead, there should be a style tag with ID twentytwentyfour-button-style-outline-inline-css in the source code.

#3 @luminuu
8 months ago

And one more TT4 @desrosj

#4 @desrosj
8 months ago

  • Version set to trunk

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


8 months ago

#6 follow-up: @joemcgill
8 months ago

Per this comment, the development GitHub repo for the TT4 theme is still open through RC1, so let's make sure these changes happen there first so they get properly synced in a future TT4 theme merge.

Last edited 8 months ago by joemcgill (previous) (diff)

@hellofromTonya commented on PR #5321:


8 months ago
#7

@felixarntz I think this needs to be fixed upstream in the TT4 repo as development is happening there https://github.com/WordPress/twentytwentyfour.

@hellofromTonya commented on PR #5321:


8 months ago
#8

@felixarntz I think this needs to be fixed upstream in the TT4 repo as development is happening there https://github.com/WordPress/twentytwentyfour.

#9 in reply to: ↑ 6 @hellofromTonya
8 months ago

Replying to joemcgill:

Per this comment, the development GitHub repo for the TT4 theme is still open through RC1, so let's make sure these changes happen there first so they get properly synced in a future TT4 theme merge.

I agree with @joemcgill. The report and fix needs to happen in the TT4 repo, rather than in Core itself.

@flixos90 can you open the issue in that repo and then port the PR over there please?

Then this ticket can be closed as reported upstream.

#10 @flixos90
8 months ago

I have implemented a Twenty Twenty-Four PR for this in https://github.com/WordPress/twentytwentyfour/pull/545.

I would prefer that we keep this ticket open and close it as part of the merge / update of TT4 to WP core trunk with the fix included.

#11 @flixos90
8 months ago

In 56754:

Script Loader: Fix missing documentation for the $path argument of wp_enqueue_block_style().

See #59466.

#13 @sabernhardt
8 months ago

The theme change was part of changeset 56764. Is this completed now?

Last edited 8 months ago by sabernhardt (previous) (diff)

#14 @flixos90
8 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

Thanks @sabernhardt, good catch!

This is indeed fixed as of [56764].

Note: See TracTickets for help on using tickets.