WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 9 months ago

#29022 new defect (bug)

Screen reader text (and link title) isn't updated when plugin update count is updated

Reported by: pento Owned by:
Milestone: Future Release Priority: normal
Severity: minor Version: 3.9
Component: Upgrade/Install Keywords: has-patch
Focuses: accessibility, javascript, administration Cc:

Description

When plugins and themes are updating the update count on the admin bar and admin menu update automatically.

The admin bar also has screen reader text associated with this button, which isn't updated - leaving the wrong text associated with the new update count.

Attachments (9)

29022.patch (516 bytes) - added by paulschreiber 3 years ago.
29022.wip.patch (5.6 KB) - added by paulschreiber 3 years ago.
29022.diff (6.0 KB) - added by adamsilverstein 3 years ago.
First pass at updating screen reader and title counts after update
29022.2.diff (5.7 KB) - added by adamsilverstein 3 years ago.
update counts after upgrades
29022.3.diff (6.3 KB) - added by adamsilverstein 3 years ago.
fine tuning/refactoring
29022.4.diff (6.4 KB) - added by adamsilverstein 3 years ago.
remane variable title to screenReaderText - thats what its for
29022.5.diff (6.4 KB) - added by adamsilverstein 3 years ago.
always parseInt on localized ints
29022.6.diff (6.0 KB) - added by pento 3 years ago.
29022.7.diff (6.0 KB) - added by pento 3 years ago.

Download all attachments as: .zip

Change History (33)

#1 @pento
3 years ago

  • Keywords good-first-bug added

@paulschreiber
3 years ago

#2 @paulschreiber
3 years ago

  • Keywords has-patch added; needs-patch removed

In the admin bar, title attribute of the ab-item link contains the same text as the screen-reader-text ("1 Plugin Update"). When the update count changes, the title attribute gets the new count (n) but loses the localized text. The same is true of the update count title attribute in the admin menu.

Updating the screen reader text with the new count only is easy (see attached patch).

Updating all three locations with the correct string would require significantly more work (an AJAX call to get the correct localized string?).

#3 @pento
3 years ago

So, it turns out I may've been a bit optimistic in labelling this a good-first-bug. :-)

What I would suggest is using wp_localize_script() to send all the strings you'll need, and the actual update counts, in update-core.php.

In the JS, you'll need to duplicate the part of wp_get_update_data() that generates $update_title, to create the screan reader string.

Because you're sending the actual update counts, it's probably worth refactoring wp.updates.decrementCount() to use those counts instead of reading it from the HTML.

#4 @paulschreiber
3 years ago

Yesterday, at WordCamp, I created a slightly more elaborate patch. It removes the extra title attributes and passes a message to decrementCount containing the localized string.

Unfortunately, the string is incorrect and contains only the original number of updates needed (it doesn't decrement).

#5 @paulschreiber
3 years ago

pento: Are there other places in the WordPress code where localization is done in JavaScript? If so, I'd like to reuse that/emulate that behaviour.

#6 @paulschreiber
3 years ago

How the upgrade engine works was unclear. I was expecting XHR calls. Instead, there's an iframe. I still don't know how the iframe content is updated as the installs happen.

Perhaps it would make sense for the upgrade page to fetch in-progress status from the server and use that for page content.

#7 @pento
3 years ago

The strings generated by wp_get_update_data() will be incorrect until all updates have finished - the data is cached, and isn't regenerated until the end of the update process.

The iframe method is used, instead of an XHR-based system, because we're able to control the upgrade environment much more easily that way.

You can see an example of sending translated strings to JS in wp-admin/nav-menu.php, search for wp_localize_script. The menus data object created by this call can be seen used in wp-admin/js/nav-menu.js, search for menus. to see instances of it. In this case, you'd create an array of the strings in PHP, and call it like so:

wp_localize_script( 'updates', 'updates', $update_strings );

You could then access the updates data object in the JS, to get the strings. These strings will only be loaded when updates.js is loaded.

@adamsilverstein
3 years ago

First pass at updating screen reader and title counts after update

#8 @adamsilverstein
3 years ago

  • Keywords dev-feedback added; good-first-bug removed
  • Severity changed from normal to minor
  • Summary changed from Screen reader text isn't updated when plugin update count is updated to Screen reader text (and link title) isn't updated when plugin update count is updated

@paulschreiber thanks for working on this. your patch seemed so very close to working!

its amazing how complicated it is when seeming so simple at first glance. I tried working with your patch and discovered, as @pento pointed out, that the count is cached and not updated at the point you were regenerating the title string.

29022.diff takes a different approach, I think this is what pento was talking about?

  • localize the singular and plural forms of 'plugin', 'theme', and 'translation'
  • pass the individual counts to the JavaScript as data attributes of a hidden span
  • decrement the appropriate count based on upgradeType
  • reconstruct the title/screen reader screen string based on these counts and translated words

I think the patch works correctly, though it needs more testing. Its also a bit verbose with a bunch of repeated code that should be refactorable into something a bit more succinct. @paulschreiber since you are already familiar with the code here, can you take a look at the patch to improve & test?

Thanks!

#9 follow-up: @pento
3 years ago

attachment:29022.diff is closer to what I had in mind. There are a few changes that it'll need:

The strings need to keep the %d (ie, "%d Plugin Updates"). Some languages will move the number to a different location in the string, so we can't just do a string concat in the JS. Instead, we can search and replace the %d, like what already happens in nav-menu.js.

Because the strings are used in update.js, not plugin-install.js, the $scripts->localize() should be for install.

Instead of including the numbers in a hidden span, they can also be included as part of the $scripts->localize() call. This is the standard method for passing this kind of data.

#10 in reply to: ↑ 9 @adamsilverstein
3 years ago

Thanks for the detailed feedback! That all makes sense... i think we are getting closer :)

