Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 17 months ago

#59466 closed defect (bug) (fixed)

Twenty Twenty-Four custom button block style path incorrectly registered

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

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 (15)

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


3 years 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
3 years 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
3 years ago

And one more TT4 @desrosj

#4 @desrosj
3 years ago

  • Version → trunk

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


3 years ago

#6 follow-up: @joemcgill
3 years 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 3 years ago by joemcgill (previous) (diff)

​@hellofromTonya commented on ​PR #5321:


3 years 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:


3 years 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
3 years 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
3 years 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
3 years ago

In 56754:

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

See #59466.

#13 @sabernhardt
3 years ago

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

Last edited 3 years ago by sabernhardt (previous) (diff)

#14 @flixos90
3 years ago

  • Resolution → fixed
  • Status assigned → closed

Thanks @sabernhardt, good catch!

This is indeed fixed as of [56764].

#15 @wordpressdotorg
17 months ago

  • Keywords has-test-info added; has-testing-info removed
Note: See TracTickets for help on using tickets.