WordPress.org

Make WordPress Core

Opened 6 weeks ago

Closed 3 weeks ago

Last modified 2 weeks ago

#54597 closed defect (bug) (fixed)

Featured Image section missing on Posts and Pages

Reported by: noisysocks Owned by: audrasjb
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-screenshots has-patch has-unit-tests
Focuses: Cc:

Description

Originally reported in https://github.com/WordPress/twentytwentytwo/issues/273.

Describe the bug
With WP 5.9-beta1 with Twenty Twenty-Two block theme, and when editing a Post (All Posts > Hello World), I don’t see the section to add the Featured Image in the Post Settings sidebar.
If I switch to Twenty Twenty-One, the Featured Image section is there, and if I activate the Gutenberg plugin, the Featured Image section is there in the sidebar.

To Reproduce
Steps to reproduce the behavior:

  1. Go to 'Posts > All Posts > Hello World (or any other added post)'
  2. Click on 'Settings > Posts sidebar'
  3. Scroll down to 'Featured Image section'
  4. It's not there

Expected behavior
Section to add Featured Image should be in the Set

Screenshots
Attached are screenshots of Twenty Twenty-Two and Twenty Twenty-One

Device:

  • Device: Mac Mini
  • OS: Mac OS Big Sur 11.6
  • Browser: Chrome iVersion 96.0.4664.55 (Official Build) (x86_64) and FireFox 94.0.2 (64bit)
  • Version:

Additional context

Attachments (2)

54597-unit-test.diff (3.7 KB) - added by costdev 6 weeks ago.
Unit tests added for review. Updated to meet Coding Standards.
non-block-caption-54597.png (2.9 MB) - added by hellofromTonya 3 weeks ago.
TT2 theme with using non-block [gallery] and [caption] content: Before and after adding gallery and caption HTML5 theme supports. No difference.

Change History (33)

#1 @noisysocks
6 weeks ago

As identified in https://github.com/WordPress/twentytwentytwo/issues/273#issuecomment-987023378, the problem is that this Gutenberg PR was missed while updating Core with Gutenberg changes:

https://github.com/WordPress/gutenberg/pull/35593

We need to make the PHP changes in the PR above to Core.

#2 @audrasjb
6 weeks ago

  • Keywords has-screenshots added

I can confirm I can reproduce the issue as well on WordPress 5.9 Beta 2 using TT2 theme.
https://i.gyazo.com/e6f8727425f712bcece75e6d352eb62f.mp4

This ticket was mentioned in PR #2029 on WordPress/wordpress-develop by noisysocks.


6 weeks ago

  • Keywords has-patch added; needs-patch removed

By default, block themes should have a few theme supports enabled by default. These are: post-thumbnails, responsive-embeds, editor-styles, html5, automatic-feed-links.

This is a backport of https://github.com/WordPress/gutenberg/pull/35593.

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

#4 @prbot
6 weeks ago

noisysocks commented on PR #2029:

I tried adding a unit test for this but couldn't quite figure it out. Perhaps it's not possible because setup_theme is called during bootstrap. Any advice on that is welcome.

#5 @costdev
6 weeks ago

  • Keywords has-unit-tests added

#6 @costdev
6 weeks ago

Test Report

Env

  • WordPress 5.9-beta2-52344-src
  • Chrome 96.0.4664.93
  • Windows 10
  • Theme: Twenty Twenty-Two
  • Gutenberg Editor
  • Plugin: None activated.

Steps to test

  1. Navigate to Posts > Add New.
  2. There should not be a "Featured image" section in the sidebar.
  3. Apply PR 2029.
  4. Navigate to Posts > Add New.
  5. There should be a "Featured image" section in the sidebar.
  6. Repeat the process for Pages > Add New.

Results

  • Before PR 2029: There is no "Featured image" section on both Posts > Add New and Pages > Add New.
  • After PR 2029: There is a "Featured image" section on both Posts > Add New and Pages > Add New.

@costdev
6 weeks ago

Unit tests added for review. Updated to meet Coding Standards.

#7 @hellofromTonya
6 weeks ago

  • Keywords commit added