I knew there was something wrong with the translation approach, couldn't remember why so thanks for explaining that detail.

Will work on an updated patch!

Replying to pento:

attachment:29022.diff is closer to what I had in mind. There are a few changes that it'll need:

The strings need to keep the %d (ie, "%d Plugin Updates"). Some languages will move the number to a different location in the string, so we can't just do a string concat in the JS. Instead, we can search and replace the %d, like what already happens in nav-menu.js.

Because the strings are used in update.js, not plugin-install.js, the $scripts->localize() should be for install.

Instead of including the numbers in a hidden span, they can also be included as part of the $scripts->localize() call. This is the standard method for passing this kind of data.

@adamsilverstein
3 years ago

update counts after upgrades

#11 @adamsilverstein
3 years ago

Thanks to paulschreiber for working thru the details and testing with me at #wcnyc2014.

In 29022.2.diff:

  • pass counts to JavaScript via wp_localize_script
  • parseInt on counts so comparison !== 0 works correctly
  • localize updates.js, not plugin-install.js
  • localize strings with %d, then replace with counts in JavaScript
  • track decremented counts, so updating works correctly when updating multiple items

also:

  • add a translation count to initial title/screen-reader values

Appreciate testing/review... thanks!

#12 @pento
3 years ago

This is starting to shape up nicely. Good work on finding each other at WCNYC! :-)

I think each of the count decrement / string generation chunks could be merged into a single loop - the code is all the same, just with different variable names, and a special case for wordpress updates only having the single string. We could also remove reduceCount() if its functionality is reduced to a single use.

Now that we have the count data stored on updatesL10n, I think we could also use that for the bits at the start of decrementCount() - it's better to use a canonical data source than to parse a number from the HTML.

#13 @adamsilverstein
3 years ago

All good points, thanks again for the feedback. I will iterate on this and post an updated patch.

@adamsilverstein
3 years ago

fine tuning/refactoring

@adamsilverstein
3 years ago

remane variable title to screenReaderText - thats what its for

@adamsilverstein
3 years ago

always parseInt on localized ints

#14 @adamsilverstein
3 years ago

