-
Notifications
You must be signed in to change notification settings - Fork 328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor character count to inject new element #2577
Conversation
c0df273
to
bd5e75e
Compare
Regarding #2487: My intended approach for this was to debounce the update of the character counter text until after the user had stopped typing for a period of time (like 300 milliseconds). However, this approach conflicts with the existing Dragon Naturally Speaking bugfix; which runs a Debouncing the character counter update also makes for a bit more of a jarring experience for sighted users, as the counter text will appear to "lag" behind their typing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had a look at the code and left 2 little comments. Looking sharp 👍🏻
I'd also add to consider squashing your commits either now or once it's out of draft as they're close enough to sit under a single commit and we're otherwise knowingly introducing a test failure into our commit history.
@owenatgov Ta! I've squashed those first two to remove the failing test from history. Would further commits to this branch still be separate, or is it normal to squash when the PR is completed? Wondering how the new guidance about making atomic commits (alphagov/gds-way#696) plays out. |
dd58ee8
to
cd61600
Compare
@querkmachine It's a tricky one. I generally play it by ear depending on how many "things" the code I'm writing includes. Things in this context being changes to the code that we could reliably ship, big or small, even if they're part of a wider bit of work so it makes sense to chunk them into one PR. In this case my advice would be if you end up adding anything to this that isn't covered by the original commit message, make it a new commit. I think how you've split it out so far makes sense. |
Again regarding #2487, I've considered a couple of approaches to trying to fix this:
I'm open to any other approaches that people come up with |
Testing this PR in various AT/browser combinations (particularly those the issues were initially identified in). Checked items work as expected or within an acceptable level, such as having non-persistent or relatively minor issues (in my opinion). Any combinations not listed haven't been tested in.
|
cd61600
to
c57d8b5
Compare
I've applied the suggestion from @36degrees (here: #2487 (comment)) for debouncing the character counter update. The counter is now not updated until 250 milliseconds after the user has stopped typing, which (mostly) successfully stops screen readers from queuing up multiple updates or attempting to read them whilst a user is still typing. The delay before the update is made may still need tweaking based on testing. There is a need to strike a balance between immediacy of feedback for users of the visual counter and that of screen reader users. The debounced nature of the counter now means that the tests fail too. Gotta fix that. |
Have discussed some of the issues that came up in testing and we have decided that none of these are blockers (they are all equivalent to or improvements on the existing component, despite the flaws. Having these be documented as known issues would be useful. The VoiceOver/Safari issue with the counter being read out twice has been opened as #2587. The JAWS/Chrome issue with textareas with default values has been opened as #2585. |
@querkmachine do the visual updates and screen reader updates need to be tied together? Perhaps they should just be two independent elements? One hidden from screen readers, the other visually hidden. |
@edwardhorsford They don't have to be tied, strictly speaking. I discussed the idea previously with David, our accessibility specialist (mentioned in this comment), but we were worried that having multiple updating elements—some visible and others not, some debounced and others not—would be making the component JS more complex than is strictly necessary. The change to how the visual counter updates hasn't undergone any user testing, and we were considering splitting it out if we felt it was necessary. Do you feel like it might? |
@querkmachine is there a review app I could try? My worry is it might impact faster typists - as 250ms would be quite a while for and you might end up with an extended period without update. |
Decided no action on outstanding point.
Tested with the latest changes (including dd564e3 ) and this seems like a huge improvement on the issues we'd previously noticed! Unfortunately I don't have access to an iOS or stock Android device to test on mobile. Spreadsheet of testing results here: https://docs.google.com/spreadsheets/d/1i2oexwBY-xs-HmRTce9xiOSX6RYBJigeRcRQwlMiCU8/edit#gid=2015583909 |
@querkmachine It would be good if we could get this tested in Android and iOS and record it on that spreadsheet. You might need to ask around for someone with an Android device to test 🤔 |
Have updated testing spreadsheet with results for VoiceOver / iOS 15.3 and Talkback / Android 11. Can additionally confirm that the counter seems to work with the built-in voice inputs for each OS, too. |
Checking the latest review app, the responsiveness (for a fast typist) is looking much better. |
fdd1503
to
a876f41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work, this looks really good 🎉
Modifies the character count JavaScript to compose and inject new elements that are live-updated, instead of overwriting the content of the initial HTML hint.
There are now three elements responsible for character count announcements:
aria-hidden
to hide itself from screen readers.aria-live="polite"
.Only the first element must be present in the initial HTML. The other two are injected when JavaScript runs.
Upgrading
Users of the govuk-frontend Nunjucks macros do not have to make any changes.
Users of static HTML can remove the hardcoded
aria-live="polite"
attribute from.govuk-character-count__message
. We haven't found any negative repercussions to it still being present, but it is not needed as this element's content no longer updates.For backwards compatibility, custom classes that have been added to the HTML count element are copied onto the visible, injected counter. If you previously used a custom class to change the visibility of the counter outside of the existing threshold functionality, this may no longer work as expected. The screen reader only counter does not inherit any classes.
Why these changes have been made
These changes are intended to resolve a number of issues that came up in a recent investigation into how the character count interacted with more recent versions of screen readers: #2539
Should resolve:
Current known issues