Edit Media button within WYSIWYG should include media label for screen readers

As mentioned in comment #107 on #2994702: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`, I would like to see this issue escalated to stable blocker.

Reasoning:

When a screen reader user gets to the link to activate this widget, the text that is read is "edit media."

While this might be sufficient in the event that the user only has one embedded media item, this will be a big problem the moment they have more than one, as "edit media" isn't sufficient to tell them *which* media they are editing.

Suggestion: add the filename at the end so that it reads "edit media [filename]"

Quoting #2994702-59: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`:

RE: media label. I am very very hesitant about this. My concern distilled to the essence: why is this a problem for editing embedded media, but not for links, images, tables, et cetera?

Hm... it may be a problem for those other elements, as well. When I have some time, I'd like to go in and play with the experience of toggling the edit option in these other experiences. It really depends on how contextualized the button is at the moment of activation. Questions to ask include: Do you know exactly where you are? If you pull up a list of the links in your screen reader, can you tell what the link is when going through that list out of the context? Can you distinguish it from others?

In the overall UI, these configuration options toggle buttons are read out by the screen reader with all necessary contextual information. E.g., if you are on the default search block, you hear "open search configuration options toggle button" and if you are on breadcrumbs, you hear "open breadcrumb configuration options toggle button."

Ideally, media (including images) would read the alt text or file name (normally, I would say alt text, but in this case I'm thinking the filename might be more logical and would be interested in others' opinions on this). Tables would read the table caption. And so on...

Agreed! 🙂

It really depends on how contextualized the button is at the moment of activation. Questions to ask include: Do you know exactly where you are? If you pull up a list of the links in your screen reader, can you tell what the link is when going through that list out of the context? Can you distinguish it from others?

So, rather than waiting for @rainbreaw to check this, I just checked it myself :)

1. Create some content with embedded media. That results in this situation + markup:
   
2. Which triggers this VoiceOver narration:
   Edit media, button, Embedded media widget, region
   

The key problem here is that Embedded media widget does not allow you to know exactly where you are.

The good news is that that is just an aria-label, which we can easily change. In fact, CKEditor's own media embed system (which we don't use for reasons to long to get into here, plus out-of-scope) ran into similar issues, which is why that aria-label even exists: https://github.com/ckeditor/ckeditor4/commit/d9873dd5c0bfbc9c25fe612a9a8... — that's why they added the getLabel() method to CKEditor's Widget API.

Now, @phenaproxima was right in #2994702-53: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`, where he wrote:

The hidden text of the edit and remove buttons for an embedded media item should probably include the label of the item, to be more screen reader-friendly. This could pose a few technical difficulties (mostly in terms of getting the label back from the server), so let's open a follow-up for that.

That is exactly the difficulty. Because Media entities are a layer of abstraction, there is no easy, cheap, instantaneous way to get a reasonable label.

Considerations:

1. Why did I say "instantaneous"? Because the rendering of media into something useful happens dynamically: it needs to talk to the server.
2. The request does not pass on media identifiers, it passes some text that contains a media identifier. That's slightly tricky, but definitely doable.
3. Filename indeed seems fine … but what about non-file-based media, such as tweets or YouTube videos? In that case, the Media entity's label would have to do. In fact, that seems a safer choice overall.

So, attached patch:

1. Ensures that a fallback ARIA label is used by default initially in the getLabel() implementation, but it is updated automatically whenever label is being set on the CKEditor Widget instance's data.
2. The server side detects which media is being loaded by using the cache tags being bubbled from the filtered text.
3. It loads the relevant media entity, gets the label, and passes this to the client using a response header.
4. The JS parses this response header and sets it on the CKEditor Widget instance's data

Adding credit for reporter.

@xjm asked in Slack for some triage advice for Media Library. However, is the media embed actually anything to do with the media library? The patch here doesn't touch the media library module. I haven't had time to fully review or test the patch in #8. It looks like it's proving tricky?

Distinguishing the edit buttons is highly desirable. I'd say should-have, while Rain thinks must-have (in #2). Hey-ho.

Let's say "strong should-have". If it's proving tricky to achieve it needn't be a blocker; but if it looks achievable do it.