Marking PR 2029 for commit.

#8 @audrasjb
5 weeks ago

  • Owner set to audrasjb
  • Resolution set to fixed
  • Status changed from new to closed

In 52369:

Editor: Activate missing default theme features for block themes.

By default, block themes should have a few theme supports enabled by default. These are: post-thumbnails, responsive-embeds, editor-styles, html5, automatic-feed-links. This changeset backports the changes introduced in https://github.com/WordPress/gutenberg/pull/35593.

Props noisysocks, ocean90, youknowriad, audrasjb, hellofromTonya.
Fixes #54597.

#9 @audrasjb
5 weeks ago

Ah, I didn't notice @costdev proposed some unit tests to check for default theme supports.
@hellofromTonya should we open a new ticket to consider adding these before beta 4 or should we just reopen this one and add them before beta 3?

#10 @joyously
5 weeks ago

Should the list also have 'title-tag'?
Isn't there a block for logo? (so the list should have 'custom-logo'?)
I don't know how the blocks are for custom-header or custom-background.
And considering that blocks could be added anywhere, that includes Classic blocks and legacy widgets, so should the list of html5 support include 'gallery', 'caption', 'search-form', 'navigation-widgets' or would that depend on what the theme provides in its CSS? And if the theme has CSS for those, does it get the proper markup by calling add_theme_support() itself?

#11 @SergeyBiryukov
5 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to address the follow-up comments.

#12 @SergeyBiryukov
5 weeks ago

@costdev Thanks for the tests in 54597-unit-test.diff!

They look good to me, I'm only confused a bit by this line, as there is no such method in the WP_Hook class:

@covers WP_Hook::should_load_separate_core_block_assets

I guess the intention was to test the should_load_separate_core_block_assets filter, but in that case, I think we can just mention the function name here:

@covers wp_should_load_separate_core_block_assets

#13 @costdev
5 weeks ago

@SergeyBiryukov You're right! I'll update the unit tests with:

@covers ::wp_should_load_separate_core_block_assets

I'll wait to submit the updated unit tests until the follow up comments have been addressed, in case this requires more tests.

This ticket was mentioned in Slack in #core-editor by costdev. View the logs.


5 weeks ago

#15 @Mamaduka
5 weeks ago

Hello, everyone

Should the list also have 'title-tag'?

I'm not 100% sure about this. @aristath, I think you worked on this part of the code for Block Themes. Do you know if we need to declare support?

Isn't there a block for logo? (so the list should have 'custom-logo'?)

There's a Site Logo block, and it doesn't require declaring support for custom-logo.

I don't know how the blocks are for custom-header or custom-background.

Site Editor handles these with template parts and Global Styles.

so should the list of html5 support include 'gallery', 'caption', 'search-form', 'navigation-widgets'

The blocks provide their markup; this shouldn't be necessary.

#16 @SergeyBiryukov
5 weeks ago

In 52383:

Tests: Add unit tests for theme features that block themes should support by default.

By default, block themes should have a few theme supports enabled:

  • post-thumbnails
  • responsive-embeds
  • editor-styles
  • html5 for comment-form, comment-list, style, script
  • automatic-feed-links

They should also load core block assets only when the blocks are rendered. This commit adds the associated tests.

Follow-up to [52369].

Props costdev.
See #54597.

#17 follow-up: @joyously
5 weeks ago

so should the list of html5 support include 'gallery', 'caption', 'search-form', 'navigation-widgets'

The blocks provide their markup; this shouldn't be necessary.

You missed the point that the legacy markup can be either html5 or not, dependent on the value of the theme_support. So for the theme's styles to match the markup, it needs to select which markup to use. Or just go forward with html5 only.

#18 @SergeyBiryukov
5 weeks ago

In 52386:

Tests: Move the tests for theme features that block themes should support by default to a more appropriate place.

As these tests are intended for the _add_default_theme_supports() function rather than WP_Theme class methods, the tests/theme.php file is a more logical place for them than tests/theme/wpTheme.php.

Follow-up to [52369], [52383].

See #54597.