In 29022.5.diff:

  • roll count updates/screen reader text into loop
  • change variable name from 'title' to 'screenReaderText' (we're removing these titles eventually, right?)
  • jshint corrections - remove unused variables, add /* global */ line for localized variable

decrementCount funciton got removed as well, since its one line in the loop; WordPress updates treated the same as the rest, except singular and plural are the same, since there can't be a plural WordPress update count.

This is getting better, I'd love to remove all those parseInt's necessitated by wp_localize_script's annoying conversion of ints to strings. Hopefully, we can get #25280 in soon!

#15 @pento
3 years ago

  • Keywords 4.1-early added; dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release

Looking good!

For this line:

screenReaderText += ( 1 === parseInt( updatesL10n[ updateItem.count ], 10 ) ? updateItem.singular : updateItem.plural ).replace( '%d', updatesL10n[ updateItem.count ] );

I'd be inclined to expand it to an if...else statement, or at least put the .replace() on a second line. This use of the ternary operator is probably a little too clever. :-)

Given that #25280 is tagged for 4.1-early, I think it'd be easier to do the same for this ticket, too, and just lose the parseInt() calls.

#16 @SergeyBiryukov
3 years ago

Some languages have multiple plural forms (see #13651), so we should use _n() in script-loader.php instead of hardcoding just one plural form.

Can we reuse the existing strings from wp_get_update_data(), without introducing new ones?

#17 follow-up: @pento
3 years ago

  • Keywords 2nd-opinion added; 4.1-early removed

That's a good point, I think we'll need a different approach to handle plurals properly.

We have a few options, but I'm not sure which is the least insane:

  • Add Jed to core. We can use this to properly handle the multiple plural forms, with minimum modification to existing code.
  • After an update has happened, manipulate the appropriate transient that wp_get_update_data() uses to generate the string, then WP_Upgrader_Skin::decrement_update_count can call wp_get_update_data(), send the string to the browser, then the string can be updated easily.
  • Same as above, but call wp_version_check()/wp_update_plugins()/wp_update_themes() to update the transients.

Alternatively, we can go for the easier option of just setting the string to "Updates available", and not bothering trying to generate an accurate string.

#18 in reply to: ↑ 17 ; follow-up: @adamsilverstein
3 years ago

Jed looks cool and worth adding - we are going to face this issue with increasing frequency as we add more JavaScript/Backbone to core.

The problem with relying on the php translation function is that the updates occur in an iframe and there can be multiple updates, the JavaScript runs on the page without a refresh. Hitting the php would likdely require an ajax call.

I like the last, simplest option: reconstruct the string so we don't have to worry about plurals. Not sure this works correctly in all languages, I'm ok with that even if the resulting string isn't perfectly grammatically correct. The code would be greatly simplified and the resulting string would still convey the important information.

Something like:

Updates available - Themes: 3, Plugins: 1, Wordpress: 1

Even when only one update remains, the string still makes perfect sense:

Updates available - Themes: 1

Doing it this way eliminates the translations entirely, what do you think?

Replying to pento:

That's a good point, I think we'll need a different approach to handle plurals properly.

We have a few options, but I'm not sure which is the least insane:

  • Add Jed to core. We can use this to properly handle the multiple plural forms, with minimum modification to existing code.
  • After an update has happened, manipulate the appropriate transient that wp_get_update_data() uses to generate the string, then WP_Upgrader_Skin::decrement_update_count can call wp_get_update_data(), send the string to the browser, then the string can be updated easily.
  • Same as above, but call wp_version_check()/wp_update_plugins()/wp_update_themes() to update the transients.

Alternatively, we can go for the easier option of just setting the string to "Updates available", and not bothering trying to generate an accurate string.

#19 in reply to: ↑ 18 @pento
3 years ago

Jed is super useful, we use it a lot on WP.com. I'm not sure that there's a feature that we need it for in Core, though.

Replying to adamsilverstein:

Something like:

Updates available - Themes: 3, Plugins: 1, Wordpress: 1

This looks like a reasonable string in English, but I agree that we should check if it will work in other languages, first.

@pento
3 years ago

@pento
3 years ago

#20 @pento
3 years ago

  • Keywords 2nd-opinion removed

Instead of adding a new string, or a new way of generating the string, attachment:29022.7.diff reuses the PHP string generator after each item has been updated, by subtracting a non-permanent cache from the number of items to updated.

This feels pretty close to the "lightest touch" change that we could make to fix this bug.

#21 @afercia
2 years ago

Tried to test the patch (didn't apply fully) and noticed there's a small detail to consider regarding the final string, see for example the screenshot below:

https://cldup.com/QOgvTZkBOI.png

in this case, the updates total "2" and the number of specific updates will be printed in a string that doesn't have any separation between the two. Screen readers will read out, literally:

twentyone Plugin Update [pause] 1 Theme Update

Tested with Firefox + NVDA.

#23 @afercia
20 months ago

Related: #33030.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


9 months ago

Note: See TracTickets for help on using tickets.