Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#52047 closed defect (bug) (fixed)

Twenty Twenty-One: Remove untranslatable post type names

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: desrosj's profile desrosj
Milestone: 5.6.1 Priority: normal
Severity: normal Version: 5.6
Component: Bundled Theme Keywords: has-patch, commit, twenty-twenty-one-1.1
Focuses: Cc:

Description

Twenty Twenty-One uses these labels for the_post_navigation():

/* translators: %s: The post-type singular name (example: Post, Page, etc.) */
$twentytwentyone_next_label = sprintf( esc_html__( 'Next %s', 'twentytwentyone' ), $twentytwentyone_post_type_name );
/* translators: %s: The post-type singular name (example: Post, Page, etc.) */
$twentytwentyone_previous_label = sprintf( esc_html__( 'Previous %s', 'twentytwentyone' ), $twentytwentyone_post_type_name );

Unfortunately, just inserting a post type name into a generic string like Next %s is not translatable, hence the need for a full array of various labels provided when registering a post type.

A general best practice in WordPress is to avoid post type and taxonomy names in generic strings due to i18n concerns and structural differences in languages. For reference, see:

Since "Next post" and "Previous post" strings are often used in themes, it seems like adding them to post type labels in a future WordPress version would be beneficial.

For Twenty Twenty-One, I would suggest using "Next post" and "Previous post" for now instead of the code above, which is what all previous bundled themes did.

Attachments (3)

52047.patch (3.3 KB) - added by poena 4 years ago.
Remove post type names
52047.2.diff (4.3 KB) - added by SergeyBiryukov 4 years ago.
52047.3.diff (4.5 KB) - added by SergeyBiryukov 4 years ago.

Download all attachments as: .zip

Change History (26)

#1 @joyously
4 years ago

This came up because I complained that there was no navigation at all for custom post types, and the consensus was to use the post type in the string instead of a generic "Next" and "Previous".

This ticket was mentioned in PR #814 on WordPress/wordpress-develop by mukeshpanchal27.


4 years ago
#2

  • Keywords has-patch added

#3 @mukesh27
4 years ago

above PR #814 fix the issue

#4 follow-up: @aristath
4 years ago

The post-type label is translatable... Just not in the theme itself. I'll use WooCommerce products for this example but the same goes for any plugin that registers a post-type.
If WooCommerce registers the product post-type then the label for that post-type is translatable in WooCommerce since it gets added with a label __( 'Products', 'woocommerce' ).
So the theme just re-uses the label registered. If that label is translated then it will show as translated.
The fact that the post-type label is untranslatable in the theme doesn't mean that the post-type label is untranslatable... It just needs to be translated in the plugin that adds it.

Last edited 4 years ago by aristath (previous) (diff)

#5 @desrosj
4 years ago

  • Version set to 5.6

#6 in reply to: ↑ 4 @SergeyBiryukov
4 years ago

Replying to aristath:

The post-type label is translatable... Just not in the theme itself. I'll use WooCommerce products for this example but the same goes for any plugin that registers a post-type.

Sorry, I should have been a bit more clear :) Yes, the post type name itself might be translatable, it's just that Next %s or any other generic string like that is not correctly translatable.

For example, in Russian, "Post" is feminine and "Product" is masculine, so "Next post" and "Next product" would require a different translation and there is simply no way to translate Next %s to account for both. We also don't use capital letters for multiple words in a row, so "Next Post" or "Next Product" is not quite correct, it should be "Next post" or "Next product".

That is the case for many other languages too, and is the reason why WordPress core does not use generic strings for post type or taxonomy names, and offers an array of labels to be used instead.

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#7 @aristath
4 years ago

For example, in Russian, "Post" is feminine and "Product" is masculine, so "Next post" and "Next product" would require a different translation and there is simply no way to translate Next %s to account for both. We also don't use capital letters for multiple words in a row, so "Next Post" or "Next Product" is not quite correct, it should be "Next post" or "Next product".

Ah OK, that makes sense now. Thank you for the insights Sergey!

In that case, I suppose changing it to Next Posts would make sense, and other post-types can just create their own templates (as they usually do) :+1:

#8 @joyously
4 years ago

Change it to Next and Previous, and it works for all. Isn't that the default anyway?

#9 @poena
4 years ago

The default for the_post_navigation is to only show the title of the content.

The default aria label for the nav is "Posts", and default screen reader text is "Post navigation",
and when testing a custom post type, these two remain the same.

When I look at the previous default themes they don't use the same texts so there is no consistency.

The change will have the least impact on current users if we keep the word post in the text.
I have tested the PR and it works well for me.

#10 @poena
4 years ago

In my eagerness to get this into version 1.1. I missed two things:

Remove the post type label code from the pagination in
twenty_twenty_one_the_posts_navigation in the file template-tags.php:

https://github.com/WordPress/wordpress-develop/blob/master/src/wp-content/themes/twentytwentyone/inc/template-tags.php#L217

Make the casing for the word posts lowercase: Newer Posts would be Newer posts.

Casing
Change the casing in single.php to lower case from Post to post in the navigation.
(Newer comments is already lower case)

Last edited 4 years ago by poena (previous) (diff)

@poena
4 years ago

Remove post type names

#11 follow-up: @desrosj
4 years ago

52047.patch looks good to me, but I'm still not 100% sure it solves some of the problems that @SergeyBiryukov raised above.

The Newer/Previous and posts parts are still separated. I know this is because the latter has markup surrounding it, but those problems persist with this.

Maybe the best answer is to adjust the inline translator comments. Perhaps %s: posts, the post type name.