The key problem here is that Embedded media widget does not allow you to know exactly where you are.

Um, I don't know what this problem is. It needs more explanation. "Embedded media widget" describes what it is. I don't know why it needs to describe where it is.

The issue here isn't knowing where the embedded media item is. It's about knowing what media item it is. The issue is specifically about the name of the button, not the name of the container, or it's position in the content.

You are where you are. The media widget occupies a place in an arbitrary sequence of content, inside the text editor. For example the content might be look like: paragraph, paragraph, media-item, paragraph. If you're editing content in a text area, you keep track of this mentally, and explore forwards and backwards whenever you need to. We don't provide any special signifier to say "this is paragraph 3, which is after paragraph 2 and before paragraph 4", so why would the embedded media widget need to inform you where it is?

This isn't any different to reading at a web page; you are in a narrative sequence of content. There might be structured headings, but that depends on the content. An image doesn't need to tell you where it is on the page, you just read/explore the page however you please.

Edit media, button, Embedded media widget, region

Aside: role="region" is inappropriate for the embedded media widget. The region role is for creating custom landmark regions; having those inside an editor could be a bit weird. Better to use role="group". I'll file a separate issue for that, it's major but should be a quick fix, easier than the button text issue here.

role="region" is enforced by CKEditor: that is the approach they settled on after their extensive accessibility work. I really don't want to be going against that. All CKEditor Widgets use this pattern.

