Make WordPress Core

Opened 11 years ago

Closed 3 years ago

#29022 closed defect (bug) (fixed)

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

Reported by: pento's profile pento Owned by: joedolson's profile joedolson
Milestone: 5.9 Priority: normal
Severity: minor Version: 3.9
Component: Upgrade/Install Keywords: has-patch commit
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 (14)

29022.patch (516 bytes) - added by paulschreiber 11 years ago.
29022.wip.patch (5.6 KB) - added by paulschreiber 11 years ago.
29022.diff (6.0 KB) - added by adamsilverstein 11 years ago.
First pass at updating screen reader and title counts after update
29022.2.diff (5.7 KB) - added by adamsilverstein 11 years ago.
update counts after upgrades
29022.3.diff (6.3 KB) - added by adamsilverstein 11 years ago.
fine tuning/refactoring
29022.4.diff (6.4 KB) - added by adamsilverstein 11 years ago.
remane variable title to screenReaderText - thats what its for
29022.5.diff (6.4 KB) - added by adamsilverstein 11 years ago.
always parseInt on localized ints
29022.6.diff (6.0 KB) - added by pento 10 years ago.
29022.7.diff (6.0 KB) - added by pento 10 years ago.
updates initial.jpg (143.3 KB) - added by afercia 6 years ago.
visible text count and screen reader text counts initially synced
updates desync.jpg (112.5 KB) - added by afercia 6 years ago.
desynced
comments desync.jpg (74.4 KB) - added by afercia 6 years ago.
comments count desynced
29022.8.diff (8.9 KB) - added by afercia 6 years ago.
29022.9.diff (1013 bytes) - added by sabernhardt 4 years ago.

Download all attachments as: .zip

Change History (56)

#1 @pento
11 years ago

  • Keywords good-first-bug added

#2 @paulschreiber
11 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
11 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
11 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
11 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
11 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
11 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
11 years ago

First pass at updating screen reader and title counts after update

#8 @adamsilverstein
11 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
11 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
11 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
11 years ago

update counts after upgrades

#11 @adamsilverstein
11 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
11 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
11 years ago

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

@adamsilverstein
11 years ago

fine tuning/refactoring

@adamsilverstein
11 years ago

remane variable title to screenReaderText - thats what its for

@adamsilverstein
11 years ago

always parseInt on localized ints

#14 @adamsilverstein
11 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
11 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
11 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
11 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
11 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
11 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
10 years ago

@pento
10 years ago

#20 @pento
10 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
10 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.

#22 @afercia
9 years ago

Related: #26562

#23 @afercia
9 years ago

Related: #33030.

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


8 years ago

#26 @afercia
6 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone set to Future Release
  • Status changed from new to reopened

Reopening because this is still an issue to address, as the information provided on this link is incorrect (see screenshots below). See also the related #33030 and https://core.trac.wordpress.org/ticket/26562#comment:18

Worth noting also the Comments screen-reader-text suffers from the same bug: while the visible count does get updated, the screen reader text doesn't so the information provided to users is desynced and incorrect.

@afercia
6 years ago

visible text count and screen reader text counts initially synced

@afercia
6 years ago

desynced

@afercia
6 years ago

comments count desynced

#27 @afercia
6 years ago

its amazing how complicated it is when seeming so simple at first glance

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.

@pento @adamsilverstein looking into this, I'd be totally in favor of a drastic simplification. I'd propose to always use a visible text "Updates". As a user, I don't need over-detailed information in the hidden text. For example, when all update types are present, the string would be something like this:

1 WordPress Update, 1 Plugin Update, 2 Theme Updates, Translation Updates

That's redundant and adds a lot of noise. I wouldn't need the count either: I just need to know there are updates. The details are already available in the Updates page. Removing the count entirely and making the text always visible would also make this link accessible to speech input users.

http://cldup.com/aeKtZl6Vth.png

