script-loader.php Need to use _n() when more than one results are found

[tobifjellner]

Location:
​https://build.trac.wordpress.org/browser/trunk/wp-includes/script-loader.php?marks=1178#L1178

Content:

'manyResults'  => __( '%d results found. Use up and down arrow keys to navigate.' ),

should (if possible) be converted to use _n() in order for some languages to be able to correctly construct the phrase.

[swissspidy]

The correct number is only known client-side, so for correct results one would need to use _n() from the @wordpress/i18n JS package.

This would need to happen in at least two places:

​https://github.com/WordPress/wordpress-develop/blob/3b14a06c8cb187d8abd3d4af8ea9c0ea3a475228/src/js/_enqueues/vendor/tinymce/plugins/wplink/plugin.js#L459-L469
​https://github.com/WordPress/wordpress-develop/blob/715a65c56142262b938d6860f840e201b1fe074a/src/js/_enqueues/admin/tags-suggest.js#L135-L144

However, this is by far not the only place where plural forms in JS could or should be improved. In #20491 you can find a huge patch with possible changes.

[SergeyBiryukov]

Introduced in [36806].

[shaampk1]

Hey folks,

This ticket really got my attention. I found it an interesting issue to work with. I know there has been already said a lot about L10n which I have been reading and trying to catch up with for the past few days now but still, I am not sure how far I have come regarding this issue. I'd appreciate it if you could review the following code snippet and let me know what I am missing.

For script-loader.php

	// Strings for 'jquery-ui-autocomplete' live region messages
	did_action( 'init' ) && $scripts->localize(
		'jquery-ui-autocomplete',
		'uiAutocompleteL10n',
		array(
			'noResults'    => __( 'No results found.' ),
			/* translators: %s: Number of results found when using jQuery UI Autocomplete. */
			'manyResults' => _n( '%s result found. Use up and down arrow keys to navigate.', '%s results found. Use up and down arrow keys to navigate.', did_action( 'init' ) ),
			'itemSelected' => __( 'Item selected.' ),
		)
	);

For plugin.js & tags-suggest.js

messages: {
	noResults: ( typeof window.uiAutocompleteL10n !== 'undefined' ) ? window.uiAutocompleteL10n.noResults : '',
	results: function( number ) {
		if ( typeof window.uiAutocompleteL10n !== 'undefined' ) {
			if ( number > 1 ) {
				return window.uiAutocompleteL10n.manyResults.replace( '%s', number );
			}

			return window.uiAutocompleteL10n.oneResult;
		}
	}
}

Thank you!

[shaharia.azam]

_n() has been applied in script-loader.php when more than one results are found

[shaharia.azam]

String is returning correctly. Removed .replace() from plugin.js

[shaharia.azam]

String is returning correctly. Removed .replace() from tags-suggest.js

[shaharia.azam]

It's for 4.5 . Both plugin.js and script-loader.php has been updated

[shaampk1]

Merged plugin.js, tags-suggest.js, and script-loader.php patches into one .diff file

[shaharia.azam]

As per @swissspidy on slack, _n implemented on plugin.js & tags-suggest.js

[swissspidy]

Please refer to the documentation on how to properly use these functions in JS: ​https://github.com/WordPress/gutenberg/tree/6942c1df30250d5f27e36c2938b7905acf79644c/packages/i18n#usage

We'd need to make sure that these scripts will have wp-i18n as a dependency as well.

Also: please test your patches.

[rsiddharth]

Initial version (2021091100).

[rsiddharth]

@swissspidy @SergeyBiryukov uploaded initial version of the patch.

[swissspidy]

Thanks for the patch. Two things I noticed at first glance:

• uiAutocompleteL10n definition needs to be removed in script-loader.php
• window.uiAutocompleteL10n needs to be deprecated using deprecateL10nObject

See r48925 for an example of how that deprecation works.

[rsiddharth]

Patch version 2021091300 - addresses issues pointed out by @swissspidy 

[rsiddharth]

@swissspidy, Thank you for reviewing the patch.

I've:

• Removed uiAutocompleteL10n from script-loader.php
• Deprecated window.uiAutocompleteL10n using deprecateL10nObject

Questions:

• Is script-loader.php:1099 (in the patch) the right place to add wp-i18n?
  I see the wplink plugin used in wp_tinymce_inline_scripts. Wondering if
  wp-i18n needs to be loaded there somehow?

• Are the @since and @deprecated version numbers correct for
  window.uiAutocompleteL10n?
  I guessed @since from https://core.trac.wordpress.org/changeset/36806

