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

  1. Create command
  2. Create tests
  3. Manually test

To manually test:

  1. apply the patch
  2. enable the test module 'ajax_forms_test'.
  3. goto */ajax_forms_test_ajax_commands_form*
  4. click the button *AJAX 'Announce': Click to announce*
  5. 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Status: Active » Needs review
FileSize
6.55 KB

Here it is, with tests!

tedbow’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 2: 3020352-announce-command.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.49 KB
6.69 KB

\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.

andrewmacpherson’s picture

Title: Create a new AnnounceCommand to call Drupal.annouce on an AjaxResponse » Create a new AnnounceCommand to call Drupal.announce on an AjaxResponse

typi in title

tedbow’s picture

re

typi in title

Thanks for fixing but also 😂

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.5 KB
7.1 KB
171.98 KB

Found 3 nits, which I fixed. More importantly, I did manual testing:

  1. Installed https://www.drupal.org/project/devel_a11y, which makes announcements show up in the browser console whenever they're made.
  2. Using the Toolbar and CKEditor config UI still resulted in the expected announcements.
  3. Crucially, I installed ajax_forms_test, went to http://d8/ajax_forms_test_ajax_commands_form, clicked the 'announce' button, and it worked:
tedbow’s picture

@Wim Leers thanks for the review and nit fixes!

alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record, +Needs release notes snippet, +8.7.0 release notes

We need a change record to tell devs and themers about this. Plus a release notes snippet.

tedbow’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Created CR https://www.drupal.org/node/3021276
and add release note snippet here

lauriii’s picture

Issue tags: -Needs change record, -Needs release notes snippet
lauriii’s picture

Status: Reviewed & tested by the community » Needs review

Should we allow setting the announcement priority as well?

Wim Leers’s picture

Should we allow setting the announcement priority as well?

Ah, yes. Very good point. Let's do that. There's little point in not doing that.

Wim Leers’s picture

Status: Needs review » Needs work

@lauriii++

tedbow’s picture

Status: Needs work » Needs review
FileSize
8.73 KB
10.74 KB

@lauriii good call.

  1. I added priority as a string argument that defaults 'polite'. I thought about making it a boolean argument $is_assertive because currently Drupal.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 override Drupal.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.

  2. I added a test to make sure if you include 2 instances of 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.
GrandmaGlassesRopeMan’s picture

