Problem/Motivation
In #2995689: Allow reordering blocks without a pointer device we have the need to call Drupal.announce() to inform screen reader users that block list has been updated. Currently this patch has no javascript changes.
We currently have \Drupal\Core\Ajax\AlertCommand
but no way to call Drupal.annouce()
directly via an Ajax response.
Proposed resolution
Create \Drupal\Core\Ajax\AnnouceCommand
that would call Drupal.announce()
This would make it easy to use Drupal.announce and encourage to its use when an ajax response should notify screen reader users of an update.
Remaining tasks
Create commandCreate tests- Manually test
To manually test:
- apply the patch
- enable the test module 'ajax_forms_test'.
- goto */ajax_forms_test_ajax_commands_form*
- click the button *AJAX 'Announce': Click to announce*
- Confirm the message appears in HTML
<div id="drupal-live-announce" class="visually-hidden" aria-live="polite" aria-busy="false">This is my announcement.</div>'
User interface changes
none
API changes
none
Data model changes
None
Release notes snippet
The new Ajax command AnnounceCommand is now available to trigger screen reader announcements directly in Ajax responses. This command can be added to a \Drupal\Core\Ajax\AjaxResponse object like any other command that implements \Drupal\Core\Ajax\CommandInterface. The announcements will be made via the Drupal.announce()
JavaScript method.
Comment | File | Size | Author |
---|---|---|---|
#40 | 3020352-40.patch | 11.52 KB | tedbow |
#40 | interdiff-35-40.txt | 563 bytes | tedbow |
#35 | 3020352-announce-34.patch | 11.51 KB | tim.plunkett |
#31 | 3020352-31.patch | 11.53 KB | tedbow |
#31 | interdiff-30-31.txt | 4.47 KB | tedbow |
Comments
Comment #2
tedbowHere it is, with tests!
Comment #3
tedbowComment #5
tedbow\Drupal\Tests\big_pipe\FunctionalJavascript\BigPipeRegressionTest::testMultipleClosingBodies_2678662
failed because it didn't expect the #drupal-live-announce element. I started to fix that test but then found\Drupal\Core\Ajax\CommandWithAttachedAssetsInterface
.This allows the command to attach the core/drupal.announce library itself. So then we don't need to make it core/drupal.ajax depend on it.
Comment #6
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedtypi in title
Comment #7
tedbowre
Thanks for fixing but also 😂
Comment #8
Wim LeersFound 3 nits, which I fixed. More importantly, I did manual testing:
ajax_forms_test
, went tohttp://d8/ajax_forms_test_ajax_commands_form
, clicked the 'announce' button, and it worked:Comment #9
tedbow@Wim Leers thanks for the review and nit fixes!
Comment #10
alexpottWe need a change record to tell devs and themers about this. Plus a release notes snippet.
Comment #11
tedbowCreated CR https://www.drupal.org/node/3021276
and add release note snippet here
Comment #12
lauriiiComment #13
lauriiiShould we allow setting the announcement priority as well?
Comment #14
Wim LeersAh, yes. Very good point. Let's do that. There's little point in not doing that.
Comment #15
Wim Leers@lauriii++
Comment #16
tedbow@lauriii good call.
$is_assertive
because currentlyDrupal.announce()
only supports 'polite' and 'assertive'. But there is not reason we couldn't support support more values in the future. I think the spec supports 'off' but it might support more values in the future. If it did and our argument was boolean we would be stuck or would have to make another argument. Also someone could overrideDrupal.announce()
.Drupal.announce()
actually always uses 'polite' unless the priority is set to 'assertive'. So server side implementation actually uses the same logic as the JavaScript method itself. Only explicitly using 'assertive' priority will make difference.AnnounceCommand
both messages are included.I reload the page each time I tested a new priority because theoretically if the ajax response was very quick the messages would be in the same div. But usually because of the delay the div would be replaced(but each would be read). This is because how the JS method is written to use debounce. This timing issue could cause random failures so I reload with each new response.
Comment #17
GrandmaGlassesRopeMan`status` is passed but we're never using it, same with `ajax`.
Can we be more careful here with accessing object properties? Both `text` and `priority` could be missing, let's set some sensible defaults.
Comment #18
tedbow@drpal thanks for review
ajax
because I can't change how this method gets called since it is the same for methods that correlate to server-side commands. but removingstatus
.Comment #20
tedbowfix unit test
Comment #21
lauriiiThank you! Looks good for me now.
Comment #22
xjmThis looks good to me overall. One point:
I asked @tedbow why
text
defaulted to an empty string in the JS, and @tedbow pointed out that this is to clear an existing announcement.I think that this behavior needs to be documented if so. Also, it's somewhat strange that the PHP command and the JS method have different defaults. For PHP you could still explicitly pass empty string, so it would probably be worth documenting in both places if we include this behavior, and maybe should consider making the PHP and JS defaults line up.
NW for that. Thanks!
Comment #23
tedbow@xjm thanks for the review.
I investigated this and actually no where in core are we call
Drupal.announce()
with an empty string. I don't think we should introducing a new thing here. Because whether it would actually clear the element would depend on how long it was since the last call. This is because we usedebounce()
to concat calls that are very close together.So I am removing the default on the JS command. This command, like all other commands on
Drupal.AjaxCommands.prototype
should never be called directly from JS except for the logic we have that invoke them from the server. So I think we can safely assume like all other commands, that required arguments on the server will be there. Asnew AnnounceCommand()
with no arguments would cause a server-side error.If you tried to call
Drupal.AjaxCommands.announce(status, {})
from JavaScript with no 'text property on the second argument you should get an error. From JavaScript you should always be callingDrupal.announce()
directly anyways.Drupal.announce()
so there is no need to duplicated it here.If we duplicate it we have to keep it in sync in both places which is there is not need to.
Also if we duplicated on the server if anyone wanted to replace
Drupal.announce()
and have a different default priority they would also have to replace the server command.Comment #24
tedbowComment #25
tim.plunkettChanges make sense, thanks for the thorough explanation!
Missing space after (optional), and it should explain what the default is/does
?
Comment #26
tedbow@tim.plunkett thanks the review
Fixed both in #25
Comment #27
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedRe #23.1 - agreed, the way Drupal.announce works, there's never really a need to clear the messages out. ARIA live regions can in theory be configured to react to deletions using
aria-relevant
, but last time I checked this was poorly implemented in browsers, a11y APIs and AT. In any case, Drupal.announce doesn't use this, so clearing out the div is never necessary.I found another use-case for this feature - when a media item is added/removed in the MediaLibraryWidget, we need to inform the user of the outcome of their action. See #2988431-9: [Meta] Accessibility plan for Media Library Widget
I've a feeling this might help in #3020716: Add vertical tabs style menu to media library too.
Is this flexible enough for AJAX actions where the outcome can vary depending on what the server decides? The test scenarios in the patch are buttons which always give the same response, without any conditional outcomes server-side. I'm supposing that we don't construct the AnnounceCommand until after we know what we need to say. Can this work with a coin-toss button, where we might respond with heads or tails announcements?
Comment #28
tedbowYes this Command and other commands can do this. These are actually already conditional outcomes on the server-side. The form is being to submitted to the same path for all buttons and conditionally determines which function to call.
Yep, this can react to any condition on the server-side.
Comment #29
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedCool. I think this will be a really useful API addition.
Comment #30
phenaproximaLooks great! Really useful addition, I think, and certainly an accessibility win :) All I found is minor stuff and nitpicks.
Despite being constants, aren't these supposed to be annotated
@var string
?I'm not sure that the PHP side should be describing the implementation details of the JavaScript side. Maybe just say "the JavaScript Drupal.announce() default will be used for the message"?
Nit: Why not move these callback functions to the form builder class?
This is a definite nitpick, so feel free to disregard: I'm not sure we need the _ajax_forms_test_create_announce_response() function. It's not really adding much -- we could just as easily have the callback functions do something like
return (new AjaxResponse())->addCommand(new AnnounceCommand(...))
.Nit: $expected should be type hinted as an array.
I think that the priority argument in each of these data sets should use the PRIORITY_* constants from the AnnounceCommand class, and the expected render should use the hard-coded values. That way we're actually testing the constants.
Comment #31
tedbowre #30 @phenaproxima thanks for review
$priority
, polite will be used. They shouldn't have to read the JS(and Drupal.announce() is confusing to read, separate issue). Otherwise PHP developers might not understand it and always passAnnounceCommand::PRIORITY_POLITE
and then if we every change the JS side to have a different default then all calls that passed would have to be updated.AnnounceCommand::PRIORITY_POLITE
_ajax_forms_test_create_announce_response()
, yes I think this is better.Comment #32
tim.plunkettAll of the feedback has been addressed, and sign-offs have been given.
Great work!
Comment #34
phenaproximaSadness!
Comment #35
tim.plunkettFailed with
git apply
but cleanly applied withgit apply -3
Comment #36
xjmAll the changes since I last reviewed this look great!
The fact that
CommandWithAttachedAssetsInterface
does not extendCommandInterface
has tripped me up every time I've reviewed this issue, but turns out it's even more confusing than I thought, because...This method does not do what I'd expect in constructing an empty new
AttachedAssets
and setting libraries on it, rather than accessing a protected property containing the attached assets or something.I looked for core usages of this interface to compare. The only other things to implement the interface in core are
OpenDialogCommand
andInsertCommand
. And for extra confusion, those also doesn't actually implement the method; it instead usesCommandWithAttachedAssetsTrait
which implements the method on its behalf plus do a bunch of other stuff. The trait's getter does:However, the trait also provides
getRenderedContent()
, which is not on the interface, but does:(...without checking if the property was already set or not initially, but writing it based on what looks to be a majorly heavy computation).
So the situation in HEAD is a bit of a mess, but can we clarify whether or not commands should be able to have stative attached assets, and why we made a different choice here in that regard than HEAD?
Comment #37
xjmAlso, why does this need to be mentioned in the release notes? It doesn't break anything AFAIK? It's just a new (useful) API, but a CR is enough for that.
Comment #38
tedbowCreated #3030758: Update documentation in CommandWithAttachedAssetsInterface to clarify that it can be used with commands that don't return render arrays . I don't think we should have to explain in
AnnounceCommand
why we are not using the trait which is separate from the interface.. I think the problem is in the docs inCommandWithAttachedAssetsInterface
which can be updated to just reflect that it is about attaching libraries which what it is used for when we actually use it in\Drupal\Core\Ajax\AjaxResponse::addCommand
Comment #39
xjmOK, after @tedbow walked me through
AjaxResponse
and helped me sort through the confusing/brittle things that the core implementations have, I'm convinced we're doing the right thing here. So long as the only assets an announce command needs is this one library, the implementation we have is fine.AjaxResponse
takes care of collecting and merging together the assets. And if someone wanted to extend the AnnounceCommand to add additional assets, then they could simply callparent::getAttachedAssets()
and add theirs to that.Was about to commit this, but we need a coding standards cleanup in aisle 2:
If you could also eslint that I'd appreciate it since my JS linting is busted atm. Thanks!
Comment #40
tedbow@xjm
Great. This did take a while for me to figure out.
I think after #3030758: Update documentation in CommandWithAttachedAssetsInterface to clarify that it can be used with commands that don't return render arrays it will at least be clearer for other classes using the interface.
yarn lint:core-js-passing
Passes with flying colors!!!!
node ./node_modules/eslint/bin/eslint.js misc/ajax.es6.js
Does have pre-existing problems but none for the code we are adding.
Just to be sure
yarn build:js
Makes no changes to the built JS. So properly built
Also looking at the build artifacts for #35 the eslint command found no problems
Comment #42
xjmCommitted to 8.7.x and published the change record.
Removing the release notes tag because this sort of change does not belong in the release notes. Only disruptive changes that break a public API or might require site owners to change something should be in the release notes.
Comment #43
xjmUpon reflection, I think this is what was intended (instead of the release notes) to highlight the accessibility improvement that Drupal now provides an easy way for developers to add aria announcements in Ajax responses.
Comment #45
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedfixing accessibility tag