#19 @hellofromTonya
4 weeks ago

  • Keywords commit removed

All patches have been committed. Removing the commit keyword. Leaving the ticket open to resolve the open questions.

#20 in reply to: ↑ 17 @hellofromTonya
3 weeks ago

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

Replying to Mamaduka:

Should the list also have 'title-tag'?
I'm not 100% sure about this. @aristath, I think you worked on this part of the code for Block Themes. Do you know if we need to declare support?

No the <title> tag is automatically added in a block theme. Adding add_theme_support( 'title-tag' ); doesn't do anything.

Replying to joyously:

so should the list of html5 support include 'gallery', 'caption', 'search-form', 'navigation-widgets'

The blocks provide their markup; this shouldn't be necessary.

You missed the point that the legacy markup can be either html5 or not, dependent on the value of the theme_support. So for the theme's styles to match the markup, it needs to select which markup to use. Or just go forward with html5 only.

Did some testing on 'gallery', 'caption', 'search-form', and 'navigation-widgets'. Without adding them to the HTML5 theme support array, each renders as HTML5. So by default, they are already HTML5. As @Mamaduka noted, the block has the markup in it.

This makes sense as core blocks should be HTML5 markup IMO given that it was introduced back in 2014. Thinking out loud, supporting legacy and HTML5 in blocks seems unnecessary. That said, if there's a use case, I'd suggest opening an issue in Gutenberg for discussion and discovery as the work would be done there for block packages to be published and then backported to Core.

I'm reclosing this ticket as no additional theme supports are needed for 5.9.

#21 @joyously
3 weeks ago

No the <title> tag is automatically added in a block theme.

I see that there is now a private function https://developer.wordpress.org/reference/functions/_block_template_render_title_tag/ which does not check the support. That page refers to the old function _wp_render_title_tag() which did, and that link is 404.

Did some testing on 'gallery', 'caption', 'search-form', and 'navigation-widgets'. Without adding them to the HTML5 theme support array, each renders as HTML5. So by default, they are already HTML5. As @Mamaduka noted, the block has the markup in it.

Again, it sounds like you are talking about blocks, not legacy content which millions of sites still have. The theme_support for html5 refers to legacy functions that generate the search form (widget or direct call), gallery shortcode, caption shortcode, and Menu widgets (this is for a11y).
Or are you saying that using a block theme, the legacy content is somehow transformed? Or that the block theme doesn't have specific styles for anything but blocks so these things don't matter?

@hellofromTonya
3 weeks ago

TT2 theme with using non-block [gallery] and [caption] content: Before and after adding gallery and caption HTML5 theme supports. No difference.

#22 @hellofromTonya
3 weeks ago

Replying to joyously:

I see that there is now a private function https://developer.wordpress.org/reference/functions/_block_template_render_title_tag/ which does not check the support. That page refers to the old function _wp_render_title_tag() which did, and that link is 404.

