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: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (56)
#3
@
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
@
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
@
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
@
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
@
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.
#8
@
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:
↓ 10
@
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
@
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
, notplugin-install.js
, the$scripts->localize()
should be forinstall
.
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.
#11
@
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
@
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
@
11 years ago
All good points, thanks again for the feedback. I will iterate on this and post an updated patch.
#14
@
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
@
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
@
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:
↓ 18
@
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, thenWP_Upgrader_Skin::decrement_update_count
can callwp_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:
↓ 19
@
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, thenWP_Upgrader_Skin::decrement_update_count
can callwp_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
@
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.
#20
@
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
@
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:
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.
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
8 years ago
#26
@
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.
#27
@
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.
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
@
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
@
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
#32
@
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
#34
@
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.
#36
@
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
#39
@
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
@
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
@
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
@
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.
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?).