Thanks @andrewmacpherson, that makes sense. It sounds like this is another one of those existing major accessibility issues in core that interferes with the media library (in the same way that #2973140: Convey AJAX progress messages to assistive technology. and #3076773: Edit Media button within WYSIWYG should include media label for screen readers do). For that reason, I agree with making it a should-have.

Thank you!

This patch addresses the test failures in #7. I think this is a safe way to address them, but this being the first time I've dealt with window.performance.getEntries() it would be good to get agreement that it's OK to look for the first preview request in that call to the new method getInitialPreviewRequestTransferSize()

I read through #11 and #12 and it looks(?) like there's no need to alter the current approach. If there are additional considerations to be made I know that it was a parsing error and not an intentional dismissal.

#14: woah, that's super fascinating! I don't understand it at all 😆 In #7 I added a response header to an existing response. I'm not triggering additional requests. How is it then possible that you had to read a different request?
(I can step through this with a debugger in the future, but per #13 I think we should spend our limited time on other issues for now.)

Tagging for badcamp2019, thanks!

So, regarding #14 and #15: the reason I did not understand the need for that change is that somehow this is apparently causing a second request to be triggered. Which makes sense: we're modifying the CKEditor Widget's data, which triggers a second request.

This is why the CKEditor plugin has the _previewNeedsServerSideUpdate() and _hashData() functions: we can easily ensure this new piece of data does not trigger preview requests 👍 I should've done that right away of course, but I'm glad the existing functional JS test coverage caught the performance regression! 🥳

1. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
   @@ -478,6 +486,8 @@
   +          // The media entity's label is server-side data and cannot be modified by the content author.
   

   Nit: Longer than 80 characters.

2. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
   @@ -504,8 +514,9 @@
   +              this.setData('label', jqXhr.getResponseHeader('Drupal-Media-Label'));
   

   😲 This is really clever!

3. +++ b/core/modules/media/src/Controller/MediaFilterController.php
   @@ -81,12 +105,24 @@ public function preview(Request $request, FilterFormatInterface $filter_format)
   +    $media_cache_tags = array_filter($build['#cache']['tags'], function ($t) {
   

   Can we rename $t to $tag? My first thought when I saw this was that we were passing some reference to the t() function.

4. +++ b/core/modules/media/src/Controller/MediaFilterController.php
   @@ -81,12 +105,24 @@ public function preview(Request $request, FilterFormatInterface $filter_format)
   +      return strpos($t, 'media:') === 0;
   +    });
   +    if (!empty($media_cache_tags)) {
   +      $embedded_media_id = substr(reset($media_cache_tags), 6);
   +      $media = $this->mediaStorage->load($embedded_media_id);
   +      $headers['Drupal-Media-Label'] = $this->entityRepository->getTranslationFromContext($media)->label();
   +    }
   

   This seems potentially brittle to me. Surely we can extract the entity another way, without needing to rely on a particular cache tag format.

   To wit: don't we have access to the drupal-media tag in this controller? If so, can't we just use its data-entity-uuid attribute?

   If this is too complex, I'm okay deferring refactoring to a follow-up, since the most important thing is to improve the accessibility. But I thought I'd call it out anyway.

#19.4: implementing our own parsing is 10x more brittle than this. The approach in the patch in #18 is in fact not brittle at all, although it surely is a bit unorthodox :)

#21: -1 to that new method.

If we really want to avoid the "detect the media in the piece of text" problem, then I suggest we let the client send the embedded media entity's UUID (which it has readily available) via either the URL path, a URL query argument or a request header.

👍🙂

+++ b/core/modules/media/src/Controller/MediaFilterController.php
@@ -107,8 +107,13 @@ public function preview(Request $request, FilterFormatInterface $filter_format)
+    if ($uuid = $request->query->get('uuid')) {

A little bit earlier in this method, there's this:

  public function preview(Request $request, FilterFormatInterface $filter_format) {
    $text = $request->query->get('text');
    if ($text == '') {
      throw new NotFoundHttpException();
    }

We either should expand that logic to include this new URL query argument, or we should document why we're choosing not to do that.

I think this is ready :) I can't RTBC though, I contributed too much to this.

😨😅🤦‍♂️

1. +++ b/core/modules/media/src/Controller/MediaFilterController.php
   @@ -81,12 +106,19 @@ public function preview(Request $request, FilterFormatInterface $filter_format)
   +    if ($media = $this->entityRepository->loadEntityByUuid('media', $uuid)) {
   +      $headers['Drupal-Media-Label'] = $this->entityRepository->getTranslationFromContext($media)->label();
   +    }
   

   I was going to ask what would happen if the header doesn't get set, but I see from reading https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/getRespo... that, on the client side, data.label will be null. Since we are handling that case, I think that's totally acceptable.

2. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -759,6 +764,8 @@ public function testTranslationAlt() {
   +    // Test `aria-label` attribute appears on the widget wrapper.
   +    $assert_session->elementExists('css', '.cke_widget_drupalmedia[aria-label="Tatou poilu hurlant"]');
   

   👍 Thanks for testing the multilingual support!

This needs accessibility sign-off before it can be RTBC. But once it has that...it's RTBC. Nice job!

Pinged @rainbreaw and @andrewmacpherson in Drupal Slack :)

Reviewing for accessibility. Used: keyboard only, screenreader only (no mouse or eyes). Screenreader used for testing: VoiceOver on MacOSX, both Chrome and Safari.

• In Chrome, when I tab into the body field, I can then further tab into the edit media button. What is read for me (whether there is a caption or not) is "Edit media button, [filename], region"
• In Safari, when I tab into the body field, I can further tab into the edit media button. What is read for me when there is no caption is "Edit media button, [filename], group"
• In Safari, when I tab into the body field, I can further tab into the edit media button. What is read for me when there is a caption is "Edit media button, [caption], group"

The experience was clear and easy, so that is definitely a win.

I'm a little confused because I saw code in the patch referring to alt text, and was expecting to hear the alt text when it exists, but this will also work as far as resolving a blocker is concerned. It may be that I would have heard the alt text with a different screenreader configuration (e.g., NVDA on Firefox?).

I'm comfortable RTBC'ing this from an accessibility standpoint because it does significantly improve accessibility of this item and resolves the aspect of this issue that, in my minds, makes it a blocker in the media library roadmap; at the same time, however, I am going to ping @andrewmacpherson to see if there should be a non-blocker followup to this.

Wonderful, thank you @rainbreaw! 🥳

Briefly discussed with @webchick and we agreed that it's best to open a follow-up now to land this blocker. We can always close the related issue if we need to.

Saving issue credit.

INTERESTING FACT OF NOTE: A "screaming hairy armadillo" (LOL at that, btw, thank you! <3) is an ACTUAL THING:

https://en.wikipedia.org/wiki/Screaming_hairy_armadillo

Ok, excellent! Thanks for the fix, as well as the great testing!

Committed and pushed to 8.8.x!