Right, that private function was added in 5.8.0. In 5.9, in the locate_block_template() function, the _wp_render_title_tag callback is unhooked from 'wp_head' (see https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/block-template.php#L109). In 5.9, the <title> tag is automatically rendered for block themes. Non-block themes are not changed or impacted.

Replying to joyously:

Did some testing on 'gallery', 'caption', 'search-form', and 'navigation-widgets'. Without adding them to the HTML5 theme support array, each renders as HTML5. So by default, they are already HTML5. As @Mamaduka noted, the block has the markup in it.

Again, it sounds like you are talking about blocks, not legacy content which millions of sites still have. The theme_support for html5 refers to legacy functions that generate the search form (widget or direct call), gallery shortcode, caption shortcode, and Menu widgets (this is for a11y).
Or are you saying that using a block theme, the legacy content is somehow transformed? Or that the block theme doesn't have specific styles for anything but blocks so these things don't matter?

The default theme supports additions are for block themes. Non-block themes are not impacted.

What about the non-block content when using a block theme? Hmm, I was curious. So I did a test.

  • Started with WordPress 4.9 and Twenty Fifteen theme. Added a gallery, image with caption, and search widget.
  • Then upgraded to 5.9. Result: No changes in the HTML markup. The original content in the editor shows the [gallery] and [caption] shortcodes.
  • Then switched to TT2.
  • Then added default theme support for 'gallery' and 'caption'.

See non-block-caption-54597.png. Notice there's no difference in the markup before and after adding HTML5 support for both 'gallery' and 'caption'.

What do you think @joyously?

#23 @joyously
3 weeks ago

That page refers to the old function _wp_render_title_tag() which did, and that link is 404.

Yes, but something is wrong with the Code Reference if there is a link to a non-existent page.

Notice there's no difference in the markup before and after adding HTML5 support

https://core.trac.wordpress.org/browser/tags/5.8.1/src/wp-includes/media.php#L2303 is where the support is checked for the gallery shortcode. Without support, a gallery uses dl, dt, and dd with inline style tag. With it, a gallery uses figure, div, and figcaption and no inline style tag.

The caption shortcode https://core.trac.wordpress.org/browser/tags/5.8.1/src/wp-includes/media.php#L2202
uses figure and figcaption for html5, and div with p and an extra 10px of width without html5. So your screenshots show two cases of no html5 support.

#24 follow-up: @hellofromTonya
3 weeks ago

You're right @joyously. Thanks for reminding me. For some reason, the theme support was not being added correctly in my test. Fixed it. Thanks.

The change in this ticket is targeted at block themes and blocks.

Question: @joyously, for block themes, do you think that Core should automatically default to HTML5 for all content (i.e. block and non-block content)? Or should the block theme itself set the non-block content theme supports (as non-block themes do)?

#25 in reply to: ↑ 24 ; follow-up: @audrasjb
3 weeks ago

Replying to hellofromTonya:

Question: @joyously, for block themes, do you think that Core should automatically default to HTML5 for all content (i.e. block and non-block content)? Or should the block theme itself set the non-block content theme supports (as non-block themes do)?

As far as I can tell, it would make sense if all block themes could only use and provide HTML5 markup. Block themes are a good way to drop non-HTML5 support in long term, if possible :)

#26 in reply to: ↑ 25 @hellofromTonya
3 weeks ago

Replying to audrasjb:

As far as I can tell, it would make sense if all block themes could only use and provide HTML5 markup. Block themes are a good way to drop non-HTML5 support in long term, if possible :)

I agree. Might want to open a new ticket though as the scope of this ticket has grown past the original reported issue. It sounds like we're leaning towards making all block themes HTML5 markup only by default within Core itself.

Last edited 3 weeks ago by hellofromTonya (previous) (diff)

#27 @joyously
3 weeks ago

Might want to open a new ticket though as the scope of this ticket has grown past the original reported issue.

It's part of the original patch. Just some parameters got left out.
I think it would be good to force HTML5 for block themes, but that's just an opinion.

#28 @costdev
3 weeks ago

+1 That block themes should use HTML5 by default.

#29 @costdev
3 weeks ago

Separate ticket for full HTML5 support opened here: #54731

#30 @hellofromTonya
3 weeks ago

Moving the discussion about making block themes support HTML5 by default to #54731. Why? To capture the context for history. The issue raised and fixed in this ticket is highly specific. But making block themes HTML5 is a bigger and worthy of its own ticket.

#31 @hellofromTonya
2 weeks ago

In 52439:

Themes: Make block themes support HTML5 by default.

For block themes, [52369] added HTML5 support for 'comment-list', 'comment-form', 'style', and 'script'. However, when sites upgrade to 5.9 with non-block content such as a gallery and caption, the markup was not HTML5.

This commit adds full HTML5 theme feature support for block themes. Non-block content such as a [gallery] and [caption] shortcodes will natively be in HMTL5 markup without block themes needing to specifically add add_theme_support( 'html5, .. ) to the theme.

Follow-up to [24417], [27302], [34261], [52369], [52383], [52386].

Props @joyously, costdev, hellofromTonya, audrasjb, Mamaduka, ocean90.
Fixes #54731. See #54597.

Note: See TracTickets for help on using tickets.