#52047 closed defect (bug) (fixed)
Twenty Twenty-One: Remove untranslatable post type names
Reported by: | SergeyBiryukov | Owned by: | 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)
Change History (26)
This ticket was mentioned in PR #814 on WordPress/wordpress-develop by mukeshpanchal27.
4 years ago
#2
- Keywords has-patch added
Trac ticket: https://core.trac.wordpress.org/ticket/52047
#4
follow-up:
↓ 6
@
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.
#6
in reply to:
↑ 4
@
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.
#7
@
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
@
4 years ago
Change it to Next
and Previous
, and it works for all. Isn't that the default anyway?
#9
@
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
@
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:
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)
#11
follow-up:
↓ 12
@
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
@
4 years ago
- Keywords commit added
Replying to desrosj:
The
Newer/Previous
andposts
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 withesc_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.
#13
@
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
@
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:
↓ 17
@
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
@
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' ) ) );
This ticket was mentioned in Slack in #core-themes by poena. View the logs.
4 years ago
#20
@
4 years ago
- Owner set to desrosj
- Resolution set to fixed
- Status changed from new to closed
In 49866:
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".