[swissspidy]

Nice! We're getting there!

With the deprecation being done in common.js, common should be added as a dependency to both scripts.

No need to add wp-i18n as a dependency.

Instead, use $scripts->set_translations( 'wplink' );, which will do this automatically. tags-suggest already has this call, so all good there.

Nice find with r36806! Since that changeset is referencing a ticket with milestone 4.5.0, that would be the correct version for the @since annotation.

[rsiddharth]

Patch version 2021091500 - addresses issues pointed out by @swissspidy

[rsiddharth]

@swissspidy, uploaded new version of the patch with the following changes:

• Added common as a dependency for wplink and tags-suggest.
• Used $scripts->set_translations( 'wplink' ); for loading wp-i18n for wplink.
• Changed @since for window.uiAutocompleteL10n to 4.5.0.

[hellofromTonya]

With 5.9 Beta 1 is happening in less than 4 hours, moving this ticket to 6.0.

[rsiddharth]

Regenerate patch

https://core.trac.wordpress.org/ticket/48244

Existing patch, some cs fixes.

[costdev]

As we are past soft-string freeze and now approaching RC1, I'm moving this ticket to the 6.1 milestone for remaining work to be carried out in the next cycle.

[audrasjb]

I refreshed the existing PR and updated the @since mentions.
Tests are still passing.

[chaion07]

@tobifjellner thank you for reporting this. We reviewed this ticket during a ​recent bug-scrub session. Based on the feedback received we are updating the ticket with the following changes:

1. Add keywords needs-testing since it needs to be tested. Even though JB mentioned in his most recent comment that the tests are still passing.
2. Removing keywords good-first-bug since the ticket has multiple patches

Cheers!

Props to @audrasjb & @hilayt24

[chaion07]

We discussed this ticket during another ​recent bug-scrub session and would love for a response from JB on this ticket. Thanks!

Props to @mukesh27

[audrasjb]

With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone as the proposed patch still needs testing.

[costdev]

With 6.2 Beta 4 now released, this ticket hasn't had any commits this cycle and still needs testing. Moving this to 6.3.

[oglekler]

@peterwilsoncc (or anyone who can squeeze it into own to-do list) I was unable to apply the patch, can you please look at it and update, and let's try to make it into 6.3 for not changing milestone continuously.

@tobifjellner it would be nice to provide testing instructions or/and screenshots.

Thank you all 🙏

[audrasjb]

As per today's bug scrub, let's move this to the next milestone as we are at the end of WP 6.3 beta cycle.

[marybaum]

@tobifjellner is going to talk to @SergeyBiryukov about doing some testing. Thanks, y'all!

[nicolefurlan]

@tobifjellner and @SergeyBiryukov is there any update on testing for this ticket?

[tobifjellner]

Testing is tricky, since it would need to be tested by running JavaScript, having a test language with test translation configured.

One simple way to solve this would be to change the string in line 937

'manyResults' => ( '%d results found. Use up and down arrow keys to navigate.' ),

To something like:
Number of results found: %d. Use up and down...

[oglekler]

@peterwilsoncc can you please rebase the patch?

[peterwilsoncc]

@oglekler Done: I merged in trunk and updated the deprecation version.

[oglekler]

@tobifjellner we can run JS, please, just tell us where to look. It is difficult to understand where this phrase should appear. To have a screenshot will be good.

[nicolefurlan]

This ticket was discussed in today's bug scrub.

Since this ticket still needs testing info and testing, we're bumping it to 6.5.

@tobifjellner could you check out #comment:43 and provide some testing info?

[swissspidy]

The patch at ​https://github.com/WordPress/wordpress-develop/pull/2618 looks good to me, marking as ready for commit.

[peterwilsoncc]

Moving to the script loader component as it covers a few JS files

[peterwilsoncc]

In 57654:

Script Loader: Switch to JavaScript translations.

Update JavaScript files for tag suggestions and the TinyMCE link plugin to use client side translations. This allows for _n() to be used for strings requiring singular and plural versions in which the correct form is only known client side.

Props audrasjb, chaion07, costdev, hellofromtonya, johnbillion, marybaum, nicolefurlan, oglekler, rebasaurus, rsiddharth, sergeybiryukov, shaampk1, shahariaazam, swissspidy, tobifjellner.
Fixes #48244.

Merged r57654 / ​https://github.com/WordPress/wordpress-develop/commit/de1ed9c57b1d2c00abaa0e473bec6979595489c3