#12 in reply to: ↑ 11 @SergeyBiryukov
4 years ago

  • Keywords commit added

Replying to desrosj:

The Newer/Previous and posts parts are still separated. I know this is because the latter has markup surrounding it, but those problems persist with this.

Right, it's always better to have a full sentence for translation, even with some HTML in it, than several parts concatenated together.

52047.2.diff is my take on this:

  • Note that we don't need esc_html__() here, as core translations are considered safe, see previous discussions in comment:10:ticket:30724, #47385, #48161, #49190, #49535, #49536, #49537, etc. Overzealous escaping seems to come from _s or other themes following the WordPress.com VIP guidelines, or Theme Check suggestions, however not all of them apply to bundled themes. Previous bundled themes only escaped string when necessary (e.g. when used in attributes), but almost never as a security precaution with esc_html(). It looks like some new instances were introduced lately though, specifically with block patterns. Removing unnecessary escaping from Twenty Twenty-One would be something for another ticket.
  • While refreshing the patch, I also noticed that one string uses a space at the end, which also goes against the i18n best practices, as this can easily be missed, even with a translation comment:
    /* translators: There is a space after page. */
    'before_page_number' => esc_html__( 'Page ', 'twentytwentyone' ),
    
    If the space is required, it should be added in code, not as a translation. That would not cause any problems with RTL, if that was a concern here, as the label would just follow the text direction as for the rest of the page. This is also addressed in the patch.
Version 0, edited 4 years ago by SergeyBiryukov (next)

#13 @SergeyBiryukov
4 years ago

Just noting that the .nav-short class is used to hide some parts of a string. Looking at the CSS, apparently "posts" would be hidden on screens of 480px or less, and "Newer posts" would become just "Newer". This is also not quite translation-friendly and would look weird in some languages ("Newer what?"), but can be addressed in a new ticket.

#14 @poena
4 years ago

The context of where the navigation is shown, below the list of posts, should be enough for users to understand what the links are used for, even if it only says "Newer" and "Older".
Both the aria label and the screen reader text describes that it is the post navigation.

Has there been many reports about this being an issue in Twenty Twenty?

Twenty Seventeen and Twenty Sixteen for example does not have a visible text at all for the newer and older post pagination, only arrows.

This ticket was mentioned in Slack in #core-themes by desrosj. View the logs.


4 years ago

#16 follow-up: @poena
4 years ago

The escaping must not be removed. The themes team and contributors have discussed this thoroughly
and have worked towards the goal of having secure themes over the past several years.
Including discussions on GitHub during the development of this theme. https://github.com/WordPress/twentytwentyone/issues/479

It is not about if core translations are considered safe or not, because the scope is much larger than core translations.
The default themes are used as examples and are copied by both new and experienced developers.
The majority of the resulting code will not be something that is meant for core.

We see the result of this on daily basis when we review themes where the escaping is not correct, and that is why we also argumented for Twenty Twenty to include escaping last year.

#17 in reply to: ↑ 16 @SergeyBiryukov
4 years ago

Replying to poena:

The context of where the navigation is shown, below the list of posts, should be enough for users to understand what the links are used for, even if it only says "Newer" and "Older".
Both the aria label and the screen reader text describes that it is the post navigation.
Has there been many reports about this being an issue in Twenty Twenty?

It's more about the word "Newer" when used on its own requiring a different translation than in "Newer posts". In some languages, the form used for "Newer posts" does not look correct when shortened to "Newer", and another form should be used instead. I have just noticed now that it's the same in Twenty Twenty, so this can be addressed for both themes in a new ticket.

The escaping must not be removed. The themes team and contributors have discussed this thoroughly
and have worked towards the goal of having secure themes over the past several years.
Including discussions on GitHub during the development of this theme. https://github.com/WordPress/twentytwentyone/issues/479

Thanks, I was not aware of that. In that case, I agree with reconsidering the previous decision and keeping the escaping. Having a consistent standard for both bundled themes and any themes in the repo is indeed beneficial.

52047.3.diff uses wp_kses() for escaping here instead, which is consistent with the existing code we have in template-parts/content/content-none.php:

printf(
	'<p>' . wp_kses(
		/* translators: 1: link to WP admin new post page. */
		__( 'Ready to publish your first post? <a href="%1$s">Get started here</a>.', 'twentytwentyone' ),
		array(
			'a' => array(
				'href' => array(),
			),
		)
	) . '</p>',
	esc_url( admin_url( 'post-new.php' ) )
);

#18 @poena
4 years ago

52047.3.diff looks good to me.

This ticket was mentioned in Slack in #core-themes by poena. View the logs.


4 years ago

#20 @desrosj
4 years ago

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

In 49866:

Twenty Twenty-One: Improve strings found in post navigations for easier translating.

This change adjusts strings found within post navigations to ensure translators are provided the full context needed to properly translate.

Props poena, SergeyBiryukov, aristath.
Fixes #52047.

#21 @desrosj
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#22 @desrosj
4 years ago

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

In 49869:

Twenty Twenty-One: Improve strings found in post navigations for easier translating.

This change adjusts strings found within post navigations to ensure translators are provided the full context needed to properly translate.

Props poena, SergeyBiryukov, aristath.
Merges [49866] to the 5.6 branch.
Fixes #52047.

#23 @desrosj
4 years ago

  • Keywords twenty-twenty-one-1.1 added

Adding a custom keyword for querying tickets included in the Twenty Twenty-One version 1.1 release in the future.

Note: See TracTickets for help on using tickets.