Worth remembering this link is not visible when there are no updates.

I've noticed there's a filter for the various counts and titles, not sure if it's possible to remove it: not sure if plugins or themes actually use it.

If there's consensus, I'd like to explore this approach.

#28 @afercia
6 years ago

In #33030 I've tried a similar approach, see https://core.trac.wordpress.org/attachment/ticket/33030/33030-updates.diff

Of course, it doesn't work when updating from the Updates page, for the same iframe problem faced here. I'd be glad to move that proof of concept patch to this ticket to see if the iframe issue can be worked around. I'm not sure the methods tried in 29022.6.diff and 29022.7.diff still work. Any help to move this ticket on would be greatly appreciated :)

/Cc @pento @adamsilverstein

#29 @adamsilverstein
6 years ago

looking into this, I'd be totally in favor of a drastic simplification. I'd propose to always use a visible text "Updates". As a user, I don't need over-detailed information in the hidden text.

I'm also in favor of this type of simplification as long as we still provide the essential information the user needs.

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


6 years ago

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


6 years ago

@afercia
6 years ago

#32 @afercia
6 years ago

  • Keywords has-patch added; needs-patch removed

[44924] updated the Comments counts, using the data from the AJAX response.

Taking inspiration from there, 29022.8.diff uses the data from the AJAX response as well. This works for the updates that happen from the Plugins or Themes pages. It doesn't work for update.php for the same iframe issue faced here.

I've tried the solution explored in the previous patches but I wasn't able to make them work. Not sure they can work, actually. However, I'd appreciate some eyes on 29022.8.diff: it solves the problem only partially but it's better than what we have now. /Cc @pento

#33 @sabernhardt
4 years ago

  • Keywords needs-refresh added

#34 @sabernhardt
4 years ago

If 26562.2.patch is chosen for #26562, the title attribute would be gone, but the screen reader text still would need updating with the script.

@sabernhardt
4 years ago

#35 @sabernhardt
4 years ago

  • Keywords needs-testing added; needs-refresh removed

#36 @sabernhardt
3 years ago

  • Milestone changed from Future Release to 5.9
  • Owner set to sabernhardt
  • Status changed from reopened to accepted

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


3 years ago

#38 @joedolson
3 years ago

  • Owner changed from sabernhardt to joedolson

#39 @joedolson
3 years ago

@sabernhardt The refreshed patch you added only contains a small fraction of the code from the prior patch. I know that the title attribute manipulation is no longer needed, but I'm pretty confident that this updated patch is incomplete; if not, can you give an explanation of the changes?

#40 @sabernhardt
3 years ago

Yes, an explanation would help here.

The toolbar link no longer includes either the title attribute or the itemized list of updates that were in the attribute. So this patch should correct the total count in the screen reader text there, plus it can delete the now-unnecessary line that had removed the title attribute in the JS file.

The previous patch included changing the itemized list text, and it proposed adding that list in the side menu's Updates link.

Of course, it's still possible that I missed something important.

#41 @joedolson
3 years ago

  • Keywords commit added; needs-testing removed

Given all the removals since the original patch, that seems to make sense. Certainly has become a much, much simpler patch since the original version.

Tested and it seems to do the job just fine; marking for commit.

#42 @joedolson
3 years ago

The patch omits the change to the adminbar text to say 'Updates', but I think that's intentional, given that everything else has been simplified. There are a lot of icon-primary links in the adminbar, and I don't think changing this one as part of this ticket is sensible; it should be done more systematically if that's going to change.

#43 @joedolson
3 years ago

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

In 52139:

Upgrade/Install: Update screen reader text counts in adminbar.

Update the hidden screen-reader-text update count in the adminbar when updates are run on the plugin or theme screens.

Props pento, paulschreiber, adamsilverstein, afercia, SergeyBiryukov, sabernhardt.
Fixes #29022.

Note: See TracTickets for help on using tickets.