Status: Needs review » Needs work
Issue tags: +JavaScript
  1. +++ b/core/misc/ajax.es6.js
    @@ -1335,6 +1335,24 @@
    +    announce(ajax, response, status) {
    

    `status` is passed but we're never using it, same with `ajax`.

  2. +++ b/core/misc/ajax.es6.js
    @@ -1335,6 +1335,24 @@
    +      Drupal.announce(response.text, response.priority);
    

    Can we be more careful here with accessing object properties? Both `text` and `priority` could be missing, let's set some sensible defaults.

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
10.87 KB

@drpal thanks for review

  1. I remove 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 removing status.
  2. Yep, providing defaults for these.

Status: Needs review » Needs work

The last submitted patch, 18: 3020352-18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.01 KB
11.75 KB

fix unit test

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! Looks good for me now.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

This looks good to me overall. One point:

+++ b/core/lib/Drupal/Core/Ajax/AnnounceCommand.php
@@ -0,0 +1,71 @@
+   * @param string $text
+   *   The text to be announced.
...
+  public function __construct($text, $priority = AnnounceCommand::PRIORITY_POLITE) {
+    $this->text = $text;
+    $this->priority = $priority;
+  }

+++ b/core/misc/ajax.es6.js
@@ -1335,6 +1335,22 @@
+     * @param {string} [response.text='']
+     *   The text that will be read.
...
+    announce(ajax, { text = '', priority = 'polite' }) {
+      Drupal.announce(text, priority);
+    },

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!

tedbow’s picture

@xjm thanks for the review.

  1. 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 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 use debounce() 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. As new 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 calling Drupal.announce() directly anyways.

  2. I am also removing default 'polite' value for the $priority from the server command. This logic is already taken into account in 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.

  3. I have update unit tests but the functional test remain same because remove the default for priority has no effect since it is already in effect on the client.
tedbow’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Changes make sense, thanks for the thorough explanation!

  1. +++ b/core/lib/Drupal/Core/Ajax/AnnounceCommand.php
    @@ -40,10 +40,10 @@ class AnnounceCommand implements CommandInterface, CommandWithAttachedAssetsInte
    +   *   (optional)The priority that will be used for the announcement.
    

    Missing space after (optional), and it should explain what the default is/does

  2. +++ b/core/misc/ajax.es6.js
    @@ -1173,7 +1173,7 @@
    -   * Provide a series of commands that the client will perform.
    +   * Provide a f
    

    ?

tedbow’s picture

@tim.plunkett thanks the review
Fixed both in #25

andrewmacpherson’s picture

Re #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?

tedbow’s picture

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.

Yes 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.

Can this work with a coin-toss button, where we might respond with heads or tails announcements?

Yep, this can react to any condition on the server-side.

andrewmacpherson’s picture

Cool. I think this will be a really useful API addition.

phenaproxima’s picture

Status: Needs review » Needs work

Looks great! Really useful addition, I think, and certainly an accessibility win :) All I found is minor stuff and nitpicks.

  1. +++ b/core/lib/Drupal/Core/Ajax/AnnounceCommand.php
    @@ -0,0 +1,77 @@
    +  /**
    +   * The assertive priority attribute value.
    +   */
    +  const PRIORITY_ASSERTIVE = 'assertive';
    +
    +  /**
    +   * The polite priority attribute value.
    +   */
    +  const PRIORITY_POLITE = 'polite';
    

    Despite being constants, aren't these supposed to be annotated @var string?

  2. +++ b/core/lib/Drupal/Core/Ajax/AnnounceCommand.php
    @@ -0,0 +1,77 @@
    +   * @param string|null $priority
    +   *   (optional) The priority that will be used for the announcement. Defaults
    +   *   to NULL which will not set a 'priority' in the response sent to the
    +   *   client and therefore the JavaScript Drupal.announce() default of 'polite'
    +   *   will be used for the message.
    

    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"?

  3. +++ b/core/modules/system/tests/modules/ajax_forms_test/ajax_forms_test.module
    @@ -43,6 +44,55 @@ function ajax_forms_test_advanced_commands_alert_callback($form, FormStateInterf
    +/**
    + * Ajax form callback: Selects 'announce' with no priority specified.
    + */
    +function ajax_forms_test_advanced_commands_announce_callback($form, FormStateInterface $form_state) {
    +  return _ajax_forms_test_create_announce_response('Default announcement.');
    +}
    

    Nit: Why not move these callback functions to the form builder class?

  4. +++ b/core/modules/system/tests/modules/ajax_forms_test/ajax_forms_test.module
    @@ -43,6 +44,55 @@ function ajax_forms_test_advanced_commands_alert_callback($form, FormStateInterf
    +/**
    + * Create an AjaxResponse with an AnnounceCommand.
    + *
    + * @param string $announcement
    + *   The announcement.
    + * @param string|null $priority
    + *   (optional)The priority.
    + *
    + * @return \Drupal\Core\Ajax\AjaxResponse
    + *   The AjaxResponse with an AnnounceCommand instance.
    + */
    +function _ajax_forms_test_create_announce_response($announcement, $priority = NULL) {
    +  $response = new AjaxResponse();
    +  if ($priority) {
    +    return $response->addCommand(new AnnounceCommand($announcement, $priority));
    +  }
    +  return $response->addCommand(new AnnounceCommand($announcement));
    +}
    

    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(...)).

  5. +++ b/core/tests/Drupal/Tests/Core/Ajax/AjaxCommandsTest.php
    @@ -78,6 +80,60 @@ public function testAlertCommand() {
    +  public function testAnnounceCommand($message, $priority, $expected) {
    

    Nit: $expected should be type hinted as an array.

  6. +++ b/core/tests/Drupal/Tests/Core/Ajax/AjaxCommandsTest.php
    @@ -78,6 +80,60 @@ public function testAlertCommand() {
    +  public function announceCommandProvider() {
    +    return [
    +      'no priority' => [
    +        'Things are going to change!',
    +        NULL,
    +        [
    +          'command' => 'announce',
    +          'text' => 'Things are going to change!',
    +        ],
    +      ],
    +      'polite priority' => [
    +        'Things are going to change!',
    +        'polite',
    +        [
    +          'command' => 'announce',
    +          'text' => 'Things are going to change!',
    +          'priority' => 'polite',
    +        ],
    +
    +      ],
    +      'assertive priority' => [
    +        'Important!',
    +        'assertive',
    +        [
    +          'command' => 'announce',
    +          'text' => 'Important!',
    +          'priority' => 'assertive',
    +        ],
    +      ],
    +    ];
    

    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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.47 KB
11.53 KB

re #30 @phenaproxima thanks for review

  1. Fixed
  2. I know this not ideal but I think we should let php developers know that if they don't provide $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 pass AnnounceCommand::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
  3. All of the other callback for the other commands are done this way so I think this makes it more readable to have all done the same way so you don't have to look at some of the callbacks on the form class and some on the .module file.
  4. Ok I removed _ajax_forms_test_create_announce_response(), yes I think this is better.
  5. fixed
  6. fixed
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Blocks-Layouts

All of the feedback has been addressed, and sign-offs have been given.
Great work!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 31: 3020352-31.patch, failed testing. View results

phenaproxima’s picture

Issue tags: +Needs reroll

Sadness!

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
11.51 KB

Failed with git apply but cleanly applied with git apply -3

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Accessibility +accessibility

All the changes since I last reviewed this look great!

+++ b/core/lib/Drupal/Core/Ajax/AnnounceCommand.php
@@ -0,0 +1,81 @@
+class AnnounceCommand implements CommandInterface, CommandWithAttachedAssetsInterface {

The fact that CommandWithAttachedAssetsInterface does not extend CommandInterface has tripped me up every time I've reviewed this issue, but turns out it's even more confusing than I thought, because...

+++ b/core/lib/Drupal/Core/Ajax/AnnounceCommand.php
@@ -0,0 +1,81 @@
+  public function getAttachedAssets() {
+    $assets = new AttachedAssets();
+    $assets->setLibraries(['core/drupal.announce']);
+    return $assets;
+  }

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 and InsertCommand. And for extra confusion, those also doesn't actually implement the method; it instead uses CommandWithAttachedAssetsTrait which implements the method on its behalf plus do a bunch of other stuff. The trait's getter does:

  public function getAttachedAssets() {
    return $this->attachedAssets;
  }

However, the trait also provides getRenderedContent(), which is not on the interface, but does:

  protected function getRenderedContent() {
    $this->attachedAssets = new AttachedAssets();
    if (is_array($this->content)) {
      $html = \Drupal::service('renderer')->renderRoot($this->content);
      $this->attachedAssets = AttachedAssets::createFromRenderArray($this->content);
      return $html;
    }
    else {
      return $this->content;
    }
  }

(...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?

xjm’s picture

Also, 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.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

Created #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 in CommandWithAttachedAssetsInterface 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

xjm’s picture

Status: Reviewed & tested by the community » Needs work

OK, 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 call parent::getAttachedAssets() and add theirs to that.

Was about to commit this, but we need a coding standards cleanup in aisle 2:

FILE: ...ules/system/tests/modules/ajax_forms_test/ajax_forms_test.module
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 45 | ERROR | [x] Expected 1 blank line after function; 2 found
----------------------------------------------------------------------

If you could also eslint that I'd appreciate it since my JS linting is busted atm. Thanks!

tedbow’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
563 bytes
11.52 KB

@xjm

OK, 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.

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.

If you could also eslint that I'd appreciate it since my JS linting is busted atm. Thanks!

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

  • xjm committed d08335f on 8.7.x
    Issue #3020352 by tedbow, Wim Leers, tim.plunkett, xjm, andrewmacpherson...
xjm’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -8.7.0 release notes

Committed 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.

xjm’s picture

Issue tags: +8.7.0 highlights

Upon 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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

andrewmacpherson’s picture

Issue tags: -accessibility (duplicate tag) +Accessibility

fixing accessibility tag