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?