Problem/Motivation

This is a follow-up to #2994699: Create a CKEditor plugin to select and embed a media item from the Media Library, which is itself a follow-up to #2994696: Render embedded media items in CKEditor, which is itself a follow-up to #2940029: Add an input filter to display embedded Media entities, and the third step of #2801307: [META] Support WYSIWYG embedding of media entities.

The first part is a @Filter plugin, that's what #2940029: Add an input filter to display embedded Media entities provides: Drupal stores media embeds in a structured way using <drupal-media> tags, that are transformed on output. This way, Drupal stays true to its ethos of structured content.

The second part is to make these <drupal-media> tags have a great authoring experience. We don't want content authors to have to look, understand and manipulate that markup: that'd be a usability nightmare. That's what #2994696: Render embedded media items in CKEditor handles.

The third part is to not require content authors to manually construct <drupal-media> tags by hand, thanks to using the Media Library for selection and insertion, handled in #2994699: Create a CKEditor plugin to select and embed a media item from the Media Library.

The fourth and final part is to allow per-embed overrides of metadata (e.g. alt). But also the per-embed customization of the visual representation (view mode using data-view-mode and alignment using data-align + filter_align) and annotation (caption using data-caption + filter_caption).

The scope of this issue is solely the selecting of media from the Media Library and embedding it. Toggling captions and overriding metadata such as alt is the scope #2994702: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`!

Proposed resolution

Quoting @seanB from #3023807-10: Override media fields from the reference field:

WYSIWYG proposal:
WYSIWYG media with button sketch.

Override metadata modal proposal:
Override metadata modal sketch.

Also made a proposal on what the image with the buttons could look like:
Edit and delete example

Remaining tasks

  1. Reviews!
  2. UX sign-off
  3. Accessibility review

User interface changes

Yes! Details TBD.

API changes

None.

Data model changes

None.

Testing steps

  • Install latest 8.8.x
  • Apply the patch
  • Go to Administration => Configuration => Content authoring => Text formats and editors
  • Select a text format (e.g. Basic HTML)
  • Check " Embed media" under "Enabled filters"
  • Unlike in #2994696: Render embedded media items in CKEditor and #2940029: Add an input filter to display embedded Media entities, this step is no longer necessary!Add <drupal-media data-entity-type data-entity-uuid data-view-mode data-align data-caption> in "Limit allowed HTML tags and correct faulty HTML" if the filter enabled and "Save configuration"
  • Drag-and-drop the "Insert Media from Library" button to enable it, this automatically whitelists the necessary HTML. Note that if you also want alignment and captioning, you'll need to still add data-align and data-caption manually — these are not required and hence not added automatically. That should look like this:
  • Go to node add article (/node/add/article)
  • Click the button you just added, you should see the media library selection:
  • Select something you like:
  • Click "Insert selected", you should now see

  • Click the pencil button that's overlaid on top of the embedded media. You should now see (#13) or (#14)
  • Click the "Source" button in CKEditor
  • Add
    <drupal-media data-align="center" data-caption="The first core Media embed ever." data-entity-type="media" data-entity-uuid="84911dc4-c086-4781-afc3-eb49b7380ff5" data-view-mode="full"></drupal-media>
    
    <p>and</p>
    
    <drupal-media data-align="center" data-caption="This media entity is missing" data-entity-type="media" data-entity-uuid="wrong" data-view-mode="full"></drupal-media>
    
  • Click the "Source" button in CKEditor again. Now you should see:
  • Now just like in #2940029: Add an input filter to display embedded Media entities's testing steps, save the article, and you should see:

    Good preview, isn't it? 🙂
  • Now go back to edit it, and observe how the preview loads instantly — just like in https://www.drupal.org/files/issues/2019-06-28/media_embed%20cached%20pr...
CommentFileSizeAuthor
#158 interdiff-2994702-147-158.txt2.1 KBphenaproxima
#158 2994702-158.patch69.91 KBphenaproxima
#147 interdiff-2994702-144-147.txt6.45 KBphenaproxima
#147 2994702-147.patch69.86 KBphenaproxima
#144 2994702-144--combined--3078161-31.patch72.69 KBoknate
#144 2994702--interdiff-141-144.txt1.81 KBoknate
#144 2994702-144--do-not-test.patch70.15 KBoknate
#141 2994702-141.patch72.57 KBoknate
#141 2994702--interdiff-139-141.txt3.32 KBoknate
#139 2994702-139.patch72.82 KBoknate
#139 2994702--interdiff-137-139.txt1.84 KBoknate
#138 2994702--interdiff-137-138.txt1.74 KBoknate
#138 2994702-138.patch74.62 KBoknate
#138 2994702--interdiff-134-138.txt33.81 KBoknate
#137 2994702-137.patch72.73 KBoknate
#137 2994702--interdiff-136-137.txt1007 bytesoknate
#136 2994702-136.patch72.73 KBoknate
#136 2994702--interdiff-134-136.txt32.35 KBoknate
#133 2994702-134.patch70.88 KBoknate
#133 2994702--interdiff-133-134.txt5.73 KBoknate
#133 2994702--interdiff-120-134.txt13.54 KBoknate
#132 2994702-133.patch67.55 KBoknate
#132 2994702--interdiff-132-133.txt3.15 KBoknate
#132 2994702--interdiff-120-133.txt9.66 KBoknate
#131 2994702--interdiff-120-132.txt7.86 KBoknate
#131 2994702--interdiff-130-132.txt2.74 KBoknate
#131 2994702-132.patch65.76 KBoknate
#129 2994702-129--FAIL.patch63.83 KBoknate
#129 2994702--interdiff-120-129--FAIL.txt5.94 KBoknate
#120 2994702-120.patch60.33 KBoknate
#120 2994702--interdiff-119-120.txt534 bytesoknate
#120 2994702--interdiff-109-120.txt7.91 KBoknate
#119 2994702-interdiff--118-119.txt701 bytesoknate
#119 2994702-119.patch60.27 KBoknate
#118 2994702-118.patch60.27 KBoknate
#118 2994702--interdiff-111-118.txt2.12 KBoknate
#111 2994702--interdiff-110-111.txt3.25 KBoknate
#111 2994702-111.patch60.11 KBoknate
#111 2994702-111--FAIL.patch58.87 KBoknate
#110 2994702-110.patch57.95 KBoknate
#110 2994702--interdiff-109-110.txt3.24 KBoknate
#109 2994702-109.patch57.06 KBoknate
#109 2994702-interdiff--104-109.txt1.68 KBoknate
#104 2994702-interdiff--102-104.txt805 bytesoknate
#104 2994702-104.patch56.47 KBoknate
#102 2994702-interdiff--99-102.txt1.94 KBoknate
#102 2994702-interdiff--96-102.txt15.6 KBoknate
#102 2994702-102.patch56.42 KBoknate
#100 2994702-interdiff--98-99.txt3.48 KBoknate
#99 2994702-99.patch56.52 KBoknate
#99 2994702-interdiff--96-99.txt15.3 KBoknate
#98 2994702-98.patch56.29 KBoknate
#98 2994702-interdiff--96-98.txt15.06 KBoknate
#96 2994702-96.patch57.07 KBoknate
#96 2994702-interdiff--91-96.txt18.12 KBoknate
#88 2994702-88.patch53.65 KBoknate
#87 2994702-87.patch69.65 KBoknate
#87 2994702--interdiff-85-87.txt2.6 KBoknate
#87 2994702--interdiff-82-87.txt16.5 KBoknate
#85 2994702-85.patch53.66 KBoknate
#85 2994702--interdiff-82-85.txt15.64 KBoknate
#82 2994702--interdiff-81-82.txt723 bytesoknate
#82 2994702--interdiff-71-82.txt19.45 KBoknate
#82 2994702-82.patch53.3 KBoknate
#81 2994702--interdiff-80-81.txt773 bytesoknate
#81 2994702--interdiff-71-81.txt19.48 KBoknate
#81 2994702-81.patch53.41 KBoknate
#80 2994702--interdiff-71-80.txt19.97 KBoknate
#80 2994702--interdiff-78-80.txt6 KBoknate
#80 2994702-80.patch53.44 KBoknate
#78 2994702-78--FAIL.patch50.79 KBoknate
#78 2994702--interdiff-77-78.txt7.65 KBoknate
#78 2994702--interdiff-71-78.txt18.32 KBoknate
#78 2994702-78.patch51.31 KBoknate
#77 2994702-77.patch47.29 KBoknate
#77 2994702--interdiff-71-77.txt12.62 KBoknate
#77 2994702--interdiff-76-77.txt878 bytesoknate
#76 2994702-76.patch47.42 KBoknate
#76 2994702--interdiff-71-76.txt12.13 KBoknate
#71 new filter description in context.png252.65 KBoknate
#71 link-in-dialog.png42.33 KBoknate
#71 annoying-ckeditor-functionality.mov1.01 MBoknate
#71 2994702-71.patch44.39 KBoknate
#71 2994702--interdiff-68-71.txt3.91 KBoknate
#70 insert-media-in-wysiwyg.mp42.41 MBseanB
#68 2994702-68.patch42.29 KBoknate
#68 2994702--interdiff-65-68.txt12.4 KBoknate
#65 2994702-65.patch48.47 KBoknate
#65 2994702--interdiff-61-65.txt45.18 KBoknate
#61 nothing to override.png29.28 KBoknate
#61 2994702-61.patch57.14 KBoknate
#61 2994702--interdiff-56-61.txt9.36 KBoknate
#56 2994702-56.patch52.11 KBoknate
#56 2994702--interdiff-43-56.txt29.22 KBoknate
#43 2994702-43.patch56.07 KBoknate
#43 2994702--interdiff-42-43.txt1.17 KBoknate
#42 2994702-42.patch56.06 KBoknate
#42 2994702--interdiff-40-42.txt10.84 KBoknate
#40 2994702-40.patch55.43 KBoknate
#40 2994702--interdiff-39-40.txt16.18 KBoknate
#39 2994702-39.patch53.92 KBoknate
#39 2994702--interdiff-26-39.txt47.28 KBoknate
#33 2994702-33--do-not-test.patch53.92 KBoknate
#33 2994702--interdiff-32-33.txt9.59 KBoknate
#32 2994702-32--combined-2994699-86.patch97.66 KBoknate
#32 2994702-32--do-not-test.patch63.51 KBoknate
#32 2994702--interdiff-30-32.txt11.42 KBoknate
#32 missing toolbar.png35.46 KBoknate
#31 2994702-30--combined-2994699-86.patch87.26 KBoknate
#31 2994702--interdiff-29-30.txt1.65 KBoknate
#31 2994702-30--do-not-test.patch53.12 KBoknate
#29 2994702-29--do-not-test.patch53.35 KBoknate
#29 2994702-29--combined--2994699-86.patch87.49 KBoknate
#29 2994702--interdiff-26-29.txt46.7 KBoknate
#26 2994702-26-do-not-test.patch18.76 KBWim Leers
#26 2994702-26-combined-2994699-45.patch48.34 KBWim Leers
#24 2994702-24-combined-299469925-and-2994696-25.patch60.78 KBWim Leers
#24 2994702-24-do-not-test.patch16.59 KBWim Leers
#24 interdiff.txt2.47 KBWim Leers
#23 2994702-14-combined-2994699-25-and-2994696-25.patch60.79 KBWim Leers
#20 2994702-15-combined-2994699-25-and-2994696-23-and-2940029-104.patch124.14 KBkcolaers
#19 2994702-14-combined-2994699-25-and-2994696-23-and-2940029-104.patch124.14 KBWim Leers
#17 2994702-14-combined-2994699-24-and-2994696-23-and-2940029-99.patch123.89 KBWim Leers
#15 2994702-14-combined-2994699-22-and-2994696-16-and-2940029-91.patch122.47 KBWim Leers
#14 2994702-14 clicked pencil button.png959.53 KBWim Leers
#14 2994702-14-combined-3039829-34-and-2994699-15-and-2994696-16-and-2940029-68.patch142.47 KBWim Leers
#14 2994702-14-do-not-test.patch14.21 KBWim Leers
#14 interdiff.txt7.31 KBWim Leers
#13 2994702-13 clicked pencil button.png1.07 MBWim Leers
#13 2994702-13 inserted media.png2.1 MBWim Leers
#13 2994702-13-combined-3039829-34-and-2994699-15-and-2994696-16-and-2940029-68.patch137.74 KBWim Leers
#13 2994702-13-do-not-test.patch9.49 KBWim Leers
#8 sea-otters.jpg149.1 KBwebchick
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phenaproxima created an issue. See original summary.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Status: Postponed » Postponed (maintainer needs more info)

Thinking about this more: why would we require this when embedding media in CKEditor, but not when referencing media through an entity reference field? In both scenarios, the result is the reuse of media, and in both cases, you may need to be able to specify e.g. an alternative text (alt) for that particular use.

Wim Leers’s picture

Wim Leers’s picture

We discussed this yesterday; @seanB and @phenaproxima agreed that if #3023807: Override media fields from the reference field is not a must-have, then this should not be a must-have either.

phenaproxima’s picture

I agree. Per-usage metadata overrides are a tricky problem, and we should probably tackle it all at once for both entity references and CKEditor embeds. So yes, I agree the priority of this issue should be synced with that of #3023807: Override media fields from the reference field.

seanB’s picture

Agreed! The 2 issues are basically about the same problem, just related to a different widget.

*edit hah, cross-posted with phenaproxima, didn't mean to repeat exactly what you said :)

webchick’s picture

Status: Postponed (maintainer needs more info) » Active
Issue tags: +Needs accessibility review
FileSize
149.1 KB

I think the "must-have"-ness of this issue might pre-date the re-prioritizing of things for Media Library, which includes #3023807: Override media fields from the reference field.

From a product management POV, I'm comfortable with this as a "strong should have" vs. a "must-have." However, this might need accessibility team input as well.

To illustrate the problem we're trying to work around, is let's say that there's this adorable image of sea otters:

Sea otters!!

You upload it to your Drupal site attached to a blog post about the Vancouver Aquarium, and give it the caption/alt text "Tanu and Katmai are the two eldest sea otters at the Vancouver Aquarium." (Which, true story, is its actual caption. :))

Sometime later, you're writing a blog post about the dangers of plastic in the ocean. You re-use the same adorable sea otter image, this time to illustrate that sea animal life is impacted by this. But the original caption makes zero sense in this usage.

The reason for "strong should have" vs. "must have" is because there's a workaround: give the image a more generic caption/alt text that supports the media being used in multiple places ("Two sea otters swimming together"), and add the additional context ("Tanu and Katmai are the two eldest" vs. "Otters are one example of sea life impacted by plastics") in the surrounding text of the article itself.

In any case, I do agree that the problem is the same regardless if it's caused by CKEditor or Media fields, so they should share the same priority, as well as ideally the same solution.

I vote for "should have," personally, because I think that having a stable version of Media Library with CKEditor integration is more impactful than having this one particular feature, especially if we get it later at some point (When I spoke with the Media team about this earlier today, they confirmed that this issue isn't a matter of "if" but rather "when"). But that assumes the above scenario is not an accessibility blocker, so tagging for accessibility review.

webchick’s picture

Status: Active » Postponed (maintainer needs more info)

Oops, guess we still need "more info" from the a11y folks.

andrewmacpherson’s picture

Priority: Normal » Major
Issue tags: -Needs accessibility review +Accessibility, +atag

I concur with making this a strong should-have on the media roadmap.

Text alternatives are a WCAG level A requirement. That's a must-have, but there are lots of situations to support. If we don't implement the embed-specific feature here, we still have a lot of situations covered. We would satisfy the following parts of WCAG and ATAG at level AA:

  1. For single-instance images (i.e. the image is in the media library, but an author has only used it once), using the alt text from the media entity, WCAG SC 1.1.1 "Non-text content" is satisfied, and so is all of ATAG guideline B.2.3. "Assist authors with managing alternative content for non-text content".
  2. For re-usable images, ATAG's B.2.3.3 "Save for Reuse" criterion says it's desirable to store alt text alongside images in the media library, so it's available the next time the image gets re-used. Even if we don't provide authors with the ability to tailor the alt text per embed-instance, we have at least partly satisfied this ATAG criterion (the "save and suggest" aspect). Note this ATAG criterion is level AAA, and our accessibility gate only calls for level AA.
  3. Saving the alt text in the media library alongside the image also means that ATAG B.2.3.2 "Automating Repair of Text Alternatives" is partially satisfied. Re-using alt text that was previously entered for the image is an acceptable way to satisfy the "no irrelevant strings" aspect, even if it doesn't satisfy the "in-session repairs" aspect. Quote from the implementation guide: "Since the alternative content was gathered from authors' previous entries into the same fields for the same objects, these are acceptable as relevant sources."
  4. Drupal's multi-lingual features mean that alt text in an image field, on a media entity, is translatable. Right? If so, that's another aspect of ATAG B2.3.3 "Save for re-use" covered, at level AAA.

In #8, @webchick correctly describes a feasible workaround, with additional context for the image in nearby visible text. If authors can't tailor the image alt text for a specific embedded usage, this workaround will suffice in many cases where an image is presented as part of a text narrative.

However, beware of conflating text alternatives (shown instead of the image) with captions (which are supplemental, shown alongside the image). The nearby-text workaround isn't suitable for all situations. "A picture is worth a thousand words", the saying goes. The CMS corollary is that it should be possible for an image to be accessible in context by itself (using alt text only), without needing visible text nearby to supplement it.

Another workaround is that authors can re-upload an image, so there are two media entities with a duplicated image file but different alt text. This would technically allow authors to satisfy WCAG, but it's a messy solution! Many lesser CMS allow this, but it quickly gets out of hand. It can be hard to select the correct media entity when they both look the same. The feature request here is about avoiding this horrible workaround, in particular.

Reading ATAG, I'm not 100% clear whether it's a conformance requirement for a media library to store alt text in the library with the image, AND allow authors to override it with per-instance alt text. It's strongly implied by the non-normative supporting notes for success criteria B.2.3.2 Automating Repair of Text Alternatives (level A), and B.2.3.3 Save for Reuse (level AAA). The relevant implementation examples say "The stored alternative content is suggested as alternative content for author approval whenever the associated object is inserted." However it's arguably not a requirement the way the normative parts of these ATAG success criteria are worded.

I'm happy with treating this as a should-have for the media roadmap, but I'm bumping it to a major feature request, to reflect the awesome-sauce impact that this feature can bring to authors. Particularly the workarounds that it eliminates. Crappy alt text with poor context would be a WCAG level A bug, so an easy means to achieve better alt text in context is a major win overall for ATAG guideline B.2.3: "Assist authors with managing alternative content for non-text content."

This should-have justification applies equally to #3023807: Override media fields from the reference field, so I'll briefly mention it there and link to this comment.

Wim Leers’s picture

Thanks, @webchick and especially @andrewmacpherson, for the very detailed comment with references to relevant sections of the spec we're aiming to conform to. This is super valuable :)

Wim Leers’s picture

Title: [PP-3] Allow editors to alter embed-specific metadata in CKEditor » [PP-3] Allow editors to alter embed-specific metadata, as well as `data-view-mode`, `data-align` and `data-caption`
Issue summary: View changes
Status: Postponed (maintainer needs more info) » Postponed
Issue tags: +Usability, +Needs usability review, +Needs tests

Quoting one very relevant bit from #2998005-51: [PP-1] Support Drupal core's Media Library:

As discussed with @seanB, @phenaproxima and @ckrina at Drupal Dev Days:

  1. we want to ensure that embedding from the media library is a smooth experience, forcing the user to think about a view mode, alignment, caption and metadata overrides (alt for example) is bad UX
  2. we want to bring a consistent UX across all media embeds (either through entity reference fields or via CKEditor/filter) with regards to per-embed overrides — this is therefore blocked on #3023807: Override media fields from the reference field (reference field embed) and #2994702: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`) (text embed)
  3. we want to bring a consistent UX across all media library experiences: the pencil and cross icons that #3039829-35: Remove link to media item from media library view. is adding to the media library should also be used in media reference fields (for overriding metadata and deleting the reference, respectively) and in text fields (for overriding metadata and deleting the embed, respectively)
  4. There was a lot of discussion about whether to put the per-embed metadata overrides and alignment/captioning in either the same dialog, in separate dialogs, in vertical tabs in a dialog, in fieldsets in a dialog, in accordions in a dialog, … basically everything you can think of was suggested. There's no consensus on this yet. See #3023807-10: Override media fields from the reference field for the YouTube recording of the UX meeting where this was discussed at length, plus #3023807-12: Override media fields from the reference field for the mock-ups.

#2994699: Create a CKEditor plugin to select and embed a media item from the Media Library handles point 1. This issue needs to deal with point 2, 3 and 4.

Depending on the design/UX choices, we could perhaps split this up in multiple issues, but at the moment they are conceptually and UX-wise so intertwined that we must agree on a UX first.

data-align and data-caption are an absolute must-have. Depending on patch/review iterations in #2940029: Add an input filter to display embedded Media entities, it's possible that data-view-mode will get descoped, and at that point we won't have to deal with that here anymore either.

Wim Leers’s picture

@seanB has posted #3039829-34: Remove link to media item from media library view. whose CSS we can reuse to achieve this:
Edit and delete example

Here's a first iteration that deals only with data-align and data-caption!

Embedded media now get two overlaid buttons: a pencil for editing and a cross for deleting. This is consistent with what #3039829: Remove link to media item from media library view. does for the Media Library.
Clicking the pencil button triggers a dialog for changing alignment and toggling the caption on/off
Wim Leers’s picture

And one more iteration, this time with alt support and even title support.

(The "Image" button in CKEditor and the corresponding EditorImageDialog do not yet support title!).

This changes that last screenshot to something that looks exactly like EditorImageDialog! 🤩 Well, with two differences:

  1. No input[type=file] 🥳
  2. A placeholder attribute containing the alt value stored in the Media Library, which signifies that this is the default.

Wim Leers’s picture

A few rounds of feedback have been incorporated into #2940029, rebasing #16 on top of #2940029-91: Add an input filter to display embedded Media entities.

Plus, even more importantly, #3039829: Remove link to media item from media library view. landed! That means we no longer need to include that in our combined patch here, and it's one blocker less, plus less uncertainty. 🥳

Wim Leers’s picture

Wim Leers’s picture

phenaproxima’s picture

I demoed this to the UX team today. Feedback was overwhelmingly positive, so hooray for that!

There were a few potential points of contention, not necessarily things we should address in this patch (or even in core), but worth pointing out:

  1. Choosing a view mode per embed is, in the words of @jrockowitz, not an 80% use case, but more like 50-50. So, supporting data-view-mode in #2940029: Add an input filter to display embedded Media entities is a very wise decision on our part. It might be worth having a separate issue to discuss exposing the available view modes as a set of checkboxes, and allowing the user to choose which one to use per-embed if more than one view mode is enabled. For MVP purposes, though, having this in contrib is fine.
  2. This CKEditor plugin deviates from the core image plugin in one significant way -- with the core image plugin, the caption is not enabled by default. Instead, you have to specifically turn it on when you embed an image. In this plugin, we enable captions by default and force the user to edit the embed in order to disable the caption. It might be worth making them consistent (i.e., off by default, and you must edit the embed to turn it on). Also, it was suggested that we might want to provide the ability (if we don't already) to disable captioning globally, or at the text format level.
  3. We all wryly agreed that we look forward to the massive bikeshedding session to figure out what the editor button's icon will be. :)

All of these things can be fleshed out in separate issues.

Wim Leers’s picture

Title: [PP-3] Allow editors to alter embed-specific metadata, as well as `data-view-mode`, `data-align` and `data-caption` » [PP-3] Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`
FileSize
124.14 KB

First: darn, I wish I could've made that call! Is there a recording I can watch?

Second: thanks for presenting this, @phenaproxima! 🙏

Third: feedback to #18:

  1. I wholeheartedly agree. I am glad the UX team at large and @jrockowitz in particular leaned this way! Updated issue title to match.
  2. Again glad this was raised! I chose that default in #2994699-15: Create a CKEditor plugin to select and embed a media item from the Media Library specifically to trigger feedback:
    +++ b/core/modules/media_library/src/MediaLibraryCkeditorOpener.php
    @@ -33,6 +33,9 @@ public function getSelectionResponse(MediaLibraryState $state, array $selected_i
    +        // @todo Agree on these defaults.
    +        'data-align' => 'center',
    +        'data-caption' => ' ',
    

    So I removed data-caption, see #2994699-25: Create a CKEditor plugin to select and embed a media item from the Media Library.

  3. 😂😭

Rerolled patch for #18.2.

kcolaers’s picture

Filename: MediaLibrary*Cke*ditorOpener.php
Classname: MediaLibraryCKEditorOpener

Our build server does not like this.

Wim Leers’s picture

#20: 👍 Excellent catch :) I'll incorporate this in the actual-to-review patch rather than the combined patch. That patch lives in #2994699: Create a CKEditor plugin to select and embed a media item from the Media Library.

Also, if at all possible, we should not make this coupled to ckeditor.module, but instead to editor.module.

Wim Leers’s picture

Title: [PP-3] Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption` » [PP-2] Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`
Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

Actually, #24's interdiff belonged on #2994699. My bad. Did that: #2994699-28: Create a CKEditor plugin to select and embed a media item from the Media Library. Ignore #24's interdiff and patch. The "combined" patch is still accurate though, but should've been named 2994702-14-combined-2994699-28-and-2994696-25.patch.

Wim Leers’s picture

#2994696: Render embedded media items in CKEditor landed! Rerolled, on top of #2994699-45: Create a CKEditor plugin to select and embed a media item from the Media Library.

Notes:

  • Previous patches in the chain have ES6-ified the JS, since core requires that. That made this patch hard to apply. I manually ported it. Hopefully I did not make mistakes. 🤞 I'm sure @oknate will double-check my work :D
  • #2994699-44: Create a CKEditor plugin to select and embed a media item from the Media Library removed some logic that now should be added by this patch. Did that.
  • +++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
    @@ -78,6 +78,7 @@ public function getCssFiles(Editor $editor) {
    +      drupal_get_path('module', 'media_library') . '/css/media_library.theme.css',
    

    I don't know what this was doing here. This file does not exist. Omitted it.

Wim Leers’s picture

Title: [PP-2] Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption` » [PP-1] Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`

🤓

oknate’s picture

I suggest we change "Delete media" to something less scary sounding, maybe:

  • "Remove media"

or

  • "Remove media embed"

Even though I knew it wasn't going to delete the media, I was still a little nervous to push the button. 😱

oknate’s picture

  1. Adds test coverage
  2. Fixes undo for delete button.
  3. Fixes bug where setting alignment to none wasn't updating widget.
  4. Fixes the empty string indicator code ('""'), so it matches the behavior of entity embed. To test this, I changed the display from thumbnail to image formatter in the test. It works with thumbnail, but thumbnail wasn't outputting the title attribute. So if I figure out how to make the thumbnail do that, we don't need the new code to change the entity view display for media default in that test, although I think it's fine as it is, and possibly better, since it's probably more common to use the image formatter.
  5. Ran the javascript through prettier and eslint.

Todo:
1) change to use .'media-embed-error' class rather than checking the filename.

if (this.element.findOne('img[src$="no-thumbnail.png"]')) {
  return;
}

2) Add test coverage for 2.

4) Why does 'media.filter.preview' route take filter_format as parameter, but 'editor.media_dialog' takes an editor? It would be easier for the access check to reuse the same access. We could then reuse:

_custom_access: '\Drupal\media\Controller\MediaFilterController::formatUsesMediaEmbedFilter'

The editor is only used two places in the form, and both times it's to get the filter format:

if ($editor->getFilterFormat()->filters('filter_align')->status) 

and

if ($editor->getFilterFormat()->filters('filter_caption')->status) 

So I recommend we alter this form and route to take the filter format, as the preview route does, and then add the same access as the preview route:

    _entity_access: 'filter_format.use'
    _custom_access: '\Drupal\media\Controller\MediaFilterController::formatUsesMediaEmbedFilter'

Unless I'm missing a good reason for them to be different.

Ah, I see, it's following a pattern:

editor.image_dialog:
  path: '/editor/dialog/image/{editor}'
  defaults:
    _form: '\Drupal\editor\Form\EditorImageDialog'
    _title: 'Upload image'
  requirements:
    _entity_access: 'editor.use'

editor.link_dialog:
  path: '/editor/dialog/link/{editor}'
  defaults:
    _form: '\Drupal\editor\Form\EditorLinkDialog'
    _title: 'Add link'
  requirements:
    _entity_access: 'editor.use'
oknate’s picture

Issue tags: -Needs tests

Fixing test failure in #29.

diff --git a/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
index c0d65388f5..52e8c8c872 100644
--- a/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
+++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -84,11 +84,11 @@ protected function setUp() {
                   'Source',
                   'Bold',
                   'Italic',
+                  'Undo',
+                  'Redo',
                   'DrupalLink',
                   'DrupalUnlink',
                   'DrupalImage',
-                  'Undo',
-                  'Redo',
                 ],
               ],
             ],

For some reason the toolbar fails to render when the 'Undo' and 'Redo' are on the end. It's out of scope for this issue, I think, unless it's a bug created by the drupalmedia plugin js. Maybe I'll file a separate issue for that, and see if I can recreate it without the plugin.

Update:

See #32, it seems it has something to do with the code to remove DrupalImage adding numbered keys to the editor settings for the toolbar. Bizarre bug.

Also I fixed a cut-and-paste error, "without" should have been "with" (I refactored this away to use a css check).

+    // Wait for element to update without figcaption.
+    $result = $page->waitFor(10, function () use ($drupal_media) {
+      return !empty($drupal_media->find('css', 'figcaption'));
+    });
+    // Will be true if figcaption exists.
oknate’s picture

Oops, forgot to post the patches in the last comment.

oknate’s picture

I figured out the bug that was causing the failure in #29. It's in our test code. Somehow this code was changing the keys in the settings:

    if (!$drupalimage_is_enabled) {
      // Remove the `drupalimage` plugin's `DrupalImage` button.
      $editor = Editor::load('test_format');
      $settings = $editor->getSettings();
      $rows = $settings['toolbar']['rows'];
      foreach ($rows as $row_key => $row) {
        foreach ($row as $group_key => $group) {
          foreach ($group['items'] as $item_key => $item) {
            if ($item === 'DrupalImage') {
              unset($settings['toolbar']['rows'][$row_key][$group_key]['items'][$item_key]);
            }
          }
        }
      }
      $editor->setSettings($settings);
      $editor->save();

Before (no numbered keys):
{"toolbar":{"rows":[[{"name":"Main","items":["Source","Bold","Italic","DrupalLink","DrupalUnlink","DrupalImage","Undo","Redo"]}]]},"plugins":{"language":{"language_list":"un"}}}
After (with numbered keys):
{"toolbar":{"rows":[[{"name":"Main","items":{"0":"Source","1":"Bold","2":"Italic","3":"DrupalLink","4":"DrupalUnlink","6":"Undo","7":"Redo"}}]]},"plugins":{"language":{"language_list":"un"}}}

For some reason that caused the toolbar to fail to load when I added the two additional buttons.

Here's a screenshot of the missing toolbar:
missing toolbar

Changing the code slightly (using array_keys) preserved the original structure (minus the button we're trying to delete):

@@ -569,9 +569,11 @@ public function testLinkability($drupalimage_is_enabled) {
         foreach ($row as $group_key => $group) {
           foreach ($group['items'] as $item_key => $item) {
             if ($item === 'DrupalImage') {
-              unset($settings['toolbar']['rows'][$row_key][$group_key]['items'][$item_key]);
+              unset($group['items'][$item_key]);
+              break;
             }
           }
+          $settings['toolbar']['rows'][$row_key][$group_key]['items'] = array_values($group['items']);
         }
       }

Very weird bug! It doesn't make sense why having them in the middle would work, but not on the end. But that seemed to fix it, and so does this change.

@todo - I accidentally included my test class, remove that in the next patch.

oknate’s picture

Removing test code.

Wim Leers’s picture

#28: That sounds less scary indeed. I like your suggestion better too. Thanks for empathizing with the end user 🙏

#29: I also noticed there were some bugs in this last step of the 4-step patch set. I deliberately didn't spend time fixing those because there was a risk of this patch needing to change a lot depending on how the 3 blockers had to change. And I was hoping you were going to write test coverage 😊😅 Thanks!

  1. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +246,105 @@
    +          // No buttons for missing media.
    

    🤔 Does this truly make sense? Why shouldn't this get buttons? How would you then remove missing media?

  2. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +246,105 @@
    +          if (this.element.findOne('img[src$="no-thumbnail.png"]')) {
    

    🤔 This should indeed use the class to detect missing media, like you wrote in #29.

  3. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -237,7 +357,7 @@
    -         * @returns {boolean}
    +         * @return {boolean}
    

    🦅👀👏

  4. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    + * Provides an image dialog for text editors.
    

    🤓 Übernit: s/image/media/. Also: I introduced this! Sorry :)

  5. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -414,6 +414,14 @@ protected function applyPerEmbedMediaOverrides(\DOMElement $node, MediaInterface
    +        //print_r(trim($node->getAttribute('alt')));
    

    🤓 Debug leftover :)

  6. I still need to review the test coverage. Adding strategic blank lines to convey logical groups would make this a lot simpler to understand.
oknate’s picture

In response to #34.1:

Why shouldn't [the missing media indicator] get buttons? How would you then remove missing media?

You can remove things in the ckeditor by focusing on them and then hitting the back button. For example "p" tags don't have a remove button and no one has seen the need to add one. I'm teasing of course. But it does raise the question of whether we really need a delete button. I think it's there to align stylistically with the media library. It's not necessary, it's a nice-to-have. And I think a delete button on the missing media indicator is similarly a nice to have. I'm ok with adding it. But then maybe we should have the edit button too, to allow stylistic alignment with the media library. The edit button doesn't require a valid uuid to work since it's just editing the other metadata, although a change might need to happen where we load the placeholders for 'alt' and 'title'.

I'd like to see what delete/edit buttons look like on the missing media indicator.

phenaproxima’s picture

Title: [PP-1] Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption` » Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`
Status: Postponed » Needs work

The blocker is in, so this is no longer postponed. Avanti! Marking "needs work" to address Wim's feedback in #34.

phenaproxima’s picture

  1. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -160,12 +160,26 @@
    +            if (!this.data.hasCaption && this.oldData.hasCaption) {
    

    Why does this care if this.oldData.hasCaption is true? Seems like, if this.data.hasCaption is false, the data-caption should always be deleted.

  2. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -179,6 +193,14 @@
    +          } else {
    +            this.element.removeAttribute('data-align');
    +            const classes = this.element.getParent().$.classList;
    +            for (let i = 0; i < classes.length; i++) {
    +              if (classes[i].substr(0, 6) === 'align-') {
    +                this.element.getParent().removeClass(classes[i]);
    +              }
    +            }
    

    Why was this added? If it's necessary, seems like something that might benefit from a comment.

  3. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +246,105 @@
    +            // When starting out with an empty caption, CKEditor automatically
    +            // injects a <br> that we need to delete.
    +            // @see core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.es6.js
    +            if (
    +              captionEditable.$.childNodes.length === 1 &&
    +              captionEditable.$.childNodes.item(0).nodeName === 'BR'
    +            ) {
    +              captionEditable.$.removeChild(
    +                captionEditable.$.childNodes.item(0),
    +              );
    +            }
    

    I understand that we need this hack (ugh), but is there any external issue or URL we could reference as well?

  4. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +246,105 @@
    +          if (this.element.findOne('img[src$="no-thumbnail.png"]')) {
    

    Wim already called this out, but I agree that we should NOT be relying on a particular file name to determine if media is missing. A CSS class would be much better. :)

  5. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +246,105 @@
    +          const isElementNode = function(n) {
    +            return n.type === CKEDITOR.NODE_ELEMENT;
    +          };
    

    What is an "element node"? What will this predicate match? Could use a comment.

  6. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +246,105 @@
    +          if (this.data.link) {
    +            embeddedMedia = embeddedMedia.getFirst(isElementNode);
    +          }
    

    This could also use a comment, to explain why the presence of a link means that the element upon which we call getFirst() changes.

  7. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +246,105 @@
    +          embeddedMedia.setStyle('position', 'relative');
    

    Should we put this in ckeditor.drupalmedia.css, rather than using an inline style?

  8. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +246,105 @@
    +          embeddedMedia.getFirst().insertBeforeMe(editButton);
    +          const deleteButton = CKEDITOR.dom.element.createFromHtml(
    

    Nit: There should be an empty line between these two, IMHO.

  9. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +246,105 @@
    +              'Delete media',
    

    I agree that "Remove media" is better terminology here than "delete". Let's change it.

  10. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +246,105 @@
    +                  hasCaption: !!values.hasCaption,
    

    !!? Why would we want to use that? That seems very odd. If we need to do it this way, a comment might be wise.

  11. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    + * @internal
    + */
    +class EditorMediaDialog extends FormBase {
    

    We should expand the @internal to include the messaging we've added to other internal classes.

  12. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    +    if (isset($form_state->getUserInput()['editor_object'])) {
    

    So...we are handling raw user input here and caching it in form state. Should there be some sort of hashing present? Should this get a set of security-conscious eyes on it?

  13. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    +      $media_embed_element = $form_state->getUserInput()['editor_object']['attributes'];
    +      $form_state->set('media_embed_element', $media_embed_element);
    +      $has_caption = $form_state->getUserInput()['editor_object']['hasCaption'];
    

    Rather than repeatedly calling $form_state->getUserInput()['editor_object'] in this function, let's do that once at the top of the method and reuse the value.

  14. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    +    else {
    +      // Retrieve the user input from form state.
    +      $media_embed_element = $form_state->get('media_embed_element');
    

    IMHO, this should probably be elseif ($form_state->has('media_embed_element')). Then we should add an additional else case which throws a BadRequestException. Just in case.

  15. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    +      if ($alt === '' && !empty($media_embed_element['src'])) {
    

    Where is the 'src' attribute set? I didn't see any mention of it in the JavaScript. Is this a holdover from drupalimage?

  16. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    +          '#maxlength' => 512,
    

    Why is this half the allowed length of 'title'?

  17. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    +        '#default_value' => (!isset($media_embed_element['data-align']) || $media_embed_element['data-align'] === '') ? 'none' : $media_embed_element['data-align'],
    

    This can be shortened: empty($media_embed_element['data-align']) ? 'none' : $media_embed_element['data-align'].

  18. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    +        '#wrapper_attributes' => ['class' => ['container-inline']],
    +        '#attributes' => ['class' => ['container-inline']],
    

    Why do we need to set both #attributes and #wrapper_attributes?

  19. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    +      '#submit' => [],
    +      '#ajax' => [
    +        'callback' => '::submitForm',
    +        'event' => 'click',
    +      ],
    

    This seems a bit strange. Won't submitForm() run when the form is submitted?

  20. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    +    if ($form_state->hasValue(['attributes', 'alt']) && trim($form_state->getValue(['attributes', 'alt'])) === '""') {
    

    The hasValue() call is superfluous and can be removed.

  21. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    +    if ($form_state->hasValue(['attributes', 'alt']) && trim($form_state->getValue(['attributes', 'alt'])) === '') {
    +      $form_state->setValue(['attributes', 'alt'], FALSE);
    +    }
    +    if ($form_state->hasValue(['attributes', 'title']) && trim($form_state->getValue(['attributes', 'title'])) === '') {
    

    Same here. Also, why do we need to set the empty alt to FALSE? Can't we just use $form_state->unsetValue()?

  22. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    +    if ($item_class == ImageItem::class || is_subclass_of($item_class, ImageItem::class)) {
    

    I could be wrong, but I think we can just use is_a() here instead of two conditions ORed together.

oknate’s picture

Issue summary: View changes
Issue tags: +Needs accessibility review

Adding "Accessibility review" as a @todo in the issue summary and the 'needs accessibility review' Issue tag.

I see Andrew Macpherson commented back in #10. I'll reread that tonight, and see if there are any @todos.

oknate’s picture

FileSize
47.28 KB
53.92 KB

Just a reroll and an interdiff to back before this weekend, so all the changes so far can be reviewed.

oknate’s picture

Status: Needs work » Needs review
FileSize
16.18 KB
55.43 KB

Responses to #34:
1. TBD, I will investigate this.
2. ✅Updated to use this.element.findOne('.media-embed-error')
3. That was the linter, I can’t take credit.
4. ✅Updated to “media embed”, I can change it to “media” if you think that’s better. I think the more specific version is better.
5. Now you have eagle eyes.

Responses to #37:
1. ✅ I adjusted the logic so that it deletes the attribute if data-caption is false. CKEditorIntegrationTest.php::testEditableCaption() still ran fine.
2. regarding this code:

+          } else {
+            this.element.removeAttribute('data-align');
+            const classes = this.element.getParent().$.classList;
+            for (let i = 0; i < classes.length; i++) {
+              if (classes[i].substr(0, 6) === 'align-') {
+                this.element.getParent().removeClass(classes[i]);
+              }
+            }
           }

I found a better way to do this. In the EditorMediaDialog:

    // If `data-align`is set to "none", remove the attribute.
    if ($form_state->hasValue(['attributes', 'data-align']) && $form_state->getValue(['attributes', 'data-align']) === 'none') {
      $form_state->unsetValue(['attributes', 'data-align']);
    }

3. I couldn’t find any more documentation on this, but I added the verbiage from drupalmediacaption js.

4. ✅We can use this.element.findOne('.media-embed-error')

5. ✅Added a javadoc with some more information. I don’t know if we can add one in this location, but it would be useful to have the info.

6. ✅Added a comment.

7. 👎I’m not sure how to correctly determine the selector, since there are several various combinations of markup. drupal-media figure embed, drupal-media a embed, etc. I think setting it dynamically is better, unless we can find a rock solid way to apply it with a selector.

This also made me realize without the media library css included in the CKeditor, the buttons lack styling and positioning (because when I was testing, that was the case.) I added
$this->moduleExtensionList->getPath('media_library') . '/css/media_library.theme.css',
to core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
as a temporary measure. We’ll need to discuss what’s the right way to makes sure these styles are available.

8. ✅Added a line.

9. ✅Replaced string “Delete media” with “Replace media”.

10. !!caption coerces the object to a boolean. I added a note.

11. ✅ Added the same @interal verbiage as we use in the other classes.

12. I think hashing sounds good and getting security eyes on this sounds good. Anytime you’re using getUserInput() directly it’s a security concern. Maybe they can tell us better way to do this.

13. ✅Created a variable instead of getting editor_object from getUserInput() three times.

14. ✅I added BadRequestHttpException, and a MissingDataException if $editor_object['attributes'] is empty.

Update: I removed this temporarily when fixing the broken patch, but I'll test restoring it and add test coverage.

15. ¯\_(ツ)_/¯ It could be a holdover from an earlier iteration of the missingMediaIndicator. I’m removing this code for now.

16. “Drupal 7.12 increased alt length to 512 and title length to 1024.” I found that sentence here: https://www.drupal.org/project/imagefield/issues/331713

Added note:

// Maxlengths for `title`and `alt` fields follow ImageItem schema.
// @See core/modules/image/src/Plugin/Field/FieldType/ImageItem::schema()

17. ✅Updated.

18. ¯\_(ツ)_/¯ I don’t know anything about this. I did find this related issue: https://www.drupal.org/project/drupal/issues/1183564

19. This is the same as EditorLinkDialog and EditorImageDialog. Don’t mess with perfection? I think what happens is if there’s a regular submit function, it tries to submit the parent form, and since we don’t want, we just want to call this subforms submitForm method, we call it using the #ajax callback, which returns it’s own response.

20. ✅Updated. Added a default of ‘’ so we’re not passing NULL to trim. Although I looked it up and passing NULL to trim will cast it to a string. But I think it’s better to set the default value.

21. ✅Changing to unsetValue(). I don’t know if there’s some reason attributes[alt] and attributes[title] needs to be passed as a NULL value. We’ll see.

22. ✅Changed to is_a(). I think this makes sense, based on the documentation.

Wim Leers’s picture

Status: Needs review » Needs work
  1. #35: All good points! I remember this is the outcome of discussions with @phenaproxima, @seanB, @ckrina and others at Drupal Dev Days Cluj. But things were more abstract then and more concrete today. I think it's good to take another critical look at this 👍😊
  2. #37.2 + #40.2: this is to match the behavior that #2994696: Render embedded media items in CKEditor introduced:
              // Convert data-align attribute to class so we're not applying styles
              // to data attributes.
              // @todo Consider removing this in https://www.drupal.org/project/drupal/issues/3072279
              if (this.data.attributes.hasOwnProperty('data-align')) {
                this.element
                  .getParent()
                  .addClass(`align-${this.data.attributes['data-align']}`);
              }
    

    It's impossible to do this all on the server side, because that will cause the align-* classes to no longer be removed when appropriate on the client side.

  3. #37.5: There are "element" DOM nodes, "text" DOM nodes, and so on.
  4. #37.10: This is common in JS. Double negation to map truthy values to actual booleans.
  5. #37.12+19: This follows the pattern established by \Drupal\editor\Form\EditorImageDialog::buildForm().
  6. #37.15 + #40.15: the answer is in the long docblock just above 😉 And yes, that's copy/pasted verbatim from EditorImageDialog. It probably needs tweaking for the difference in context (<drupal-media> versus <img>).
  7. #37.18 + #40.18: LOL! That seems to have been wrong for over 5 years. Harmless, but yeah, let's keep only one of those two (whichever one makes it work).
  8. #37.21: An existing value that is FALSE is not the same as a non-existing value.
  9. #40.7: Ahhh, so that's why that file was being loaded until I removed it in #26! Odd that I wrote that it doesn't exist when it does. I must've done something wrong. This is the correct thing to do.

Patch review

  1. Overall, many of the changes that #40 made by EditorMediaDialog seem to not have been tested. Best indicator of this: there's a PHP syntax error on line 151…
  2. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -265,10 +260,20 @@
    +           * element and not a non-node element (such as text).
    

    🤓 s/non-node element/non-element node/

  3. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -278,9 +283,13 @@
    +          // If there is a link, the top-level element is the a tag,
    

    🤓s/the a tag/the tag/

  4. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -278,9 +283,13 @@
    +          // the parent element must be position relative.
    

    s/position/positioned/

  5. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -322,6 +332,9 @@
    +                  // Coerces to boolean. If it was falsey (e.g. 0, null,
    +                  // undefined, etc.), it will be false, otherwise, true.
    +                  // @see https://stackoverflow.com/a/10597474/1214689
    

    This is odd at first for a PHP developer but is trivial for any JS developer. I don't think we should be documenting this here. There are more than a dozen occurrences of this in core's JS alone, hundreds if you include our JS libs.

  6. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.js
    @@ -126,12 +126,10 @@
    -          if (this.oldData) {
    -            if (!this.data.hasCaption && this.oldData.hasCaption) {
    -              delete this.data.attributes['data-caption'];
    -            } else if (this.data.hasCaption && !this.oldData.hasCaption) {
    -              this.data.attributes['data-caption'] = ' ';
    -            }
    +          if (!this.data.hasCaption) {
    +            delete this.data.attributes['data-caption'];
    +          } else if (this.data.hasCaption && this.oldData && !this.oldData.hasCaption) {
    +            this.data.attributes['data-caption'] = ' ';
               }
    

    👎 The if-test ensured this code only ran during changes. It needed comments added, not to be deleted.

  7. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -69,17 +74,23 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
    -      $has_caption = $form_state->getUserInput()['editor_object']['hasCaption'];
    -      $form_state->set('hasCaption', $has_caption);
    +      $form_state->set('hasCaption', !empty($editor_object['hasCaption']));
    

    👎 This change broke the editing of the metadata pretty much completely 😞

  8. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -181,17 +191,22 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    -      $form_state->setValue(['attributes', 'alt'], FALSE);
    +      $form_state->unsetValue(['attributes', 'alt']);
         }
         if ($form_state->hasValue(['attributes', 'title']) && trim($form_state->getValue(['attributes', 'title'])) === '') {
    -      $form_state->setValue(['attributes', 'title'], FALSE);
    +      $form_state->unsetValue(['attributes', 'title']);
    +    }
    ...
    +    // If `data-align`is set to "none", remove the attribute.
    +    if ($form_state->hasValue(['attributes', 'data-align']) && $form_state->getValue(['attributes', 'data-align']) === 'none') {
    +      $form_state->unsetValue(['attributes', 'data-align']);
         }
    

    👎 This somehow broke edits after changing a left/center/right-aligned media embed "none" alignment. Go for example from centered to not aligned at all, then try to edit it again, the dialog won't even load.

  9. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -98,9 +109,6 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
    -      if ($alt === '' && !empty($media_embed_element['src'])) {
    -        $alt = '""';
    -      }
    

    🤔 I'm not sure this change is correct. Also see my response to #37.15 and #40.15.

  10. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -181,17 +191,22 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
         // The `alt` and `title` attributes are optional: if they're not set, their
         // default values simply will not be overridden.
         if ($form_state->hasValue(['attributes', 'alt']) && trim($form_state->getValue(['attributes', 'alt'])) === '') {
    -      $form_state->setValue(['attributes', 'alt'], FALSE);
    +      $form_state->unsetValue(['attributes', 'alt']);
         }
         if ($form_state->hasValue(['attributes', 'title']) && trim($form_state->getValue(['attributes', 'title'])) === '') {
    -      $form_state->setValue(['attributes', 'title'], FALSE);
    +      $form_state->unsetValue(['attributes', 'title']);
    +    }
    +
    +    // If `data-align`is set to "none", remove the attribute.
    +    if ($form_state->hasValue(['attributes', 'data-align']) && $form_state->getValue(['attributes', 'data-align']) === 'none') {
    +      $form_state->unsetValue(['attributes', 'data-align']);
         }
    

    👎 Unsetting those values means the client side will not receive any value for those attributes. This is wrong. We should explicitly inform the client side about new values. The client side can then choose how to act based on the new set of attributes. All attributes the form/dialog receives should also be sent back to the client, so the client is explicitly informed about all changes.

oknate’s picture

1. I started eyeballing the changes at a certain point and didn’t test them all. I figured I’d see if anything would fail on testbot. So that’s embarrassing. 😁
2.  ✅Updated text.
3. leaving “a” in but adding backtick quotes. I’m trying to say the top-level element is an a tag in this case.
4.  ✅Updated text.
5. ✅ Removed the comment.
6. ✅ Reverted the change and added a comment about the if statement.
7. ✅ Reverted this change. UPDATE restoring in #43. This wasn't breaking the form.
8. ✅ Reverted this change.
#42.1 I don't know why I thought this was working last night. I tested it last night, but I can't get it to work today.
I had changed this to server side:

    // If `data-align`is set to "none", remove the attribute.
    if ($form_state->hasValue(['attributes', 'data-align']) && $form_state->getValue(['attributes', 'data-align']) === 'none') {
      $form_state->unsetValue(['attributes', 'data-align']);
    }

This was breaking the test this morning and I couldn't get it to work.

Restoring this code:

+          } else {
+            this.element.removeAttribute('data-align');
+            var classes = this.element.getParent().$.classList;
+            for (var i = 0; i < classes.length; i++) {
+              if (classes[i].substr(0, 6) === 'align-') {
+                this.element.getParent().removeClass(classes[i]);
+              }
+            }

If I understand it correctly, the widget element doesn't rebuild its classes when the alignment changes on the server side. The preview changes, but that's within the widget element. So we have to manually update the widget elements classes. This was a bug I fixed (#29.3), then broke again last night. I'm not sure it warrants a comment, and before I leave one, I'd like Wim to corroborate the reason: That a server side preview update doesn't adjust the classes of the widget. Is that right?

It seems right, because the test coverage shows the data-align is empty when set to none in the form:

    $this->assertFalse($drupal_media->hasAttribute('data-align'));
    }

So the drupal-media tag is getting updated, but not the widget.
9. ✅ Reverted this change. “I'm not sure this change is correct.” I’m not either, so reverting for now.
10. ✅ Reverted this change. See #44 for more detail.

Update regarding #37.22, this needed the third parameter which allows strings to be tested.

-    if (is_a($item_class, ImageItem::class)) {
+    if (is_a($item_class, ImageItem::class, TRUE)) {

This completely broke the form, as a string cannot be evaluated as an ImageItem object.

oknate’s picture

Re: #37.12 and #41.7:

Restoring this change:

+      $editor_object = $form_state->getUserInput()['editor_object'];
       // By convention, the data that the text editor sends to any dialog is in
       // the 'editor_object' key.
-      $media_embed_element = $form_state->getUserInput()['editor_object']['attributes'];
+      $media_embed_element = $editor_object['attributes'];
       $form_state->set('media_embed_element', $media_embed_element);
-      $has_caption = $form_state->getUserInput()['editor_object']['hasCaption'];
+      $has_caption = $editor_object['hasCaption'];

It was not this that was breaking the form. It was this:

-    if (is_a($item_class, ImageItem::class)) {
+    if (is_a($item_class, ImageItem::class, TRUE)) {

That needed the TRUE set in the third parameter to work with a string.

oknate’s picture

More on #41.10:

when "values" is what's returned from the editor:

                CKEDITOR.tools.extend(values.attributes, widget.data.attributes);

                Object.keys(values.attributes).forEach(function (prop) {
                  if (values.attributes[prop] === false || prop === 'data-align' && values.attributes[prop] === 'none') {
                    delete values.attributes[prop];
                  }
                });

If you go from widget.data.attributes set to '""' (empty property indicator) and then re-open the editor and you set it to a blank string, then this code won't send an update:

    if ($form_state->hasValue(['attributes', 'title']) && trim($form_state->getValue(['attributes', 'title'])) === '') {
      $form_state->unsetValue(['attributes', 'title']);
    }

So if you have a property, and you want to signal to update it, you'd have to detect that no value is sent back from the dialog, and at that point unset it. It's easier to just always send the properties back from the dialog. If it were handled in the javascript, you'd have to hard code which properties need to be tracked (for now alt and title), but if someone extends the form, we'd want to allow other properties to pass through. This was something we ran into with Entity Embed. So rather than track the state of properties in the javascript by seeing if the dialog DOESN'T return the value, it's better to keep the current code and return all the attributes you want to signal might have changed. That way if you unset one, you don't need to detect that it wasn't passed back.

Wim Leers’s picture

Status: Needs work » Needs review
  1. #42.1: Don't worry! I’ve been in that place dozens of times 😅 You’d think you’d learn after the first, second or third time. But no, I also do this still when I’m convinced it’s solid. I totally get that you wanted to get it posted so I would review it! But in this case, the total set of changes is also so big that it’s really really hard to review the change compared to the previous patch
  2. I'd like Wim to corroborate the reason: That a server side preview update doesn't adjust the classes of the widget. Is that right?

    Yes, the server responds with HTML that ends up being set inside the <drupal-media> element. this.element is the <drupal-media> element. this.element.getParent() is the wrapper element (with the .cke_widget_wrapper class). The server-generated HTML therefore can never affect that wrapper's class. And that is what ckeditor.drupalmedia.css's .cke_widget_drupalmedia.align-center {…} style is targeting!

phenaproxima’s picture

Status: Needs review » Needs work

Another review (full read-through, including tests). This is shaping up nicely, I think.

  1. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -160,12 +160,27 @@
    +              this.data.attributes['data-caption'] = ' ';
    

    I was going to suggest setting placeholder text here, but in manual testing it looks like it gets a default placeholder (which is immediately changeable) of "Enter caption here". Which is totally fine. So it doesn't look like any action is needed here. 👍

  2. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -179,6 +194,14 @@
    +              if (classes[i].substr(0, 6) === 'align-') {
    

    Nit: I think classes[i].indexOf('align-') === 0 might be clearer.

  3. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -179,6 +194,14 @@
    +                this.element.getParent().removeClass(classes[i]);
    

    This could be classes.remove(classes[i]).

  4. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,9 +247,124 @@
    +           * element and not a non-node none (such as text).
    

    "non-node none"? :)

  5. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,9 +247,124 @@
    +                  Object.keys(values.attributes).forEach(prop => {
    +                    if (
    +                      values.attributes[prop] === false ||
    +                      (prop === 'data-align' &&
    +                        values.attributes[prop] === 'none')
    +                    ) {
    +                      delete values.attributes[prop];
    +                    }
    +                  });
    

    We could probably increase the readability of this function a bit if we use Object.entries() in concert with destructuring. So something like: Object.entries(values.attributes).forEach([key, value] => { ... }).

  6. +++ b/core/modules/media/media.routing.yml
    @@ -48,3 +48,11 @@ media.filter.preview:
    +editor.media_dialog:
    +  path: '/editor/dialog/media/{editor}'
    +  defaults:
    +    _form: '\Drupal\media\Form\EditorMediaDialog'
    +    _title: 'Edit media'
    +  requirements:
    +    _entity_access: 'editor.use'
    

    Should we explicitly require the POST method for this route?

  7. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +    elseif ($form_state->get('media_embed_element')) {
    

    I think this could be $form_state->has('media_embed_element').

  8. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +      if ($alt === '' && !empty($media_embed_element['src'])) {
    

    The mystery of "why would the media embed element have an src attribute" is not solved yet, nor made clear by the long comment nearby. Do we actually expect an src attribute to be here? If so, where would it be coming from?

  9. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +      if (!empty($settings['alt_field'])) {
    +        $form['alt'] = [
    +          '#type' => 'textfield',
    +          '#title' => $this->t('Alternate text'),
    +          '#default_value' => $alt,
    +          '#description' => $this->t('Short description of the image used by screen readers and displayed when the image is not loaded. This is important for accessibility.'),
    +          '#required_error' => $this->t('Alternative text is required.<br />(Only in rare cases should this be left empty. To create empty alternative text, enter <code>""

    — two double quotes without any content).'),
    + '#maxlength' => 512,
    + '#placeholder' => $media->{$image_field}->alt,
    + '#parents' => ['attributes', 'alt'],
    + ];
    + }
    + if (!empty($settings['title_field'])) {
    + $form['title'] = [
    + '#type' => 'textfield',
    + '#title' => $this->t('Title'),
    + '#default_value' => $title,
    + '#description' => t('The title is used as a tool tip when the user hovers the mouse over the image.'),
    + // Maxlengths for `title`and `alt` fields follow ImageItem schema.
    + // @See core/modules/image/src/Plugin/Field/FieldType/ImageItem::schema()
    + '#maxlength' => 1024,
    + '#placeholder' => $media->{$image_field}->title,
    + '#parents' => ['attributes', 'title'],
    + ];
    + }

    🤔I'm wondering if we should create the alt and title fields unconditionally, but only allow or deny #access to them based on field settings. This might give contrib and custom code a bit more leeway.

  10. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +    if ($editor->getFilterFormat()->filters('filter_align')->status) {
    

    Kind of a nit, but...although I know that the filter plugin collection will return a new instance of a given filter if it's not configured yet, that is kind of "magic" and not really great for DX. I would prefer if we did something like this, to make our assumptions explicit:

    $filters = $editor->getFilterFormat()->filters();
    
    if ($filters->has('filter_align') && $filters->get('filter_align')->status) { ... }
    
  11. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +        '#wrapper_attributes' => ['class' => ['container-inline']],
    +        '#attributes' => ['class' => ['container-inline']],
    

    We should still remove one of these, as per Wim's +1.

  12. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +   * @return string|null
    +   *   String of image field name.
    

    Should be "The name of the image source field configured for the media item, or NULL if the source field is not an image field."

  13. +++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
    @@ -124,6 +124,7 @@ public function getCssFiles(Editor $editor) {
    +      $this->moduleExtensionList->getPath('media_library') . '/css/media_library.theme.css',
    

    This needs a comment.

  14. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -428,6 +435,9 @@ protected function applyPerEmbedMediaOverrides(\DOMElement $node, MediaInterface
    +        if ($node->getAttribute('title') === '""') {
    +          $node->setAttribute('title', NULL);
    +        }
    

    I would like @andrewmacpherson to validate this, but one thing I know he has repeatedly said is that, for images, the title attribute is close to useless. For the sake of keeping things as simple as humanly possible for end users, I wonder if we should remove support for title and leave it to contrib.

  15. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -147,7 +152,7 @@ public function testOnlyDrupalMediaTagProcessed() {
    -    $this->getSession()->switchToIFrame('ckeditor');
    +    $session->switchToIFrame('ckeditor');
    

    Technically, these sorts of changes are out of scope, but I'm not hugely concerned. :)

  16. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -244,23 +251,52 @@ public function testPreviewUsesDefaultThemeAndIsClientCacheable() {
    +    $this->assertSame('baz', $figcaption->getHtml());
    +    // Test that disabling the caption in the metadata dialog removes it
    

    There should be a blank line between these two lines, to split up the "steps" we're taking in this test. I generally agree with Wim that well-placed blank lines will make the test considerably easier to read. :)

  17. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -347,27 +382,184 @@ public function testEditableCaption() {
    +      $this->config('field.field.media.image.field_media_image')
    +        ->set('settings.alt_field', $data['settings']['settings.alt_field'])
    +        ->set('settings.title_field', $data['settings']['settings.title_field'])
    +        ->save();
    +      $this->container->get('cache.discovery')->deleteAll();
    

    It seems like this might be better done with FieldConfig::loadByName()->setSetting()->setSetting()->save().

  18. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -347,27 +382,184 @@ public function testEditableCaption() {
    +      $alt = !empty($page->findField('attributes[alt]'));
    +      $this->assertSame($data['expected_fields']['attributes[alt]'], $alt);
    +      $title = !empty($page->findField('attributes[title]'));
    +      $this->assertSame($data['expected_fields']['attributes[title]'], $title);
    

    These can be collapsed into one line: $this->assertSame($data['expected_fields']['attributes[alt]'], $assert_session->fieldExists('attributes[alt]')->getValue()). If the field doesn't exist, the failure message will be clearer. :)

  19. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -347,27 +382,184 @@ public function testEditableCaption() {
    +    $img = $assert_session->waitForElementVisible('xpath', '//img[contains(@alt, "default alt")]');
    

    We also need to assertNotEmpty($img).

  20. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -347,27 +382,184 @@ public function testEditableCaption() {
    +    $this->assertEquals('default title', $img->getAttribute('title'));
    

    We should be using assertSame() here.

  21. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -347,27 +382,184 @@ public function testEditableCaption() {
    +    $alt = $page->findField('attributes[alt]');
    +    $title = $page->findField('attributes[title]');
    +    $this->assertSame($who_is_zartan, $alt->getValue());
    +    $this->assertSame($decepticons, $title->getValue());
    

    We should use $assert_session->fieldExists() to get the alt and title, or we could end up calling getValue() on null.

  22. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -347,27 +382,184 @@ public function testEditableCaption() {
    +    // Test that setting value to back to empty string restores the default values.
    

    Nit: > 80 characters.

  23. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -377,9 +569,11 @@ public function testLinkability($drupalimage_is_enabled) {
    +              unset($group['items'][$item_key]);
    +              break;
                 }
               }
    +          $settings['toolbar']['rows'][$row_key][$group_key]['items'] = array_values($group['items']);
    

    Why did this change?

  24. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,28 +805,163 @@ public function previewAccessProvider() {
    +      $widget = $assert_session->elementExists('css', '.cke_widget_drupalmedia.align-' . $alignment);
    +      $this->assertNotEmpty($widget);
    

    There's no need for the assertEmpty() here; the test will fail if the widget does not exist. (That's what $assert_session->elementExists() is for.)

  25. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,28 +805,163 @@ public function previewAccessProvider() {
    +      $source = $assert_session->elementExists('css', 'textarea.cke_source');
    +      $value = $source->getValue();
    

    This can be collapsed to one line: $value = $assert_session->elementExists()->getValue().

  26. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,28 +805,163 @@ public function previewAccessProvider() {
    +    if ($drupal_media->hasAttribute('data-align')) {
    +      // Wait for element to update.
    +      $page->waitFor(10, function () use ($drupal_media) {
    +        return empty($drupal_media->hasAttribute('data-align'));
    +      });
    +    }
    

    Shouldn't we be collecting and asserting the result of waitFor()?

  27. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,28 +805,163 @@ public function previewAccessProvider() {
    +    $source = $assert_session->elementExists('css', 'textarea.cke_source');
    +    $value = $source->getValue();
    

    This can also be collapsed to one line.

  28. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,28 +805,163 @@ public function previewAccessProvider() {
    +    $this->assertEmpty($drupal_media->getAttribute('data-align'));
    

    I think we can use $this->assertFalse($drupal_media->hasAttribute()) here.

  29. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,28 +805,163 @@ public function previewAccessProvider() {
    +    $dialog_buttons = $this->assertSession()
    +      ->elementExists('css', 'div.ui-dialog-buttonpane');
    +    $this->assertNotEmpty($dialog_buttons);
    +    $dialog_buttons->pressButton('Save');
    

    This can be one line: $this->assertSession()->elementExists()->pressButton().

effulgentsia’s picture

I just now started testing #43 and noticed that in the dialog, the Align radio buttons and the Caption checkbox appear even if data-align and data-caption are not added as allowed attributes to <drupal-media>. Is that intentional? I would have expected those UI elements to not be in the dialog in that case, but is there a reason that they need to be?

effulgentsia’s picture

Oh, but now I also see that those UI elements aren't there if I disable the "align images" and "caption images" filters. So maybe it was just my mistake for leaving those enabled simultaneously with not adding the data attributes to <drupal-media>?

effulgentsia’s picture

I also just now tested it with the alt attribute. I removed the alt attribute from <drupal-media> in the filter settings. But the UI still shows me the field to override the Alternate text. I don't think it should do that. I don't know if it's valuable to still display the default alternate text in such a case, but even if it is, the UI shouldn't let you change it if the change won't survive the filter.

Wim Leers’s picture

#48: That's indeed what it boils down to. This is also how EditorImageDialog handles data-align and data-caption. I think it could be argued those form elements should indeed only be displayed if those attributes are allowed. Let's do that in a follow-up, where we then do it for bot EditorImageDialog and EditorMediaDialog?

effulgentsia’s picture

Maybe it's ok to do #50 in a followup, but my concern is this... Standard Profile enables those filters by default, but when you add the Media button to the toolbar, it does not add in the attributes to it by default. So it's easy to get into the situation of #48 as I did.

Would it make sense in this issue to add validation to the filter configuration that if those filters are enabled, then their attributes are required to also be enabled in the <drupal-media> tag? Then perhaps in the followup to #50, we could relax that requirement (if we e.g., want to support a site configuration that allows those attributes on <img> but not on <drupal-media>)?

oknate’s picture

We're running into issues in entity embed too where people are editing alt and title and then those attributes are being stripped out. I think adding validation that the related attribute is allowed either in the dialog or in the editor config form or both would be a positive thing.

Here are some suggested validation rules:

  • If you have filter_caption filter enabled then the drupal-media tag should require data-caption attribute.
  • If you have filter_align filter enabled then the drupal-media tag should require data-align attribute.
  • In the dialog, the alt field shouldn't show unless the alt attribute is allowed on the drupal-media tag.
  • In the dialog, the title field shouldn't show unless the title attribute is allowed on the drupal-media tag.
phenaproxima’s picture

I just demoed this patch to the UX team. Feedback was overwhelmingly positive, to the point where I think we can remove the "needs usability review" tag. @andrewmacpherson was also in the call and had accessibility feedback, but will remove the "needs accessibility review" tag when his changes have been made and all appropriate follow-ups have been filed.

We have several follow-ups to file:

  • The team felt that it was entirely too easy to accidentally stray-click the "X" icon and remove an embed. They asked for a confirmation dialog to be displayed first. Since the existing image embed button does not display an "X" (you just have to click the embedded image and press Delete on your keyboard), we agreed to remove the "X" button from this patch and add it back, with a confirmation alert box, in a follow-up.
  • In #46.14, I wondered if we should bother supporting the title attribute. @andrewmacpherson said that he would like to rip the title attribute out of core entirely, so it seems that's a big +1. So let's totally remove support for that. Nice!
  • Everyone agreed that the UX of the "Caption" checkbox is strange. However, it is a pre-existing pattern in core; the image button does it too, exactly the same way. We all felt it would be best for them to behave consistently. So let's open a follow-up issue to improve the UX of the captioning in the embed and image dialogs in the same go -- ideally, checking the "Caption" box would expose a text field with the caption in it, and it would be live-editable in the editor.
  • The caption filter in core adds a role="group" attribute to the figure tag it creates. It would be more accessible to have that either be role="figure", or (even better) just removed entirely. We'll do that in a follow-up.
  • 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.
  • The edit and delete buttons should also be button tags, not links. I don't think this is a hard blocker if it's hard to do, but ideally we should fix that before commit.
  • The edit and delete buttons might not be optimal from a contrast perspective. (The contrast between the white background and the grey border needs to be at least 3:1 in order to satisfy WCAG 2.1.) However, both icons are pre-existing in core and not introduced by this patch. So let's open a follow-up to revisit and improve the contrast of core icon sets.
andrewmacpherson’s picture

Re. #53:

  1. The list of a11y issues looks right. After the necessary follow-ups have been filed, and/or fixes made in patches here, the "needs accessibility review" can be removed. Leave it in place until then.
  2. I think there may already be an issue for WCAG 2.1 icon contrast somewhere.
  3. <figure role="group"> can be replaced with <figure>, losing the role attribute entirely. The group role is technically permitted on the figure element, but the default implicit role is figure. The figure role was introduced in ARIA 1.1, and the HMTL 5.2 role mapping was updated accordingly. Our existing use of role=group in our filter figure probably arose because it was created prior to ARIA 1.1 - so that's really a modernization issue.
  4. Reasons why the HTML title attribute isn't good for accessibility: Using the HTML title attribute – updated

    Basically, it's content that is only available to sighted hover-capable pointer users.

    Wordpress has already removed support for the title attribute in content like this, and Joomla has removed the vast majority of title attributes from the admin UI. It's something that Drupal is specifically called out for in Tenon Research first glimpse: The best & worst of content management systems.

    For reducing our use of the title attribute, I was thinking of a meta-plan issue called "Stop using the HTML title attribute in Drupal" with child issues for all the places we use it in a non-inclusive manner. The end game would be getting rid of the vast majority of cases. Image Field is a bit more nuanced, because it's part of then configuration per field instance. Custom modules in existing sites might rely on it (who knows) so that may need a deprecation plan.

    For media entity embed, all we need to do is avoid adding any new support for title attribute.

andrewmacpherson’s picture

My roadmap triage for the accessibility bit, whether they get fixed here or in follow-ups.

  1. Confirmation alert for the remove button: unsure. I don't really have a strong view on whether this is an accessibility need, so I'll go with whatever the general usability triage is.
  2. Title attribute on embedded media: WON'T FIX here. The first quick-win in a plan to get rid of the title attribute everywhere :-)
  3. Fixing the role of the figure element in figure filter plugin: SHOULD-HAVE.
  4. Disambiguate edit/remove buttons, with media entity label: SHOULD-HAVE. I'm tempted to say it's a must-have, but I'm not sure I can justify that with reference to WCAG. Be aware this one could turn into a must-have.
  5. Edit and remove buttons as <button> elements: strong SHOULD-HAVE. Helps speech control users primarily, and screen reader users secondarily. It's mitigated by the fact that screen reader will have a new dialog announced, assuming the confirm-delete issue is dealt with.
  6. Fix contrast on edit and remove buttons. COULD-HAVE. It's a core-wide issue, so not a blocker for media initiative. Maybe it deserves to be a must-have for Claro?
oknate’s picture

Status: Needs work » Needs review
FileSize
29.22 KB
52.11 KB

Responses to #46:
1. It would be nice to expose an api to set the default caption, in a follow-up.
2. ✅Updated to if indexOf('align-‘) instead of substr(0, 6) === 'align-‘.
3. 👎If it makes no difference removeClass(classes[i]); is shorter than classes.remove(classes[i]), but that’s interesting.
I didn’t know about that.
4. ✅ 😱 "non-node none" change to “non-node element”.
5. 👎 I came up with this:

for (let [prop, value] of Object.entries(values.attributes)) {
                    if (value === false || (prop === 'data-align' && value === 'none')) {
                      delete values.attributes[prop];
                    }
                  }

but it results in all this code in the javascript:

try {
                  for (var _iterator = Object.entries(attributes)[Symbol.iterator](), _step; !(_iteratorNormalCompletion = (_step = _iterator.next()).done); _iteratorNormalCompletion = true) {
                    var _ref = _step.value;

                    var _ref2 = _slicedToArray(_ref, 2);

                    var prop = _ref2[0];
                    var value = _ref2[1];

                    if (value === false || prop === 'data-align' && value === 'none') {
                      delete values.attributes[prop];
                    }
                  }
                } catch (err) {
                  _didIteratorError = true;
                  _iteratorError = err;
                } finally {
                  try {
                    if (!_iteratorNormalCompletion && _iterator.return) {
                      _iterator.return();
                    }
                  } finally {
                    if (_didIteratorError) {
                      throw _iteratorError;
                    }
                  }
                }

Seems this results in overkill for the given task. For now, I’ll leave it as is.

6. ✅ Added “methods: [POST]”. Since we’re only using this for posting, I don’t see what it hurts. The only caveat is the other routes whose pattern this follows don’t do this.

7. ✅ Changed to use $form_state->has('media_embed_element’)

8. ✅This is pretty clearly copied from EditorImageDialog:

// The alt attribute is *required*, but we allow users to opt-in to empty
// alt attributes for the very rare edge cases where that is valid by
// specifying two double quotes as the alternative text in the dialog.
// However, that *is* stored as an empty alt attribute, so if we're editing
// an existing image (which means the src attribute is set) and its alt
// attribute is empty, then we show that as two double quotes in the dialog.
// @see https://www.drupal.org/node/2307647
$alt = isset($image_element['alt']) ? $image_element['alt'] : '';
if ($alt === '' && !empty($image_element['src'])) {
$alt = '""';
}
From this patch: https://www.drupal.org/files/issues/2307647-52.patch on this issue: https://www.drupal.org/node/2307647

So I think this is pointless, and this isn’t relevant:

that *is* stored as an empty alt attribute

nor this:

if we're editing an existing image

This isn’t an img and drupal-media will never have a src attribute, and it won’t store the empty alt as an empty attribute, but as two double quotes. It works switching it back and forth.

9. ✅Switched to creating alt field unconditionally and added #access parameter that uses the condition.

10. 🤔This seems needlessly verbose. I’d like to get Wim’s opinion before changing this. It comes over exactly from Entity Embed.

11. ✅ Removed '#wrapper_attributes' => ['class' => ['container-inline’]],. Interestingly, it didn’t work to remove the other one.

12. ✅Updated the verbiage. We should eventually update this in Entity Embed too.

13. ✅ Added comment.

14. No longer relevant now that we’re dropping title field support for this issue.

15. I know it’s kind of out of scope, but it’s the same test file, and I’m changing it one place, so it seems appropriate to change it across the one file.

16. ✅ Added line.

17. ✅Updated to use FieldConfig instead of $this->config.

18. Refactored this away, so no longer relevant.

19. ✅Added not empty assertions in two places.

20. No more assertEquals left now.

21. ✅Fixed line longer than 80 characters.

23. See #2994702-32: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption` Fixing failure in 29.

24. ✅Removed unnecessary assertNotEmpty. I think I was confusing this with the waitforElementVisible.

25. ✅Collapsed per your recommendation.

26. ✅Updated.

27. ✅Updated.

28. ✅Yes, that seems to work, and it’s more exact.

Response to #53,

1. ✅Removed remove button.
2. ✅Removed title field.
6. ✅converted edit link to edit button.

Also:
Fixed a few es-lint warnings.

Wim Leers’s picture

#51:

Would it make sense in this issue to add validation to the filter configuration that if those filters are enabled, then their attributes are required to also be enabled in the <drupal-media> tag? Then perhaps in the followup to #50, we could relax that requirement (if we e.g., want to support a site configuration that allows those attributes on <img> but not on <drupal-media>)?

I'd personally be fine with that.

But we need to be aware that this is reducing user freedom. It's intentional that data-caption and data-align are optional. A site may only want <code> and <blockquote>to be captioned for example, and not media. In that scenario, @Filter=filter_caption would be enabled, but the allowed_html setting of @Filter=filter_html would be:

<p> <a href hreflang> <em> <strong> <code data-caption><blockquote data-caption> <drupal-media>

Adding the validation logic you suggest would make this impossible. I do think that'd be reasonable though, and would make updating an existing site to use @Filter=media_embed certainly a lot smoother! 👍

I needed to call out that downside. But I personally think it's pretty odd to allow alignment and captions on some things, but explicitly not on media embeds. So I do agree this is an excellent idea! 👏

phenaproxima’s picture

Wim Leers’s picture

  1. Removal of the "X" icon 👍 I also felt this was odd.
  2. Follow-up to deal with the "Caption" checkbox for both EditorImageDialog and EditorMediaDialog 👍
  3. Regarding role=group and #54.3: this was added based on the guidance of accessibility maintainer @mgifford in #2509700: Add role="group" to <figure> to meet WCAG requirements. If you create a follow-up, please link it to that, and incorporate that discussion.
  4. RE: button tags instead of links, and their contrast. If we do this, we'll also need to change it in the media library. This functionality is specifically reusing the edit/delete links/buttons from the Media Library. See #3039829: Remove link to media item from media library view.. This patch is not introducing anything new on that front! Looks like that other patch didn't get the necessary accessibility review then.
  5. 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?
Wim Leers’s picture

d.o can be annoyingly stupid sometimes.

EDIT: Also, ignore my #59.3 and #59.4, since @phenaproxima already filed follow-ups for those while I was writing #59 obviously :)

oknate’s picture

Addressing Effulgentsia's issue raised in #47 - 51:

I removed the alt attribute from in the filter settings. But the UI still shows me the field to override the Alternate text. I don't think it should do that.

This patch adds access restriction to the fields in the dialog if the related attributes aren't set in filter_html and filter_html is enabled.

Wim Leers brought up the issue that if no fields are accessible that we'll have an empty dialog. Phenaproxima, Wim Leers and I discussed various options, such as hiding the edit button in that situation, but it was decided that could be confusing, as they wouldn't have any indication why the edit button wasn't there. So for now we're outputting a message in the dialog in that situation.

message when no fields are accessible due to filter_html restrictions

Note, we're adding the validation in the dialog rather than when editing the editor / filter format form. Wim Leers brought a good point in #57 that someone might want data-caption enabled on other tags, but not drupal-media. So it doesn't make sense to add validation there. And adding it in the dialog works.

Update: one post comment regret:

After posting this I realized these conditions can be combined:

+      if (!empty($allowed_attributes)) {
+        if (empty($allowed_attributes['alt'])) {
+          $form['alt']['#access'] = FALSE;
+        }
+      }

It can be
if (!empty($allowed_attributes) && empty($allowed_attributes['alt'])) { etc.

phenaproxima’s picture

@oknate, @Wim Leers, and I discussed this for a while in Slack. It has the potential to be a serious UX bikeshed, because there doesn't seem to be a clearly "correct" solution.

The options we discussed:

Validate the filter format configuration and require the alt, data-align, and data-caption attributes if the media is an image, filter_align is enabled, and filter_caption is enabled, respectively

Pros: This makes it pretty much impossible for site builders to configure the format such that the editor dialog is presenting useless options to users.
Cons: As Wim pointed out in #57, this is effectively forcing a certain configuration, which may not suit all (or even most) sites. We'd be likely to hear about it pretty soon.

Have the JavaScript check if the embedded media allows alt, data-align, and data-caption attributes, and hide the Edit button if it doesn't

Pros: This does not put users into the possible situation of opening a useless dialog.
Cons: It makes the behavior of the embedded media item inconsistent, for reasons that might not be clear to the user. Do we really expect authors to understand (and rightfully so) the intricacies of text formats? If the button doesn't appear, it looks like a silent failure -- chances are, the user expects that, if the "embed media" button present in CKEditor, then they can embed media in a consistent way. Also, modules could modify the dialog, or the relevant JavaScript settings, and therefore throw a monkey wrench into this idea.

In the dialog, hide the alt, alignment, and caption fields if the format has filter_html enabled and the corresponding attributes are not allowed on the drupal-media tag. Display a message if no attributes are shown, so as to not look like a bug.

Pros: Gives site builders maximum flexibility and accounts for the extreme edge case of all attributes being disallowed.
Cons: In that extreme edge case, there will be an Edit button that opens a useless dialog.

On balance, we felt that option #3 is the best. It only becomes a problem for sites that are configured in a very strange, and probably very rare, way. So that is what @oknate implemented in #61.

I would make one suggestion, though: the current error message ("There is nothing to override for this media") is not actionable. I would suggest we extend it so that, if the current user has the "administer filters" permission, provide a link to edit the filter format. Something like "Edit the text format to modify the attributes that can be overridden."

Additionally, in the description of the MediaEmbed filter, we should add some documentation. Currently, the description is "Embeds media items using a custom HTML tag. If used in conjunction with the 'Align/Caption' filters, make sure this filter is configured to run after them." I would suggest this:

Embeds media items using a custom HTML tag. If used in conjunction with the 'Align/Caption' filters, make sure this filter is configured to run after them, and that the data-align and/or data-caption attributes are allowed on the drupal-media tag.

phenaproxima’s picture

Issue tags: -Needs followup

Removing the "needs followup" tag, as we have implemented an approach to solving @effulgentsia's concern in #49 through #51, and all other remaining follow-ups are open.

phenaproxima’s picture

Status: Needs review » Needs work

Closing in, I think! This looks really, really good!!

  1. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +247,109 @@
    +           *   A dom node to evaluate.
    

    s/dom/DOM

  2. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +247,109 @@
    +          // To allow the edit and remove buttons to be absolutely positioned
    +          // the parent element must be positioned relative.
    

    There is no more remove button. :)

  3. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +247,109 @@
    +          const editButton = CKEDITOR.dom.element.createFromHtml(
    

    Nit: A blank line before this one would be useful.

  4. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +247,109 @@
    +          // Make the buttons do things.
    

    s/buttons/button

  5. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,251 @@
    +        '#access' => !empty($settings['alt_field']),
    +      ];
    +
    +      if (!empty($allowed_attributes)) {
    +        if (empty($allowed_attributes['alt'])) {
    +          $form['alt']['#access'] = FALSE;
    +        }
    +      }
    

    This can all become one line: '#access' => !empty($settings['alt_field']) && !empty($allowed_attributes['alt'])

  6. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,251 @@
    +      '#access' => !empty($editor->getFilterFormat()->filters('filter_align')->status),
    +    ];
    +
    +    if (!empty($allowed_attributes)) {
    +      if (empty($allowed_attributes['data-align'])) {
    +        $form['align']['#access'] = FALSE;
    +      }
    +    }
    

    Same here.

  7. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,251 @@
    +      '#access' => !empty($editor->getFilterFormat()->filters('filter_caption')->status)
    +    ];
    +
    +    if (!empty($allowed_attributes)) {
    +      if (empty($allowed_attributes['data-caption'])) {
    +        $form['caption']['#access'] = FALSE;
    +      }
    +    }
    

    And here.

  8. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,251 @@
    +    // The `alt` and `title` attributes are optional: if they're not set, their
    +    // default values simply will not be overridden.
    +    if ($form_state->hasValue(['attributes', 'alt']) && trim($form_state->getValue(['attributes', 'alt'])) === '') {
    +      $form_state->setValue(['attributes', 'alt'], FALSE);
    +    }
    +    if ($form_state->hasValue(['attributes', 'title']) && trim($form_state->getValue(['attributes', 'title'])) === '') {
    +      $form_state->setValue(['attributes', 'title'], FALSE);
    +    }
    

    It's not clear how the FALSE signifies "use the default value". A @see referencing the relevant JavaScript would be useful.

  9. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -428,6 +435,9 @@ protected function applyPerEmbedMediaOverrides(\DOMElement $node, MediaInterface
    +        if ($node->getAttribute('title') === '""') {
    +          $node->setAttribute('title', NULL);
    +        }
    

    We don't need this anymore :)

  10. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -244,23 +251,53 @@ public function testPreviewUsesDefaultThemeAndIsClientCacheable() {
    +    // Test that enabling the caption in the metadata dialog adds an editable
    

    This should have a blank line above it; it's a new "phase" of the test.

  11. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -244,23 +251,53 @@ public function testPreviewUsesDefaultThemeAndIsClientCacheable() {
    +    $drupal_media = $assert_session->waitForElementVisible('css', 'drupal-media figcaption', 2000);
    +    $this->assertNotEmpty($drupal_media);
    

    These can be one line. Also, let's add a blank line below it.

  12. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -347,27 +382,205 @@ public function testEditableCaption() {
    +  public function testAlt() {
    

    This test definitely needs some well-placed empty lines in it; it's hard to parse right now.

  13. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -377,9 +590,11 @@ public function testLinkability($drupalimage_is_enabled) {
    -              unset($settings['toolbar']['rows'][$row_key][$group_key]['items'][$item_key]);
    +              unset($group['items'][$item_key]);
    +              break;
                 }
               }
    +          $settings['toolbar']['rows'][$row_key][$group_key]['items'] = array_values($group['items']);
    

    These changes seem out of scope...

  14. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -275,12 +275,14 @@
    +           *   A dom node to evaluate.
    

    s/dom/DOM

  15. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -347,27 +382,205 @@ public function testEditableCaption() {
    +    $filter_format = $this->container->get('entity_type.manager')->getStorage('filter_format')->load('test_format');
    

    I think we can use the static FilterFormat::load() here. :)

  16. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -347,27 +382,205 @@ public function testEditableCaption() {
    +    $this->container->get('cache.discovery')->deleteAll();
    

    Why is this needed? A comment might be useful here.

  17. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -347,27 +382,205 @@ public function testEditableCaption() {
    +    $filter_format
    +      ->setFilterConfig('filter_caption', [
    +      'status' => FALSE,
    +    ])->setFilterConfig('filter_align', [
    +      'status' => FALSE,
    +    ])->save();
    +    // Wait for preview.
    +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', 'drupal-media', 2000));
    +    $page->pressButton('Edit media');
    +    $this->waitForMetadataDialog();
    +    $assert_session->fieldNotExists('attributes[data-align]');
    +    $assert_session->fieldNotExists('hasCaption');
    +    $page->pressButton('Close');
    +    $session->switchToIFrame('ckeditor');
    

    Let's also assert that the alt text field is still present.

  18. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,28 +826,127 @@ public function previewAccessProvider() {
    +  public function testAlignment() {
    

    This test method is a wall of text. Can we break it up a bit with some empty lines? :)

  19. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,28 +826,127 @@ public function previewAccessProvider() {
    +      $widget = $assert_session->elementExists('css', '.cke_widget_drupalmedia.align-' . $alignment);
    

    Why do we have $widget as its own variable? Doesn't look like we're using it...?

  20. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,28 +826,127 @@ public function previewAccessProvider() {
    +    $drupal_media = $assert_session->waitForElementVisible('xpath', '//drupal-media', 2000);
    +    $this->assertNotEmpty($drupal_media);
    +    if ($drupal_media->hasAttribute('data-align')) {
    +      // Wait for element to update without data-align attribute.
    +      $result = $page->waitFor(10, function () use ($drupal_media) {
    +        return empty($drupal_media->hasAttribute('data-align'));
    +      });
    +      $this->assertTrue($result);
    +    }
    +    $this->assertFalse($drupal_media->hasAttribute('data-align'));
    

    I suspect this could be a lot simpler -- one line, actually: $this->assertNotEmpty($assert_session->waitForElementVisible('css', 'drupal-media:not([data-align])', 2000)).

  21. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,28 +826,127 @@ public function previewAccessProvider() {
    +    $figure = $assert_session->waitForElementVisible('css', '.caption-drupal-media', 2000);
    +    $this->assertNotEmpty($figure);
    +    $widget = $assert_session->elementExists('css', '.cke_widget_drupalmedia');
    +    foreach ($alignments as $alignment) {
    +      $this->assertFalse($figure->hasClass('align-' . $alignment));
    +      $this->assertFalse($widget->hasClass('align-' . $alignment));
    +    }
    

    We could use a similar approach here too. :not() is a pretty nice CSS feature :)

oknate’s picture

Addressing feedback in #64:
1 -4 ✅ Updated.
5, 6, and 7. I agree it can be one line, but the example code you posted is missing a condition.
What about when $allowed_attributes is empty? That means filter_html is disabled! I came up with this one-liner that I think is more readable but still one line:
!empty($settings['alt_field']) && ($filter_html->status === FALSE || !empty($allowed_attributes['alt']))
8. ✅Added a note.
9. ✅ I deleted this, a little reluctantly. If we don’t add this, then we’re changing it in the filter for alt, but not for title. Until title is removed from the MediaEmbedFilter, I think we should leave this. But for now I’m deleting it, based on your suggestion.
10. ✅Added line of whitespace.
11. ✅Updated many similar instances in the test, then undid some because they're out of scope.
12. ✅ Added empty lines and comments to testAlt().
13. ✅ This code was fixing a bug described in #29. But since the ‘undo’ and ‘redo’ were only be ing used for the delete button, we can drop those buttons and drop this code.
14. ✅Updated.
15. ✅Switched to using FilterFormat::load instead of entity type manager.
16. ✅Added comment.
17. ✅Added assertions about the alt field not affecting changes to filter_align and filter_caption status. I don't love this change, because it's unnecessary and it confuses what we're testing at that moment. But I can see the argument that the access rules for the other fields might after tis field.
18. ✅ Added some space. I also created some helper functions to reduce the lines of code within the method, which will also make it more readable. I'm not sure if you'll think that's good, or call it "needless abstraction".
19. ✅I think I refactored away where that was being used. I removed it.
20. ✅Nice, this worked:

waitForElementVisible('css', 'drupal-media:not([data-align])', 2000)
21.  ✅This worked too: 
<code->elementExists('css', '.cke_widget_drupalmedia:not([class*="align-"])');

I'm pretty sure with 20 and 21 we won't get false negatives, because I tested it earlier where the attribute and class are present on their respective selectors and the code failed, which means there wasn't a drupal-media element without the data-align attribute earlier, and since we know there is a drupal-media element there, that means that what failed was the not part of the selector. The one in 21 with a wildcard, the same thing. I found an example on stack exchange and verified earlier in the same test method that it failed when there was a .cke_widget_drupalmedia element present, which means the part that failed was the not selector.

oknate’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

Everything in here is a nitpick. Feel free to disregard any of them.

  1. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    +    $filter_html = $editor->getFilterFormat()->filters('filter_html');
    +    $filter_align = $editor->getFilterFormat()->filters('filter_align');
    +    $filter_caption = $editor->getFilterFormat()->filters('filter_caption');
    

    Nit: Can we do $filters = $editor->getFilterFormat()->filters() above this section, and reuse that?

  2. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,237 @@
    +    if ($filter_html->status) {
    +      $restrictions = $filter_html->getHTMLRestrictions();
    +      $allowed_attributes = $restrictions['allowed']['drupal-media'];
    +    }
    

    I think we also need a else { $allowed_attributes = []; }, no?

  3. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -244,13 +245,42 @@ public function testPreviewUsesDefaultThemeAndIsClientCacheable() {
    +    $this->assertNotEmpty($figcaption = $assert_session->waitForElement('css', 'figcaption'));
    

    Not a pattern I've seen elsewhere in core, but it should work :)

  4. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -244,13 +245,42 @@ public function testPreviewUsesDefaultThemeAndIsClientCacheable() {
    +    $this->assertNotEmpty($drupal_media = $assert_session->waitForElementVisible('css', 'drupal-media figcaption', 2000));
    

    It's not clear if this needs the inline assignment. Do we use $drupal_media again in this section of the test? If not, we should remove the assignment.

  5. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -244,13 +245,42 @@ public function testPreviewUsesDefaultThemeAndIsClientCacheable() {
    +    // Type in the widget's editable for the caption.
    

    This seems oddly phrased...

  6. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +387,209 @@ public function testEditableCaption() {
    +  /**
    +   * Tests field access logic in EditorMediaDialog.
    +   */
    

    This should clarify, I think, that we are only testing access for metadata like alignment and captioning, which is based on the way that the filters are configured. The current text of the comment sounds like we are testing access to configurable entity fields.

  7. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,28 +833,125 @@ public function previewAccessProvider() {
    +   * @throws \Behat\Mink\Exception\ElementNotFoundException
    +   */
    +  protected function fillFieldInMetadataDialog($locator, $value) {
    

    This does not need the @throws, because this method does not explicitly throw an exception.

  8. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,28 +833,125 @@ public function previewAccessProvider() {
    +   * @throws \Behat\Mink\Exception\ElementNotFoundException
    +   */
    +  protected function submitDialog() {
    

    Same here.

  9. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,28 +833,125 @@ public function previewAccessProvider() {
    +   * @throws \Behat\Mink\Exception\ElementNotFoundException
    +   */
    +  protected function closeDialog() {
    

    And here.

  10. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -754,4 +1073,22 @@ protected function clickPathLinkByTitleAttribute($text) {
    +   * @throws \Behat\Mink\Exception\ElementNotFoundException
    +   */
    +  protected function getDrupalMediaFromSource() {
    

    And here too :)

I'm on the fence about some of the inline variable assignments in tests ($this->assertNotEmpty($foo = $assert_session->waitForSomething())). It should work okay, but I'm not sure about changing some of the existing code to use this pattern. It introduces patch noise. If you'd rather have it be consistent in the tests, then I'm thinking we should maybe abandon the inline assignments for now and go back to the pre-existing pattern.

oknate’s picture

Status: Needs work » Needs review
FileSize
12.4 KB
42.29 KB

1. ✅Updated.
2. ✅ Added back $allowed_attributes = []; . That was inadvertently removed.
3. ✅I don’t love it, as I think it’s less readable. But it is on one line, as per previous feedback.
4. I’m having trouble finding this particular line of code. I checked all of the inline assignments like this “$this->assertNotEmpty($drupal_media” and I couldn’t find one where the variable is not being used later.
5. ✅Updated the reference to an “editable” to “CKEDITOR.editable”
6. ✅Updated the comment to “Test the EditorMediaDialog's form elements' #access logic.”
7-10. ✅ Removed the extra throws
11. ✅Removed the extraneous inline variable assignments.

phenaproxima’s picture

Status: Needs review » Needs work

One small request: can we update the media_embed filter's description as I requested in #62? This tripped up @seanB in his own manual testing, so it would probably be valuable to do.

seanB’s picture

FileSize
2.41 MB

Finally had some time so tried out patch #68. Great work everyone, this is looking great.
I ran into a couple of issues. Attached is a video. I was using chrome 76.0.3809.100 on MacOS. I also configured the 'media library' view mode for embeds in the text format.

  1. The edit button is not shown in the top right corner.
  2. After inserting the image first thing, I can no longer type in the editor? When inserting an image in between text, I can type around the image. The video might show it a bit better.
  3. At first I didn't have the options to override metadata. Phenaproxima explained that you need to enabled alt, data-align, and data-caption on the drupal-media tag. That could use some more guidance in the interface, I wouldn't have figured that out without someone telling me.
  4. The override metadata popup could probably make the save button use the primary action (blue button). It is a bit of a nit, but I think that would be better.
  5. I ran into a JS error while making the video. Couldn't insert anything anymore, had to refresh the page.
  6. I am allowed to select more than 1 item, but when doing that only 1 item is actually inserted?
  7. The override metadata dialog does not use the UX we agreed upon when creating the mockups. With the explicit override button to allow empty override and make it more explicit that there is a default. I am ok with making that a followup though. Override metadata modal sketch.
  8. I see there is a followup to bring back the delete button, so have posted my opinion regarding that in the other issue
  9. I don't see a followup yet to use the media name in the Edit button label. When using a screen reader it might not be clear which image you are editing when you have multiple images. I see @andrewmacpherson labeled this a should have, so a followup would be fine.
oknate’s picture

1. ✅You’re right about the positioning of the edit button. When I removed the delete button, I failed to notice the absolutely positioned edit button was taking the now non-existent delete button into account. It was set to be positioned absolutely "right: 40px". Added an override and a note with the css override to remove the override in the follow-up issue.
2. This is known CKEditor behavior. See the video for how to work around this.

https://www.drupal.org/files/issues/2019-08-16/annoying-ckeditor-functio...

This was raised on the last issue too. See #2994699-71: Create a CKEditor plugin to select and embed a media item from the Media Library.4. Wim responded in #78 in that issue:

Welcome to HTML rich text editors, they sometimes have to do crazy things :) The same already exists with all other block-level elements in CKEditor. That "tiny red arrow thingy" is something CKEditor added many, many years ago. We cannot fix this in Drupal. We'd have to work with the CKEditor team to come up with a better solution.

3. ✅Added a link to the editor / text format form when no fields show and additional help text there.

Here's the link in the dialog. I was going back and forth on adding the name of the format. But I think it's unnecessary.
link in dialog

Here's the new description in context. I made a few changes to phenaproxima's help text to match the other descriptions, such as dropping "HTML tag" and replacing it with just "tag".
new MediaEmbed filter description in context.

4 .✅ Added the classes ‘button’ and ‘button—primary’. The save button is now blue in themes that apply that style.

5 and 6. This looks like a bug with the previous issue #2994699: Create a CKEditor plugin to select and embed a media item from the Media Library. I tested a little using Umami and I couldn't reproduce the issue where there's a javascript error.

oknate’s picture

In response to #70.6:

Nice catch, Sean B. I have opened an issue for this: #3075443: Follow-up for #2994699: Cardinality should be 1 for inserting embedded media. This was just a oversight in the final patch for #2994699: Create a CKEditor plugin to select and embed a media item from the Media Library.

oknate’s picture

Status: Needs work » Needs review

Marking back as needs review. I think I've addressed all of the latest feedback.

Wim Leers’s picture

Status: Needs review » Needs work

#62's summary sounds mostly accurate. I disagree with the con for the second one, like I've said in that Slack chat (A) the same user tends to have access to the same text format everywhere, B) within a text format all embedded media are always treated the same), and it's missing one con (what about additional attributes that contrib modules might add? This would then require the server side to inform the JS about those additional attributes so that it'd be able to still make the correct dynamic decision).

#70.7: That's because the current patch is not yet allowing the overriding of any media metadata, it's only allowing the editing of "decorative metadata" (alignment and captioning). Oh, no, this is wrong, because I ported the alt/title overriding from EditorImageDialog in #14. You're absolutely right! 👍


  1. So many concerns about this! It should only be displayed to people who have the necessary permissions, it should use the text format's label, and if we only display this to admins, then what's the point?
  2. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -179,6 +194,14 @@
    +            this.element.removeAttribute('data-align');
    

    I think this can be removed?

  3. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +247,110 @@
    +           * Determine if element is a node.
    

    This text is inaccurate.

    s/if element is a node/a node is an element node/

    That'd make it correct. But it'd still be kind of confusing.

  4. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +247,110 @@
    +           *   Returns true if element.nodeType is equal to 1, meaning a node
    

    "equal to 1" is meaningless here.

  5. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +247,110 @@
    +           *   element and not a non-node element (such as text).
    

    s/non-node element/non-element node/

    (Already said this in #41.)

  6. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,243 @@
    +    elseif ($form_state->has('media_embed_element')) {
    ...
    +    else {
    +      throw new BadRequestHttpException("Unable to find the required media embed data.");
    +    }
    

    I think this is needlessly complex; this if-test and exception are effectively checking that Form API isn't broken. This should just do what EditorImageDialog already does.

  7. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,243 @@
    +    $media = $this->entityRepository->loadEntityByUuid('media', $media_embed_element['data-entity-uuid']);
    ...
    +        '#placeholder' => $media->{$image_field}->alt,
    

    This should load the alt value that matches the language of the host entity.

    Do we have test coverage for that?

  8. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,243 @@
    +        '#maxlength' => 512,
    

    Why 512? EditorImageDialog uses 2048. If we deviate from that, this needs a comment.

  9. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,243 @@
    +        '#markup' => $this->t('There is nothing to override for this media. Edit the %format to modify the attributes that can be overridden.', ['%format' => $text_format_edit_link]),
    

    "There is nothing to override for this media." sounds pretty strange.

    What about "This media embed is not customizable."?

    Also see my prior remark about the subsequent bit of text.

  10. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,243 @@
    +    // The `alt` and `title` attributes are optional: if they're not set, their
    +    // default values simply will not be overridden. It's important to set
    +    // these to FALSE instead of unsetting the value so that We explicitly
    +    // inform the client side about the new value.
    +    if ($form_state->hasValue(['attributes', 'alt']) && trim($form_state->getValue(['attributes', 'alt'])) === '') {
    +      $form_state->setValue(['attributes', 'alt'], FALSE);
    +    }
    

    This is not handling the '""' case. See \Drupal\editor\Form\EditorImageDialog::submitForm().

  11. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,243 @@
    +    if ($form_state->hasValue(['attributes', 'title']) && trim($form_state->getValue(['attributes', 'title'])) === '') {
    +      $form_state->setValue(['attributes', 'title'], FALSE);
    +    }
    

    This is dead code. title is gone as of some comments ago.

  12. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -26,7 +26,7 @@
    + *   description = @Translation("Embeds media items using a custom tag, <code>&lt;drupal-media&gt;

    . If used in conjunction with the `Align images` and `Caption images` filters, make sure this filter is configured to run after them. If you are using the `Limit allowed HTML tags and correct faulty HTML` filter, be sure the data-align and/or data-caption attributes are allowed on the &lt;drupal-media&gt; tag. If you would like users to be able to override the alt text on image media, add the alt attribute as well to the &lt;drupal-media&gt; tag."),

    If we're going to literally quote the labels of filters, let's not make them part of this translatable string, but let's make them parameters instead.

    More importantly, I don't understand why we're changing them here.

  13. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -414,6 +414,13 @@ protected function applyPerEmbedMediaOverrides(\DOMElement $node, MediaInterface
    +        // Allow the display of the image without an alt tag in special cases.
    +        // Since setting the value in the EditorMediaDialog to an empty string
    +        // restores the default value, this allows special cases where the
    +        // alt should not be set to the default value but should be empty.
    +        if ($node->getAttribute('alt') === '""') {
    +          $node->setAttribute('alt', NULL);
    +        }
    

    This requires expanded test coverage in \Drupal\Tests\media\Kernel\MediaEmbedFilterTest.

phenaproxima’s picture

So many concerns about this! It should only be displayed to people who have the necessary permissions, it should use the text format's label, and if we only display this to admins, then what's the point?

Agreed about the permissions and the text format label. The point, however, is to give the poor user who sees this message some sort of action to do, or otherwise point them in a useful direction, rather than just throwing our hands in the air and giving up (almost like we just don't care.....sorry, I had to make that joke). It's entirely possible (probable, even) that they are seeing this message due to a misconfiguration, and if we don't offer them some sort of corrective path to follow, they will be on a slippery slope to Angryville.

I would argue that it's most valuable to display this message to privileged users, because they will be the people who most likely misconfigured things in the first place. But in any event, they will be the people in the greatest position to fix it. So let's make that task as easy for them as we can.

oknate’s picture

Responding to feedback in #74:
1. ✅I think this is helpful to show for admins. I feel the most useful thing would be to show this to users who have access and suppress the edit button for those who don’t. For this patch, I’ll just conditionally show the second sentence.
2. ✅ Yes, that can be removed. It does mean that you’ll have a stale attribute on the drupal-media element temporarily, but when downcast it’s removed.
3. ✅ Updated. I added a little more description to make it less confusing. But we already have a link there, so I’m not sure how much documentation we need here.
4. ✅ Dropped the equal to 1 comment.
5. ✅ Switched. Sorry for having to put that in twice.
6. ✅ Dropping the exception for now. We didn’t have test coverage for it yet anyway.
7. @todo I'll look into this. I'm pretty sure this isn't currently working, just based on looking at the code.
8. ✅Switched to 2048. I think we were enforcing this to match ImageItem::schema(), but since it doesn’t actually save this data to the database, that’s unnecessary. I added functional javascript test coverage, but it’s slow, so I removed it. I could add test coverage for this in the MediaEmbedFilterTest that really long alt text overrides work, if you feel it would be worth it.
9. 👎 I like the current version better. Your version seems to imply in no situation can these be overridden. But I guess the current one does too. Maybe we should do something like. “This media embed is not customizable with the current site configuration.”
10. The conversion of the empty string signifier can’t happen in the submit function, as it does for the EditorImageDialog. I made this change in #29. If you don’t pass the two double quotes on to the attributes, but pass an empty string, it will display the default value, not an empty alt tag. The test coverage added in #29 demonstrates this.
11. ✅ Removed the title code.
12. ✅ Switched the references to the filters back to how they were.
13. ✅ Added test coverage. I had some trouble getting it to determine that the alt tag is set to NULL with SimpleXML. SimpleXML seems to show it as NULL,

SimpleXMLElement Object
(
    [0] => 
)

but when I try to assert that it’s equal to NULL, it fails.

Failed asserting that '' is null

But i think testing it’s equal to ‘’ is fine. Maybe we should add a raw html assertion for that part.

The big @todo from #74 is to fix the media alt text default in the dialog to use the language of the host entity, and add test coverage. I'll work on this.

oknate’s picture

Forgot to commit one change, deleting the assertion about a drupal-media element without data-align attribute, related to #74.2.

oknate’s picture

Addressing #74.7

  • Fixes language of alt text when viewing translation.
  • Adds test coverage.

I think the test could be cleaned up a bit because it recreates the media and host entities that were created in the ::setUp() method. And I think if I can figure out how to set them to translatable, we won't need that, we can use the ones created in the ::setUp() method.

I noticed there doesn't seem to be data-langcode support for embedded media in languages other than the host entity's language. It's out of scope for this issue, but if added, it require an update here to switch to using the data-langcode as the language. I added an issue for it: #3075593: Allow MediaEmbed filter to use data-langcode to set media translation.

oknate’s picture

UPDATE: this was fixed in #80, and this code was dropped.

I don't think the following code is right, even though given the current context, the test is giving the right result.

    $langcode = $this->languageManager
      ->getCurrentLanguage()
      ->getId();

On the form route editor.media_dialog I don't think that would detect the current language in all language negotiation configurations.

oknate’s picture

Status: Needs work » Needs review
FileSize
53.44 KB
6 KB
19.97 KB

Continuing to address #74.7. We actually fixed this langcode passing from host to dialog in entity embed: #3018485: Selected Entity should display in host entity's language in EntityEmbedDialog when data-langcode not set

Adapting that solution. It gets the langcode from the field holding the text format, if available, or else it pulls it from the host entity. With this, we can drop the language negotiation code. This is good news, since the parent's entity language code is much less variable than the language negotiator, which can be configured many different ways and even customized with custom plugins.

oknate’s picture

FileSize
53.41 KB
19.48 KB
773 bytes

Removing a line of test code.

oknate’s picture

Removing unused use statements.

phenaproxima’s picture

I took a look at the fix @oknate implemented for #74.7. I agree that porting the fix from Entity Embed (#3018485: Selected Entity should display in host entity's language in EntityEmbedDialog when data-langcode not set, for reference) is the right way to go; that code is generic enough to work in core, and it was extensively iterated on by @oknate and @Wim Leers in that issue. So, I'm okay with continuing down this path. :)

phenaproxima’s picture

Status: Needs review » Needs work

All aboard the review train! 🚂

  1. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -152,6 +152,10 @@
    +          const hostEntityLangcode = document.getElementById(editor.name).getAttribute('data-media-embed-host-entity-langcode');
    +          if (hostEntityLangcode) {
    +            data.langcode = hostEntityLangcode;
    +          }
    

    This could use a @see referencing where the attribute is added.

  2. +++ b/core/modules/media/media.module
    @@ -496,3 +498,24 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
    +      $entity = $form_state->getFormObject()->getEntity();
    +      if ($entity instanceof ContentEntityInterface) {
    +        $element['#attributes']['data-media-embed-host-entity-langcode'] = $entity->language()->getId();
    +      }
    

    Small nit here: the language() method is part of EntityInterface, not ContentEntityInterface. Additionally, this hook is only run for field widgets, which by definition can only exist on fieldable (i.e., content) entities. So I'm not sure exactly what is being added by this additional if check...

  3. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,247 @@
    +    if (!empty($editor_object['langcode'])) {
    +      if ($media->hasTranslation($editor_object['langcode'])) {
    +        $media = $media->getTranslation($editor_object['langcode']);
    +      }
    +    }
    

    Nit: This seems like it could be collapsed into a single (albeit longer) if statement. Also, it might be helpful to have a comment here about why we are loading a translation. Additionally, this should be done inside the following if ($image_field = ...) check, since we do not need to load the translation if we're not dealing with an image.

  4. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,247 @@
    +    if ($form['alt']['#access'] === FALSE && $form['align']['#access'] === FALSE && $form['caption']['#access'] === FALSE) {
    

    This assumes that $form['alt'] exists at all, which it might not if the media being embedded is not an image.

  5. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,247 @@
    +      $form['no_access_notice'] = [
    +        '#markup' => $this->t('There is nothing to override for this media.'),
    +      ];
    

    I don't think we need to bikeshed the wording of this message right now. Since it's just a string, it can change all the way up until RC.

  6. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,247 @@
    +          '#markup' => $this->t('There is nothing to override for this media. Edit the text format %format to modify the attributes that can be overridden.', ['%format' => $text_format_edit_link]),
    

    The text of the link should be "Edit the text format $name", so that screen readers will read it as one coherent action. We can accomplish this by using $format->toUrl() and putting the a tag directly into the translated string, with the URL as a replaced variable. So:

    $edit_url = $format->toUrl('edit-form')->toString();
    $form['no_access_notice']['#markup'] = $this->t('There is nothing to override for this media. <a href="@edit_url">Edit the text format %format</a> to modify the attributes that can be overriden.', ['@edit_url' => $edit_url, '%format' => $format->label()]);
  7. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,247 @@
    +    // When the `alt` attribute is set to two double quotes, transform it to the
    +    // empty string: two double quotes signify "empty alt attribute". See above.
    +    if (trim($form_state->getValue(['attributes', 'alt'], '')) === '""') {
    +      $form_state->setValue(['attributes', 'alt'], '""');
    +    }
    

    Not a big deal, but I still don't see how this is different from calling $form_state->setValue(['attributes', 'alt'], trim($form_state->getValue(['attributes', 'alt'], ''))).

  8. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,247 @@
    +    // The `alt` and `title` attributes are optional: if they're not set, their
    +    // default values simply will not be overridden. It's important to set
    +    // these to FALSE instead of unsetting the value so that We explicitly
    +    // inform the client side about the new value.
    +    if ($form_state->hasValue(['attributes', 'alt']) && trim($form_state->getValue(['attributes', 'alt'])) === '') {
    +      $form_state->setValue(['attributes', 'alt'], FALSE);
    +    }
    

    The comment still mentions the 'title' attribute. :) Also, supernit: the word "We" is capitalized, but shouldn't be.

  9. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -26,7 +26,7 @@
    + *   description = @Translation("Embeds media items using a custom tag, &lt;drupal-media&gt;. If used in conjunction with the 'Align/Caption' filters, make sure this filter is configured to run after them. If you are using the HTML filter, be sure the data-align and/or data-caption attributes are allowed on the &lt;drupal-media&gt; tag.  If you would like users to be able to override the alt text on image media, add the alt attribute as well to the &lt;drupal-media&gt; tag."),
    

    "If you are using the HTML filter..." could be potentially confusing phrasing. However, this is just a string, and we can change strings anytime before RC. So I do NOT think we should block on this.

    I'd like a slight rephrasing, though, at the end: "...add the alt attribute to the drupal-media tag as well."

  10. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,320 @@ public function testEditableCaption() {
    +    // Now test that adding the attributes to the allowed HTML will allow
    +    // the fields to display in the dialog.
    +    $allowed_html = str_replace('<drupal-media data-entity-type data-entity-uuid data-view-mode>', '<drupal-media alt data-align data-caption data-entity-type data-entity-uuid data-view-mode>', $allowed_html);
    +    $filter_format->setFilterConfig('filter_html', [
    +      'status' => TRUE,
    +      'settings' => [
    +        'allowed_html' => $allowed_html,
    +      ],
    +    ])->save();
    +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', 'drupal-media', 2000));
    +    $page->pressButton('Edit media');
    +    $this->waitForMetadataDialog();
    +    $assert_session->fieldExists('attributes[alt]');
    +    $assert_session->fieldExists('attributes[data-align]');
    +    $assert_session->fieldExists('hasCaption');
    +    $page->pressButton('Close');
    +    $this->getSession()->switchToIFrame('ckeditor');
    

    Let's also assert that our error text does not appear.

  11. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,320 @@ public function testEditableCaption() {
    +    // Test that setting disabling `filter_caption` and `filter_align` disables
    +    // the respective fields in the dialog.
    

    The word "setting" is superfluous here.

  12. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,320 @@ public function testEditableCaption() {
    +    $alt = $page->findField('attributes[alt]');
    +    // Assert that the placeholder is set to the value of the media field's
    +    // alt text.
    +    $this->assertSame('default alt', $alt->getAttribute('placeholder'));
    

    This should be doing $alt = $assert_session->fieldExists('attributes[alt]')->getValue(), or we risk calling getAttribute() on NULL.

  13. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,320 @@ public function testEditableCaption() {
    +    // Assert that the img within the media embed within the ckeditor contains
    +    // the overridden alt text set in the dialog.
    +    $this->assertNotEmpty($img = $assert_session->waitForElementVisible('xpath', '//img[contains(@alt, "' . $who_is_zartan . '")]'));
    

    We don't need the $img = assignment here, or anywhere else that we're not actually using the variable for anything. :)

  14. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,320 @@ public function testEditableCaption() {
    +    // Test the process against with a different alt text to make sure it works
    +    // the second time around.
    +    $cobra_commander_bio = 'The supreme leader of the terrorist organization Cobra';
    +    // Set the alt field to the new alt text.
    +    $alt->setValue($cobra_commander_bio);
    +    $this->submitDialog();
    +    $this->getSession()->switchToIFrame('ckeditor');
    +    // Assert that the img within the media embed within the ckeditor contains
    +    // the overridden alt text set in the dialog.
    +    $this->assertNotEmpty($img = $assert_session->waitForElementVisible('xpath', '//img[contains(@alt, "' . $cobra_commander_bio . '")]'));
    +
    +    // Reopen the dialog.
    +    $page->pressButton('Edit media');
    +    $this->waitForMetadataDialog();
    +    // The alt field should now display the override instead of the default.
    +    $alt = $page->findField('attributes[alt]');
    +    $this->assertSame($cobra_commander_bio, $alt->getValue());
    

    Do we really need to test this twice? If not, we might as well remove it, unless there's some specific edge case we're trying to prevent. In which case we need to document that.

  15. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,320 @@ public function testEditableCaption() {
    +    $alt->setValue('""');
    

    Let's set the value to "" (note the extra space), to ensure that it is properly trimmed. (Please add a comment about that too, so our future selves don't think it was a typo.)

  16. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,320 @@ public function testEditableCaption() {
    +    \Drupal::service('module_installer')->install(['language', 'content_translation']);
    +    $this->rebuildContainer();
    

    I think we should probably use $this->resetAll(), rather than $this->rebuildContainer().

  17. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,320 @@ public function testEditableCaption() {
    +    $media_fr = $media->addTranslation('fr');
    +    $media_fr->name = "Tatou poilu hurlant";
    +    $media_fr->field_media_image->setValue([
    +      [
    +        'target_id' => '1',
    +        'alt' => "texte alternatif par défaut",
    +        'title' => "titre alternatif par défaut",
    +      ],
    +    ]);
    +    $media_fr->save();
    

    As a lapsed non-native francophone, I approve this message. 👍

  18. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,320 @@ public function testEditableCaption() {
    +    $translation->body->value = $host->body->value;
    +    $translation->body->format = $host->body->format;
    +    $translation->body->lang = 'fr';
    

    Do we actually need to set $translation->body->lang? As far as I can tell, that is not an actual property of text fields.

  19. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,320 @@ public function testEditableCaption() {
    +    $alt = $page->findField('attributes[alt]');
    +    // Assert that the placeholder is set to the value of the media field's
    +    // alt text.
    +    $this->assertSame('texte alternatif par défaut', $alt->getAttribute('placeholder'));
    

    This also needs to use the $assert_session->fieldExists()->getAttribute() pattern.

  20. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,34 +959,126 @@ public function previewAccessProvider() {
    +      $this->assertNotEmpty($assert_session->waitForElementVisible('css', 'drupal-media[data-align="' . $alignment . '"]', 2000));
    +      $this->assertNotEmpty($assert_session->waitForElementVisible('css', '.caption-drupal-media.align-' . $alignment, 2000));
    

    This is a bit misleading. Having two separate waits makes it look like we're waiting for one thing to show up after the other, which, if I understand correctly, is not the case. Can we combine the selector into a single string that reflects the thing we actually expect to show up? If I understand the element hierarchy, it will be something like this:

    $selector = sprintf('drupal-media[data-align="%s"] .caption-drupal-media.align-%s", $alignment, $alignment);

    Either that, or we should change the second call to waitForElementVisible() to $assert_session->elementExists().

  21. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,34 +959,126 @@ public function previewAccessProvider() {
    +      $drupal_media = $this->getDrupalMediaFromSource();
    +      $this->assertSame($alignment, $drupal_media->getAttribute('data-align'));
    

    If the drupal-media element did not appear, we risk calling getAttribute() on NULL, which is an ugly test failure that makes debugging harder. Maybe this could use a $this->assertNotEmpty($drupal_media) before asserting the alignment.

  22. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,34 +959,126 @@ public function previewAccessProvider() {
    +      // Leave source mode.
    +      $this->pressEditorButton('source');
    +      // Having entered source mode means we need to reassign an id to the
    +      // CKEditor iframe.
    

    ...er, did we enter source mode or leave it? The comments contradict each other.

  23. +++ b/core/modules/media/tests/src/Kernel/MediaEmbedFilterTest.php
    @@ -214,11 +214,15 @@ public function testOverridesAltAndTitle($title_field_property_enabled, array $e
    +      'title' => 'title 4',
    

    Although we are no longer supporting the title attribute, it makes sense for us to keep things consistent in the tests until a larger effort to remove all title support is undertaken. So I'm okay with this.

  24. +++ b/core/modules/media/tests/src/Kernel/MediaEmbedFilterTest.php
    @@ -244,11 +252,11 @@ public function providerOverridesAltAndTitle() {
    +      '`title` field property enabled ⇒ `title` is overridable' => [
    

    'title' is overridable? It's...not, though. We don't support it. Why do we need this test case?

oknate’s picture

Status: Needs work » Needs review
FileSize
15.64 KB
53.66 KB

Responding to feedback in #84:
1. ✅ Added a note.
2. ✅ Removed the entity check and added a check that getEntity isn’t NULL.

-    else {
-      $entity = $form_state->getFormObject()->getEntity();
-      if ($entity instanceof ContentEntityInterface) {
-        $element['#attributes']['data-media-embed-host-entity-langcode'] = $entity->language()->getId();
-      }
+    elseif ($entity = $form_state->getFormObject()->getEntity()) {
+      $element['#attributes']['data-media-embed-host-entity-langcode'] = $entity->language()->getId();
     }

3. ✅ Combined the conditions.

-    if (!empty($editor_object['langcode'])) {
-      if ($media->hasTranslation($editor_object['langcode'])) {
-        $media = $media->getTranslation($editor_object['langcode']);
-      }
+    if (!empty($editor_object['langcode']) && $media->hasTranslation($editor_object['langcode'])) {
+      $media = $media->getTranslation($editor_object['langcode']);
     }

4. ✅ Adding an empty check on $form[‘alt’], good catch there.

-    if ($form['alt']['#access'] === FALSE && $form['align']['#access'] === FALSE && $form['caption']['#access'] === FALSE) {
+    if ((empty($form['alt']) || $form['alt']['#access'] === FALSE) && $form['align']['#access'] === FALSE && $form['caption']['#access'] === FALSE) {

5. 👍 Good to know.
6. ✅ Sounds good.
7. One sets it to an empty string, the other sets it to two double quotes. In Entity Embed we used a constant, in fact I think this was based on feedback from you:

/**
 * A string that signifies not to render the alt text.
 *
 * @const string
 */
const EMPTY_STRING = '""';

Do you think we should use a constant to make it clearer? When porting this from Entity Embed, Wim replaced the constant with a simple string, which is why I've maintained it this way.
8. ✅ Updated.
9. ✅ Rephrased per your recommendation.
10. ✅ Added an assertion the error message doesn’t appear.
11. ✅ Removed superfluous word.
12. ✅ Updated.
13. ✅ Updated.
14. These aren't redundant. These are testing two different locations one outside the dialog and one within. One is testing the alt override is present in preview in the CKEditor, the next is testing that the override still shows up as the value in the dialog upon reopening it. I added some verbiage to clarify what we’re testing, so they don’t as much the same. If we removed the second one and just reset the value of the field, we wouldn't has test coverage that the value that was set the first time shows up as the default value in the form field upon reopening it.
15. ✅ Added whitespace after the double quotes and a note.
16. ✅ Changed $this->rebuildContainer() to $this->resetAll().
17. ✅ J’aime aussi le Français! Je suis heureux pour avoir un moyen de l’utilser!
18. ✅ Good call, I checked and setting ->langcode on this fieldItemList does nothing. If you call getLangcode() right after that, it doesn’t pull the value you set.
19. ✅ Updated.
20. ✅ Good call. I had the two wait statements because the preview loads separately than the drupal-media element. But with the combined selector, we can wait for both of them with the alignment change. Nice.
21. Added assertion that $drupal_media = $this->getDrupalMediaFromSource(); isn’t NULL. Also, I updated the return statement in the method to indicate that it could return NULL.

@@ -1195,8 +1197,8 @@ protected function clickPathLinkByTitleAttribute($text) {
   /**
    * Get the drupal-media element from the ckeditor source.
    *
-   * @return \DOMNode
-   *   The drupal-media element.
+   * @return \DOMNode|null
+   *   The drupal-media element or NULL if it can't be found.
    */
   protected function getDrupalMediaFromSource() {
     $this->pressEditorButton('source');
@@ -1205,7 +1207,13 @@ protected function getDrupalMediaFromSource() {
       ->getValue();
     $dom = Html::load($value);
     $xpath = new \DOMXPath($dom);
-    return $xpath->query('//drupal-media')[0];
+    $list = $xpath->query('//drupal-media');
+    if (!empty($list[0])) {
+      $drupal_media = $list[0];
+      $this->assertInstanceOf(\DOMNode::class, $drupal_media);
+      return $drupal_media;
+    }
+    return NULL;
   }
 
 }

22. Changed the note to “// Press the source button again to leave source mode.” to make it clear that yes, we’re indeed leaving source mode here.
23. 👍
24. We do support the title attribute override in the MediaEmbed filter. This test was added back with the original filter. I’m just adding a test case for the empty string indicator (‘“”’), and I realized this language was wrong when it said it wasn’t overridable.
If you look at the test, the title attribute added to drupal-media tag will override the value, so that description that it’s not overridable is incorrect.

Status: Needs review » Needs work

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

oknate’s picture

Status: Needs work » Needs review
FileSize
16.5 KB
2.6 KB
69.65 KB

Fixing the test. I guess I didn't run this one after making the change. You can see I'm trying to get the attribute on a string now. I was able to replace this with something even better: elementAttributeContains().

-    $alt = $assert_session->fieldExists('attributes[alt]')->getValue();
+
     // Assert that the placeholder is set to the value of the media field's
     // alt text.
-    $this->assertSame('default alt', $alt->getAttribute('placeholder'));
+    $assert_session->elementAttributeContains('named', ['field', 'attributes[alt]'], 'placeholder', 'default alt');
oknate’s picture

I was missing the latest commits in my working branch. Rerolling.

phenaproxima’s picture

+++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
@@ -26,7 +26,7 @@
+ *   description = @Translation("Embeds media items using a custom tag, &lt;drupal-media&gt;. If used in conjunction with the 'Align/Caption' filters, make sure this filter is configured to run after them. If you are using the HTML filter, be sure the data-align and/or data-caption attributes are allowed on the &lt;drupal-media&gt; tag.  If you would like users to be able to override the alt text on image media, add the alt attribute to the &lt;drupal-media&gt; tag  as well."),

Superdupernit: there is an extra space before "as well" which shouldn't be there.

I don't think I have any other real concerns left. Just need @andrewmacpherson to sign off on the accessibility-related changes.

oknate’s picture

One thought. The following code doesn't do anything media specific. I can think of other use cases for this, such as for the contrib module entity embed. Perhaps this could be made more generic, 'data-text-format-host-entity-langcode', and perhaps it should go in a different module, for example the filter module, that creates the TextFormat render element?

/**
 * Implements hook_field_widget_form_alter().
 */
function media_field_widget_form_alter(&$element, FormStateInterface $form_state, $context) {
  // Add an attribute so that drupalmedia's JavaScript can pass the host
  // entity's language to EditorMediaDialog, allowing it to present entities
  // in the same language.
  if (!empty($element['#type']) && $element['#type'] == 'text_format') {
    if (!empty($context['items']) && $context['items'] instanceof FieldItemListInterface) {
      $element['#attributes']['data-media-embed-host-entity-langcode'] = $context['items']->getLangcode();
    }
    elseif ($entity = $form_state->getFormObject()->getEntity()) {
      $element['#attributes']['data-media-embed-host-entity-langcode'] = $entity->language()->getId();
    }
  }
}

oknate’s picture

Removing the extra space (#89.1).

Wim Leers’s picture

Status: Needs review » Needs work

This is looking very close now!

  1. #75: I understand. I helped arrive us at this least-horrible-solution. I should've phrased it differently. Basically, I think that most of the points I raised would make several scenarios have a better UX. That's all. And I see I failed to mention that the text is wrong: it's not only about overriding attributes, because for example data-align is not an override at all.
  2. #76.1:
    +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -150,13 +148,18 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
    +      if ($this->currentUser()->hasPermission('administer filters')) {
    

    This should use $format->access('update', …)

  3. #76.2: 👍 Right, the only thing that matters is the downcast representation, while the CKEditor Widget is shown, everything is an approximation anyway. If you remove the caption, the data-caption attribute on <drupal-media> also stays around until the downcast. If we're going to make <drupal-media> always be perfectly in sync with the current state, we're doing unnecessary work, because that's exactly what a CKEditor Widget's data is for (the canonical representation/storage of the current state).
  4. #76.3: 👍 Much better!
  5. #76.9: That's a better nuance, let's go with that 😊
  6. #76.10: 👍
  7. #78: Thanks for creating #3075593: Allow MediaEmbed filter to use data-langcode to set media translation. Let's indeed keep that out of the scope of this issue. Responded at #3075593-5: Allow MediaEmbed filter to use data-langcode to set media translation.
  8. #79: I was gonna point out that that is not right, so 👍😀
  9. #90: You're right. But that would be an issue for editor.module then. For now, let's keep it specific to Media Embed. But I think it would be excellent to open a follow-up! 👍

Patch review

  1. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -152,6 +152,13 @@
    +            data.langcode = hostEntityLangcode;
    

    🤔 I think we should rename this from data.langcode to the more verbose data.hostEntityLangcode, to avoid somebody potentially misreading this as data-langcode, which is not supported (see #3075593: Allow MediaEmbed filter to use data-langcode to set media translation for that).

  2. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -160,12 +167,27 @@
    +            // In the CKEditor Widget, we use `hasCaption` to track whether
    +            // the embedded media has a caption or not. The filter on the other
    +            // hand simply uses an empty or non-existing `data-caption`
    +            // attribute. So some conversion work is necessary to allow an
    +            // empty caption with placeholder text.
    

    This is not wrong, but I think this would be clearer:

    // The server-side text filter plugin treats both an empty
    // `data-caption` attribute and a non-existing one the same: it
    // does not render a caption. But in the CKEditor Widget, we 
    // need to be able to show an empty caption with placeholder
    // text using CSS even when technically there is no `data-caption`
    // attribute value yet. That's why this CKEditor Widget has an 
    // independent `hasCaption` boolean (which is not an attribute)
    // to know when to generate a non-empty `data-caption` // attribute when the  content creator has enabled caption: this 
    // makes the server-side text filter render a caption, allowing 
    // the placeholder-rendering CSS to work.
    // @see core/modules/filter/css/filter.caption.css
    // @see ckeditor_ckeditor_css_alter()
    

    (By the way: I should have written a comment from the start, I'm sorry you had to figure out why this was here in the first place!)

  3. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,9 +253,113 @@
    +         * Injects HTML for buttons into the preview that was just loaded.
    ...
    +        _setUpButtons() {
    

    There is only one button now! So perhaps this should be renamed to setUpEditButton()?

  4. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,241 @@
    +        '#markup' => $this->t('There is nothing to override for this media.'),
    ...
    +        $form['no_access_notice']['#markup'] = $this->t('There is nothing to override for this media. <a href="@edit_url">Edit the text format %format</a> to modify the attributes that can be overridden.', ['@edit_url' => $edit_url, '%format' => $format->label()]);
    

    👎 We shouldn't have the same text twice, in two different translatable strings. The second sentence should either be in a separate piece of markup, or it should just be appended.

  5. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -26,7 +26,7 @@
    - *   description = @Translation("Embeds media items using a custom HTML tag. If used in conjunction with the 'Align/Caption' filters, make sure this filter is configured to run after them."),
    + *   description = @Translation("Embeds media items using a custom tag, <code>&lt;drupal-media&gt;

    . If used in conjunction with the 'Align/Caption' filters, make sure this filter is configured to run after them. If you are using the HTML filter, be sure the data-align and/or data-caption attributes are allowed on the &lt;drupal-media&gt; tag. If you would like users to be able to override the alt text on image media, add the alt attribute to the &lt;drupal-media&gt; tag as well."),

    👍

  6. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,323 @@ public function testEditableCaption() {
    +  public function testDialogAccess() {
    ...
    +    // Test the validation of attributes in the dialog.  If the alt,
    +    // data-caption, and data-align attributes are not set on the drupal-media
    +    // tag, the respective fields shouldn't display in the dialog.
    ...
    +    // Now test the same thing with a user who has access to edit text formats.
    +    // An extra message containing a link to edit the text format should
    +    // appear.
    ...
    +    // Now test that adding the attributes to the allowed HTML will allow
    +    // the fields to display in the dialog.
    ...
    +    // Test that setting the media image field to not display alt field also
    +    // disables it in the dialog.
    ...
    +    // Test that enabling the alt field on the media image field restores
    +    // the field in the dialog.
    

    👍 Excellent, this is the high-level dialog test coverage: which form fields appear, and which don't.

  7. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,323 @@ public function testEditableCaption() {
    +    // Clear the cached field configs.
    +    $this->container->get('cache.discovery')->deleteAll();
    

    🤔 This should not be necessary. I wonder which cache is not being updated correctly. Let's at least change this to the more precise Cache::invalidateTags(['entity_field_info']);.

  8. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,323 @@ public function testEditableCaption() {
    +    // Assert that the placeholder is set to the value of the media field's
    +    // alt text.
    +    $assert_session->elementAttributeContains('named', ['field', 'attributes[alt]'], 'placeholder', 'default alt');
    

    👍 I was explicitly looking for this test coverage, and was not disappointed! 👏

    🙏 But what I'm still missing here is an assertion that <drupal-media> does not have an alt attribute set — that is what would conclusively prove that the placeholder attribute for the alt form field in the dialog was in fact loaded directly from the underlying Media entity, and not from an existing override. This is in principle not necessary to check, but it gives us extra protection against future regressions.

  9. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,323 @@ public function testEditableCaption() {
    +    $who_is_zartan = 'Zartan is the leader of the Dreadnoks.';
    

    TIL 😆

  10. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,323 @@ public function testEditableCaption() {
    +    // Assert that the img within the media embed within the ckeditor contains
    ...
    +    // Test the process against with a different alt text to make sure it works
    ...
    +    // within the ckeditor contains the overridden alt text set in the dialog.
    

    🤓 Nits:

    • s/the ckeditor/CKEditor/
    • s/against/again/
  11. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,323 @@ public function testEditableCaption() {
    +    // Test that the default alt attribute displays without an override.
    +    $this->assertNotEmpty($assert_session->waitForElementVisible('xpath', '//img[contains(@alt, "texte alternatif par défaut")]'));
    ...
    +    // Assert that the placeholder is set to the value of the media field's
    +    // alt text.
    +    $assert_session->elementAttributeContains('named', ['field', 'attributes[alt]'], 'placeholder', 'texte alternatif par défaut');
    

    👏👏👏

  12. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +396,323 @@ public function testEditableCaption() {
    +    $assert_session->elementExists('xpath', '//img[contains(@alt, "' . $qui_est_zartan . '")]');
    +
    +  }
    

    🤓 Übernit: extraneous empty line.

  13. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,34 +962,125 @@ public function previewAccessProvider() {
    +      // Assert that the resultant downcast drupal-media tag has the proper
    +      // `data-align` attribute.
    

    🤓🤓 I think it's weird that we have `data-align` yet also drupal-media. It should be either <drupal-media> or `drupal-media`.

  14. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,34 +962,125 @@ public function previewAccessProvider() {
    +   * @see /core/modules/media/src/Form/EditorMediaDialog.php
    

    🤓 Nit: remove the leading slash, and actually, use the FQCN (Fully Qualified ClassName), not the path. We only use paths for non-PHP code. So: \Drupal\media\Form\EditorMediaDialog).

  15. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -754,4 +1194,26 @@ protected function clickPathLinkByTitleAttribute($text) {
    +   * Get the drupal-media element from the ckeditor source.
    

    🤔 This would be better I think: Parses the <drupal-media> element from CKEditor's "source" view.

effulgentsia’s picture

+++ b/core/modules/media/media.module
@@ -496,3 +497,20 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
+    if (!empty($context['items']) && $context['items'] instanceof FieldItemListInterface) {
+      $element['#attributes']['data-media-embed-host-entity-langcode'] = $context['items']->getLangcode();
+    }
+    elseif ($entity = $form_state->getFormObject()->getEntity()) {
+      $element['#attributes']['data-media-embed-host-entity-langcode'] = $entity->language()->getId();
+    }

Is there any code path where that if statement isn't true? Under what scenario would someone enter the elseif statement? If there isn't any, let's remove it.

oknate’s picture

I found this comment in the original issue from me a few months ago:

I left the entity as a fallback. I'm not sure that fallback would ever be used in hook_field_widget_form_alter().

#3018485-39: Selected Entity should display in host entity's language in EntityEmbedDialog when data-langcode not set

I'll look into this and see if I can come up with a situation where the else statement would be reached, otherwise I'll remove it.

effulgentsia’s picture

+++ b/core/modules/media/src/Form/EditorMediaDialog.php
@@ -0,0 +1,241 @@
+      $editor_object = $form_state->getUserInput()['editor_object'];
+      // By convention, the data that the text editor sends to any dialog is in
+      // the 'editor_object' key.
+      $media_embed_element = $editor_object['attributes'];
+      $form_state->set('media_embed_element', $media_embed_element);
+      $has_caption = $editor_object['hasCaption'];
+      $form_state->set('hasCaption', $has_caption);

This is risky, because it's putting unvalidated values into the form state. I don't think we should put all the attributes into the form state: rather, only put the ones that we need to use, and validate them first. E.g., validate the length of 'alt', validate that 'data-align' is one of the allowed options, validate that 'data-entity-uuid' is a valid UUID (see UuidValidator), etc. And also validate that hasCaption is a boolean (or the expected representation of one).

oknate’s picture

Feedback for #92:
1. ✅ Changed the word “override” to “configure”.
2. ✅ Switch to $format->access('update’)
9. 👍 Added a follow-up issue: https://www.drupal.org/project/drupal/issues/3076338

Patch Feedback in #92:
1. ✅ Switched langcode to hostEntityLangcode.
2. ✅ Added your new copy.
3. ✅ Renamed.
4. ✅ Updated the text so the second translatable string includes the first.
7. Cache::invalidateTags(['entity_field_info']); didn’t work. I tried

  • Cache::invalidateTags(['entity_field_info']);
  • Cache::invalidateTags$field->getCacheTags())
  • Cache::invalidateTags($field->getCacheTagsToInvalidate())
  • \Drupal::service('entity_field.manager')->clearCachedFieldDefinitions()
  • $this->container->get(‘entity_field.manager')->clearCachedFieldDefinitions()

None of the above worked as a replacement. I ended up with a slight improvement:

$this->container
  ->get('cache.discovery')
  ->delete('entity_bundle_field_definitions:media:image:en');

Which works.

8. ✅ I added assertions throughout testAlt() that the downcast element has the expected values. I hope I didn’t overdo it.
10, 12, 13 ✅
14. ✅ Updated to @see \Drupal\media\Form\EditorMediaDialog
15. ✅ Thanks, copied verbatim.

Addressing #93. ✅ This is called from WidgetBase::formSingleElement, which requires a FieldItemListInterface $items.
But, since $context is passed by reference, it’s possible, although highly unlikely, that another module might unset $context[‘items’]. And in that case the fallback would fire. This would be very unorthodox and dangerous of a developer, and given it’s highly unlikely, I think we can remove the else statement (and I'm doing so in this patch). But I think we should keep the code validating $context[‘items’], in the unlikely event it’s unset.

For #95, very good point. We're trying to allow this to extendable, so I'm not sure we should only allow the values we're explicitly using. I guess if someone extends the form, they can use the user input for other attributes, so we can limit it in this form.

Wim Leers’s picture

  1. #93++, great catch!
  2. #95: We never trust the data to the degree that we save it anywhere. The only reason we put it in form state at all is because of form rebuilds that would otherwise lose the input data. We only ever pass it back to the client. This is following the pattern established by \Drupal\editor\Form\EditorImageDialog::buildForm(). Why do we need validation here and not in EditorImageDialog?
  3. #96: 👍

Interdiff review

  1. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -151,12 +151,16 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
    +          '%warning' => $warning,
    ...
    +        $form['no_access_notice']['#markup'] = $this->t('%warning <a href="@edit_url">Edit the text format %format</a> to modify the attributes that can be overridden.', $tparams);
    

    👎 Sorry, you did this is because I gave a bad example in Slack. But %warning needs to be !warning. Because otherwise it gets wrapped in <em>.

  2. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -477,11 +477,12 @@ public function testDialogAccess() {
    -    // Clear the cached field configs.
    -    $this->container->get('cache.discovery')->deleteAll();
    ...
    +    $this->container
    +      ->get('cache.discovery')
    +      ->delete('entity_bundle_field_definitions:media:image:en');
    
    @@ -495,8 +496,9 @@ public function testDialogAccess() {
    -    // Clear the cached field configs.
    -    $this->container->get('cache.discovery')->deleteAll();
    +    $this->container
    +      ->get('cache.discovery')
    +      ->delete('entity_bundle_field_definitions:media:image:en');
    

    #96.7: 😔 So weird!

    This is better than what we had before. But let's add a @todo to remove this, because this really should not be necessary! But it doesn't need to block this patch, so a todo is fine.

  3. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -477,11 +477,12 @@ public function testDialogAccess() {
    +    $field = FieldConfig::loadByName('media', 'image', 'field_media_image')
    +      ->setSetting('alt_field', FALSE);
    +    $field->save();
    

    Nit: there's no need for the $field variable, this can be entirely chained.

  4. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -550,6 +552,14 @@ public function testAlt() {
    +    // Test that by default no alt attribute is present on the drupal-media
    +    // element.
    +    $this->assertNotEmpty($drupal_media = $this->getDrupalMediaFromSource());
    +    $this->assertFalse($drupal_media->hasAttribute('alt'));
    +    // Press the source button again to leave source mode.
    +    $this->pressEditorButton('source');
    +    // Having entered source mode means we need to reassign an id to the
    +    // CKEditor iframe.
    

    👍 This is the test coverage I was looking for in #92.8.

  5. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -572,6 +582,20 @@ public function testAlt() {
    +    // Test that the downcast drupal-media element now has the alt attribute
    +    // entered in the dialog.
    +    $this->assertNotEmpty($drupal_media = $this->getDrupalMediaFromSource());
    +    $this->assertSame($who_is_zartan, $drupal_media->getAttribute('alt'));
    

    This is also a good addition! 👍

  6. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -590,9 +614,24 @@ public function testAlt() {
    +    // Test that the downcast drupal-media element now has the alt attribute
    +    // entered in the dialog.
    +    $this->assertNotEmpty($drupal_media = $this->getDrupalMediaFromSource());
    +    $this->assertSame($cobra_commander_bio, $drupal_media->getAttribute('alt'));
    
    @@ -618,11 +657,32 @@ public function testAlt() {
    +    // Test that the downcast drupal-media element's alt attribute now has the
    +    // empty string indicator.
    +    $this->assertNotEmpty($drupal_media = $this->getDrupalMediaFromSource());
    +    $this->assertTrue($drupal_media->getAttribute('alt'));
    ...
    +    // Test that the downcast drupal-media element no longer has an alt
    +    // attribute.
    +    $this->assertNotEmpty($drupal_media = $this->getDrupalMediaFromSource());
    +    $this->assertFalse($drupal_media->getAttribute('alt'));
    

    Maybe this is a bit much. But what would help tremendously is if you'd add a helper method for this. That'd make the test coverage much easier to understand, and less repetitive. Less brittle, too!

oknate’s picture

Status: Needs work » Needs review
FileSize
15.06 KB
56.29 KB

Responding to feedback in #97.
1. I couldn't get it to work with "!", but changed it to "@", which removes the <em>.
2. Added a follow-up about the cache clearing issue. #3076544: After saving `FieldConfig` entities, the old configuration is still being used: discovery bin needs to be manually cleared
3. ✅Oops, that was left over from testing the cache clearing. Undid that change.
4, 5, 6. ✅Added a helper function or two to make this more readable.

oknate’s picture

On slack, phenaproxima wrote:

I would suggest we split them up a little more. Right now, some of these are agglomerated in odd ways. For example, leaveSourceModeAndOpenMetadataDialog() should probably be two methods: leaveSourceMode() and openMetadataDialog(). I think that will, ultimately, make things clearer to read and work with in the future.

This patch does that.

oknate’s picture

Adding an interdiff for #98 to #99.

phenaproxima’s picture

+++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -1115,6 +1066,51 @@ protected function fillFieldInMetadataDialog($locator, $value) {
+    // If not within the CKEditor iframe, switch to it.
+    if ($page->has('css', '#ckeditor')) {
+      $this->getSession()->switchToIFrame('ckeditor');
+    }

This seems a bit "magic" to me. I would suggest removing this and just mentioning in the doc comment for openMetadataDialog() that we expect calling code to have switched into the CKEditor iframe before calling it.

oknate’s picture

Status: Needs review » Needs work

The last submitted patch, 102: 2994702-102.patch, failed testing. View results

oknate’s picture

Status: Needs work » Needs review
FileSize
56.47 KB
805 bytes

Ugh. I thought I run that test locally. Here's a fix.

I created a separate issue for the sqlite failure in the other test: #3076609: \Drupal\Tests\ckeditor\FunctionalJavascript\CKEditorIntegrationTest fails on Sqlite.

Wim Leers’s picture

Assigned: Unassigned » andrewmacpherson

This is RTBC as far as I'm concerned. In #97 I mostly asked for test coverage improvements, and those are all present in the latest patch.

But there's still the needs accessibility review tag. Is that tag still accurate? AFAICT it is. Assigning to @andrewmacpherson.


One remark for the future core committer who frowns at the changes made to ::testAlignmentClasses():

+++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -611,34 +979,161 @@ public function previewAccessProvider() {
+      // Now verify the result.
+      $selector = sprintf('drupal-media[data-align="%s"] .caption-drupal-media.align-%s', $alignment, $alignment);
+      $this->assertNotEmpty($assert_session->waitForElementVisible('css', $selector, 2000));
...
-      $wrapper = $assert_session->waitForElementVisible('css', '.cke_widget_drupalmedia', 2000);
-      $this->assertNotEmpty($wrapper);
-      $this->assertTrue($wrapper->hasClass('align-' . $alignment));

This was a little hard to follow; looks like @oknate refactored this to not set the body field value directly and instead is now using the dialog to choose different alignments. The quoted lines above test exactly the same, just with more precision.

This means the existing test coverage is also strengthened, because it now is testing exactly what the end user would do, rather than pretending the end user would be typing HTML manually. (Which is what they would have had to do until before this issue.)

oknate’s picture

I looked through the accessibility issues raised and here's a little summary:

Accessibility issues addressed:

Accessibility follow-ups files:

rainbreaw’s picture

Reviewed this on a screen share call with @oknate and @phenaproxima, and here is what we uncovered that are ongoing accessibility concerns:

  1. When in the dialog itself, if you rely on the tab key to navigate from field to field and fill out the form, your tab focus is captured by something mysterious between the caption checkbox and the submit button. Tabbing again will move focus to submit, so this is not a blocker as much as a possible point of confusion. It also appears to behave the same way on standard image embed, as well, and so may belong in a separate issue.
  2. Also when in the dialog itself and using the tab key to navigate between fields, there is a point of confusion when you tab from the alt field to the align radio buttons. The focus defaults to the selected item, and it is all usable, but there is no visual distinction between "focused" and "selected" so you have no visual cue telling you where your focus is currently located. This appears to be inherited from the theme, and so likely belongs in a different issue (and potentially even a different project).
  3. When the "edit media" widget button is focused, it will not currently accept the standard expected methods of activating a button using the keyboard (spacebar and return). This is because the javascript event is limited to an onclick, and needs to also include keyup/keydown behavior so that the spacebar and return will also activate the options. **This is part of this issue and is critical, as the tool cannot be used by keyboard-reliant users without it.**
  4. Since we found the above when playing around with the widget, it is clear that test coverage also needs to be included for tabbing to ensure that actions can be accessed through tab-only.
  5. I would like to see the related issue, #3076773: Edit Media button within WYSIWYG should include media label for screen readers, escalated to stable blocker, and will make a note on that ticket separate from this one regarding why.
bwebster719’s picture

In testing the patch for this issue, I think I found a way where the pencil button doesn't show up on an image while editing an article. What I did was insert an image into the article, click the pencil button, and select caption. I then saved the article without actually entering a caption. Then, when I edit the article again, there is no pencil button. When I look at the source, I see it has data-caption="". If I put something in here for the caption, the pencil button appears again when I go back to visual mode.

Another use case would be where I insert an image with a caption, then later, edit the page and remove the caption, but not go in and uncheck the caption button.

My experience is that when caption is selected, but data-caption="", ie no caption is given, then the pencil button doesn't show for me.

oknate’s picture

Status: Needs review » Needs work
FileSize
1.68 KB
57.06 KB

Working on #107.3:
A space bar and or tab keypress will launch the dialog when focused on the "Edit media" button after inserting media with DrupalMediaLibrary button, but, if you hit the source button twice to reinitialize the editor and start pressing tab, when it gets to the "Edit media" button and you hit enter, it will enter a new paragraph where the cursor is (before the widget). I'm working on adjusting the cursor position when tabbing to the "Edit media" button so this doesn't happen.

There's another issue where when focused on the widget, you can't seem to get out of it to tab around. I would think if you're focused on the widget and you hit tab, it would go to the "Edit media" button.

This is a work in progress.

I haven't had a chance yet to look at the other feedback yet. But thank you so much for the feedback, @rainbreaw and @bwebster719.

oknate’s picture

Adding test coverage for the keyup event added in #109.

Also, I was able to easily reproduce the bug in #108. That was a good catch! I'll add a fail test for that.

oknate’s picture

Addressing #108. Fail patch shows the bug. It's the same as the other patch, minus this:

diff --git a/core/modules/media/js/plugins/drupalmedia/plugin.es6.js b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
index bd410c6bee..6dde5740e4 100644
--- a/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
+++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
@@ -142,6 +142,12 @@
           }
           data.attributes = CKEDITOR.tools.copy(attributes);
           data.hasCaption = data.attributes.hasOwnProperty('data-caption');
+          // Add space to the empty caption to allow the server-side text
+          // filter to render a caption, allowing the placeholder-rendering
+          // CSS to work.
+          if (data.hasCaption && data.attributes['data-caption'] === '') {
+            data.attributes['data-caption'] = ' ';
+          }
           data.link = null;
           if (element.parent.name === 'a') {
             data.link = CKEDITOR.tools.copy(element.parent.attributes);
diff --git a/core/modules/media/js/plugins/drupalmedia/plugin.js b/core/modules/media/js/plugins/drupalmedia/plugin.js
index a145b649c7..a7af48552d 100644
--- a/core/modules/media/js/plugins/drupalmedia/plugin.js
+++ b/core/modules/media/js/plugins/drupalmedia/plugin.js
@@ -110,6 +110,9 @@
           }
           data.attributes = CKEDITOR.tools.copy(attributes);
           data.hasCaption = data.attributes.hasOwnProperty('data-caption');
+          if (data.hasCaption && data.attributes['data-caption'] === '') {
+            data.attributes['data-caption'] = ' ';
+          }
           data.link = null;
           if (element.parent.name === 'a') {
             data.link = CKEDITOR.tools.copy(element.parent.attributes);

There's very similar code elsewhere in plugin.js:

          // Only run during changes.
          if (this.oldData) {
            // The server-side text filter plugin treats both an empty
            // `data-caption` attribute and a non-existing one the same: it
            // does not render a caption. But in the CKEditor Widget, we
            // need to be able to show an empty caption with placeholder
            // text using CSS even when technically there is no `data-caption`
            // attribute value yet. That's why this CKEditor Widget has an
            // independent `hasCaption` boolean (which is not an attribute)
            // to know when to generate a non-empty `data-caption`
            // attribute when the content creator has enabled caption: this
            // makes the server-side text filter render a caption, allowing
            // the placeholder-rendering CSS to work.
            // @see core/modules/filter/css/filter.caption.css
            // @see ckeditor_ckeditor_css_alter()
            if (!this.data.hasCaption && this.oldData.hasCaption) {
              delete this.data.attributes['data-caption'];
            } else if (this.data.hasCaption && !this.oldData.hasCaption) {
              this.data.attributes['data-caption'] = ' ';
            }
          }

But this only works when there is this.oldData. It doesn't handle the case where someone manually adds data-caption="" in source mode.

Wim Leers’s picture

#107 Thanks so much for your accessibility review! 🙏🥳

  1. This is a pre-existing problem with jQuery UI dialogs — the same is true for EditorImageDialog.
  2. Nice find! But this too is a pre-existing problem for sure 😔
  3. This is exactly what I feared — thanks for confirming. 👍
  4. 👍
  5. Left a comment at #3076773-3: Edit Media button within WYSIWYG should include media label for screen readers.

#108: 🙏👏 NICE FIND! It results in data.hasCaption=true and data.attributes['data-caption'] = '' in the CKEditor Widget, which results in a JS error. Thanks so much, @bwebster719! This will need an explicit test too. I see #111 fixed this! 🥳

#109: 🤔 WRT constraining tabbing: we should talk to the CKEditor team about that probably. Related: https://www.drupal.org/node/2014545 — but we can't use that in CKEditor.

#110: 🤔 I've never heard of triggering something using the space bar? That should scroll in a browser.

phenaproxima’s picture

Crediting @bwebster719 and @rainbreaw for reviews and finding bugs.

phenaproxima’s picture

Assigned: andrewmacpherson » Unassigned

Oh, and @andrewmacpherson.

oknate’s picture

I'm working on the tabbing issues. I have found that adding some of the editable classes to the button give us what we want to achieve with the tabbing:

var editButton = CKEDITOR.dom.element.createFromHtml('<button class="media-library-item__edit" contenteditable="true" class="cke_widget_editable" data-cke-display-name="' + Drupal.t('Edit media') + '">' + Drupal.t('Edit media') + '</button>');

You can tab into it and it and it shows in the path navigator and the cursor moves there. But there are two problems:
1) It's kind of a hack, so there's probably a better way
2) I haven't been able to turn off the editing of the button contents when using this hack.

I've been looking at the CKEditor documentation, but it's vast and I haven't found what I'm looking for. We want something that has a lot of the properties of an editable, but doesn't allow editing.

Wim Leers’s picture

RE: myself in #112: I've never heard of triggering something using the space bar? That should scroll in a browser. — turns out that space bar doesn't trigger links, but it does trigger buttons. TIL! Browsers are weird 🤷‍♂️

#115: What about just using tabindex="0"?

oknate’s picture

tabindex="0" doesn't fix the issue where when tabbing to it from the caption hitting return will put a new line in the caption, or if tabbing from the preceeding paragraph it will add a new paragraph before the widget.

oknate’s picture

@wim-leers came up with this solution.

This fixes the tabbing issue in #109 and #115:

if you hit the source button twice to reinitialize the editor and start pressing tab, when it gets to the "Edit media" button and you hit enter, it will enter a new paragraph where the cursor is (before the widget). I'm working on adjusting the cursor position when tabbing to the "Edit media" button so this doesn't happen.

@todo It needs test coverage that the tabbing works.
@todo Still some issues in #107 to look into.

oknate’s picture

I forgot to change this in the last patch:

-    $button->keyUp($char);
+    $button->keyDown($char);
oknate’s picture

Status: Needs work » Needs review
FileSize
7.91 KB
534 bytes
60.33 KB

Responding to #107:
1. ✅ I was able to remove the hidden element from tabbing by adding tabindex=“-1”. Since we found this bug in the image dialog too, we should open a follow-up to fix it there as well. I didn't add it to this patch, as it's out of scope.
2. This doesn’t happen in demo_umami profile, only standard, or minimal profile, among those i tested. I added a follow-up for this, as it’s within a few themes, and out of scope. #3077559: :focus indicator not visually distinct in ui dialog in Bartik, Seven, Classy I think I provide a decent solution there.
3. ✅ Using a key to open the dialog was fixed in #109, both space bar and return key work.
4. I haven’t been able to successfully write test coverage for the tabbing yet. There are no examples in core for me to follow, so I’m having to figure it out, and my attempts are so far fruitless. There is apparently driver support for this, in fact, we’re using key presses to test the space bar and return key opening the dialog, but I haven’t been able to get it to work with the tab key yet. So opening a follow-up for this: #3077562: Add test coverage for tabbing to "Edit media" button..
5. There’s already a follow-up for changing the "Edit Media" text to include the label of the media. This is not as straight forward as it sounds due to the way the previews are loaded through processed text or else it would already be done.

How to manually test new accessibility updates:

1) Use DrupalMediaLibrary CKEditor button to insert a media embed.
2) Instead of using the mouse and clicking, use tabbing and the enter key (or space bar) to open the "Edit media" dialog.

3) Try again by starting the cursor in a paragraph after the widget.
4) Use shift tab to go back to the Edit button and use the enter key (or space bar) to open the "Edit media" dialog.

Wim Leers’s picture

#120.2: I think Seven matters more than Bartik. Content editing usually happens in Seven.

oknate’s picture

Yeah, it could be a quirk of the way I was testing. I'll check to make sure the seven theme was the admin theme and update that ticket if not.

oknate’s picture

👍Updated #3077559: :focus indicator not visually distinct in ui dialog in Bartik, Seven, Classy, as this is focus indicator issue happens in the seven theme as well, per Wim's alerting me about that.

Wim Leers’s picture

Issue tags: -Needs tests

This issue is blocked on accessibility review. Test coverage is abundantly present.

phenaproxima’s picture

Status: Needs review » Needs work

We can address @webchick's stable-blocking feedback from #2801307-94: [META] Support WYSIWYG embedding of media entities, point 2, in this issue. Quoting for simplicity:

The description of the Media embed filter is super long and has more twists and turns than a murder mystery. Suggestion was to configure it with "smart defaults" (align, caption, alt, and title all accessible by default, to match what image does), and strike the second part of the sentence.

We are adding those additional sentences in this patch. So let us revert that change (and, if needed, the relevant test coverage).

In Slack, I asked Wim how to make it so that the HTML filter could be made to allow <drupal-media alt data-align data-caption> by default. He replied:

When dragging buttons into the “active” CKEditor toolbar, i.e. to make them show up for content creators, all required tags and attributes are automatically added to the list of allowed HTML. So to make that the default, you’d need to change

        allowedContent: 'drupal-media[!data-entity-type,!data-entity-uuid,data-align,data-caption,alt]',
        requiredContent: 'drupal-media[data-entity-type,data-entity-uuid]',

to

        allowedContent: 'drupal-media[!data-entity-type,!data-entity-uuid,!data-align,!data-caption,!alt]',
        requiredContent: 'drupal-media[data-entity-type,data-entity-uuid,data-caption,data-align,alt]',

While not technically necessary when the alignment/caption filters are not enabled, allowing attributes that simply are not being used seems perfectly acceptable.

He also said: "(Side effect & benefit: this would make it even less likely to trigger EditorMediaDialog with nothing being in there!)"

I agree. So kicking back to "needs work" for these changes, plus relevant testing.

effulgentsia’s picture

allowing attributes that simply are not being used seems perfectly acceptable

I disagree with that. It is incorrect to say that alt, data-align, or data-caption are required for the Media widget to function. I recommend not introducing that incorrectness into this patch, and instead to have a follow-up for resolving #2801307-94: [META] Support WYSIWYG embedding of media entities.2.

phenaproxima’s picture

It is incorrect to say that alt, data-align, or data-caption are required for the Media widget to function.

That's true, they're not required, but the goal is for them to be allowed by filter_html. Based on my relatively shallow research into how CKEditor plugins integrate with filter_html, I think we might be able to have it both ways -- the attributes can be added to filter_html's acceptlist by default, yet not be required in order for the CKEditor widget to function. IMHO, if we can make that happen here without a lot of extra fuss, we should.

webchick’s picture

Re: #126/127 Just a note that we (@phenaproxima, @effulgentsia, @oknate, and myself) had a discussion which confirms that these settings being in there would be default options only; not required options. And removing them from the HTML filter would still allow the Media widget to function. So I'm think we're good here.

oknate’s picture

Posting a FAIL patch first before working on the auto-population of the additional attributes in the Allowed HTML settings for FilterHtml.

  • Addresses #125 and drops the second sentence in the MediaEmbed filter.
  • Adds test coverage for automatically adding these additional attributes when the DrupalMediaLibrary button is dragged into the toolbar in the configuration form: data-align, data-caption, alt and title. (Although we're not supporting setting title in the dialog, the DrupalMedia plugin still supports it).

This patch includes the fix for a regression I found while writing the test coverage: #3078161: DrupalMediaLibrary plugin breaks things when adding a new text editor

rainbreaw’s picture

Performed an accessibility review on this tonight, and it worked beautifully:

1. Was able to tab into the embedded image once to hear that it was an embedded image and hear the alt text
2. Was able to tab a second time to get to the Edit Media button (this will be even better when #3076773: Edit Media button within WYSIWYG should include media label for screen readers is included)
3. Space bar and return/enter both activated the edit modal
4. The modal captures keyboard focus and announces itself
5. I can easily close the modal without making changes
6. Default text in the alt text field comes from the original embed alt text, and is read, and can be overridden
7. When I tab into the radios for alignment options, I can navigate through with keyboard only, and my screen reader tells me exactly what I'm on. I can also activate my desired choice with the space bar, as I should be able to
8. When I tab next, I get to the caption option, which is read to me properly, and I can activate it with my spacebar
9. When I tab again, I go immediately to the save button

There is no confusion when using a screen reader.

The only confusion when using keyboard without a screen reader is the separate focus issue, which is documented here: #3077559: :focus indicator not visually distinct in ui dialog in Bartik, Seven, Classy

oknate’s picture

Status: Needs work » Needs review
Issue tags: -Needs accessibility review
FileSize
65.76 KB
2.74 KB
7.86 KB

Thank you, @rainbreaw, for your accessibility feedback. Removing "needs accessibility review" tag based on #130:

it worked beautifully [...]

Also, implements the fix suggested by Wim and documented in #125. Now when the code in core/modules/filter/filter.filter_html.admin.js automatically adds the drupal-media tag to the allowed HTML settings of FilterHtml, it will include the extra attributes, but it doesn't require them for the MediaEmbed filter to function properly, nor for the DrupalMediaLibrary button to function properly. So this allows us to drop the second sentence telling site builders about all the wonderful attributes they might be missing.

oknate’s picture

Adding test coverage that you can use the UI to remove the extra attributes ('data-align, data-caption, alt and title') from the tag without a validation error in the Text Format edit form. Since these are automatically added, I wanted to make sure they don't get automatically added back upon save. Since they are added in javascript rather than on form save, I was pretty sure there would be no issue, but I wanted to test and add coverage for this.

There's already test coverage for the validation when you remove required attributes in MediaEmbedFilterConfigurationUiTest.php. (It won't let you save the form without those attributes).

oknate’s picture

This patch converts the allowedContent and requiredContent on the two plugins from string format to object format/CKEDitor style format.

The object format is recommended over the string format as it's generally faster:

objects (fastest) or arrays (less recommended — may be slower

See
https://ckeditor.com/docs/ckeditor4/latest/guide/dev_allowed_content_rul...

phenaproxima’s picture

I reviewed the interdiffs and I think they look great overall. I'm not even setting this back to "needs work", since the changes below are entirely optional nitpicks. I want to do some quick manual testing and a full re-read of the patch before RTBC, but I think we're just about done, to be honest. 🎉

  1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -169,7 +169,24 @@ public function testConfigurationValidation() {
    +    // Test that config form allows removing non-required attributes from
    +    // the <drupal-media> tag.
    

    I confirmed that we have test coverage of what happens if one tries to remove the required attributes. (It's in \Drupal\Tests\media\FunctionalJavascript\MediaEmbedFilterConfigurationUiTest.) So 👍

  2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -169,7 +169,24 @@ public function testConfigurationValidation() {
    +    $this->assertFalse($allowed_html_field->isVisible());
    

    Do we need this assertion? Not sure what it's adding, really...

  3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -169,7 +169,24 @@ public function testConfigurationValidation() {
    +    $this->assertTrue($assert_session->waitForElementVisible('named', ['field', 'filters[filter_html][settings][allowed_html]']));
    

    This might be a little cleaner as:

    $this->assertTrue($allowed_html_field->waitFor(10, function ($field) {
      return $field->isVisible();
    }));
    

    That way, we don't need to copy and paste the selector.

  4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -169,7 +169,24 @@ public function testConfigurationValidation() {
    +    $format = FilterFormat::load('sulaco');
    +    $allowed_html = $format->filters('filter_html')->settings['allowed_html'];
    +    $this->assertContains($replace, $allowed_html);
    +    $this->assertNotContains($search, $allowed_html);
    

    This is a bit underhanded :) Since this is a functional test, let's assert that the field has the expected value, rather than loading the format and checking it at the API level.

+++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
@@ -115,10 +115,31 @@
+        allowedContent: {
+          'drupal-media': {
+            attributes: {
+              '!data-entity-type': true,
+              '!data-entity-uuid': true,
+              'data-align': true,
+              'data-caption': true,
+              alt: true,
+              title: true,
+            },
+            classes: {},
+          },
+        },

Not only is this faster, this syntax is more consistent with what's going on in the drupalimage plugin. 👍

phenaproxima’s picture

Status: Needs review » Needs work
Related issues: +#3078161: DrupalMediaLibrary plugin breaks things when adding a new text editor

I found many things in a full read-through of the patch. Virtually all of it is minor and will take seconds to fix. After that, this code is RTBC from me. (We'll want to do some quick manual testing, but that's not hard.)

I also think we should block this on #3078161: DrupalMediaLibrary plugin breaks things when adding a new text editor. That should be a very easy issue to land; it already has tests and is looking excellent except for minor nits.

  1. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -179,6 +236,13 @@
    +            const classes = this.element.getParent().$.classList;
    +            for (let i = 0; i < classes.length; i++) {
    +              if (classes[i].indexOf('align-') === 0) {
    +                this.element.getParent().removeClass(classes[i]);
    +              }
    +            }
    

    I think this could use a comment.

  2. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,9 +288,126 @@
    +            // Some browsers will add a <br> tag to a newly created DOM
    +            // element with no content. Remove this <br> if it is the only
    +            // thing in the caption. Our placeholder support requires the
    +            // element be entirely empty. See filter-caption.css.
    +            // @see core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.es6.js
    

    Can we change "See filter-caption.css" to just a "@see path/to/filter-caption.css"?

  3. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,9 +288,126 @@
    +        /**
    +         * Injects HTML for buttons into the preview that was just loaded.
    +         */
    +        _setUpEditButton() {
    

    The name of the function is _setUpEditButton (singular), but the comment says "buttons" (plural).

  4. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,9 +288,126 @@
    +          const embeddedMediaContainer = this.data.hasCaption
    +            ? this.element.findOne('figure')
    +            : this.element;
    

    I was thinking about whether we might want to use a different selector here (like a CSS class), but after looking at FilterCaption, I think this is the most reliable option. 👍

  5. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,9 +288,126 @@
    +          const editButton = CKEDITOR.dom.element.createFromHtml(
    +            `<button class="media-library-item__edit">${Drupal.t(
    +              'Edit media',
    +            )}</button>`,
    +          );
    +          embeddedMedia.getFirst().insertBeforeMe(editButton);
    

    Should this be a JavaScript theme function? (Not blocking feedback; just thought I'd call it out.)

  6. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,9 +288,126 @@
    +                  CKEDITOR.tools.extend(
    +                    values.attributes,
    +                    widget.data.attributes,
    +                  );
    

    Can this get a short comment explaining what it's doing and why?

  7. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,9 +288,126 @@
    +          this.element
    +            .findOne('.media-library-item__edit')
    +            .on('keydown', event => {
    +              if (typeof event.data != 'undefined') {
    +                const keypress = event.data.getKey();
    +                if (keypress === 13 || keypress === 32) {
    +                  event.sender.$.click();
    +                }
    +                event.data.$.stopPropagation();
    +                event.data.$.stopImmediatePropagation();
    +              }
    +            });
    

    This could use a comment as well.

  8. +++ b/core/modules/media/media.module
    @@ -496,3 +497,17 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
    +    if (!empty($context['items']) && $context['items'] instanceof FieldItemListInterface) {
    

    I think we can remove this check. According to the documentation for hook_field_widget_form_alter(), $context['items'] will always be there (or, at least, there's no indication that $context['items'] can be null), and it will always be an instance of FieldItemListInterface. So I don't know if we are really adding anything by checking this.

  9. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +/**
    + * Provides a media embed dialog for text editors.
    + *
    + * @internal
    + *   This is an internal part of the media system in Drupal core and may be
    + *   subject to change in minor releases. This class should not be
    + *   instantiated or extended by external code.
    + */
    +class EditorMediaDialog extends FormBase {
    

    Let's expand the doc comment a bit to explain that, depending on filter configuration, this dialog allows one to set the alt text, alignment, and captioning status for an embedded media item. So, something like this:

    Provides a media embed dialog for text editors.
    
    Depending on the configuration of the filters associated with the text editor, this dialog allows users to set the alt text, alignment, and captioning status for embedded media items.
    
  10. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +      // By convention, the data that the text editor sends to any dialog is in
    +      // the 'editor_object' key.
    

    "By convention"? That seems inaccurate. This is not a convention; there is code which is packaging the data into the editor_object key, and it's in core/modules/ckeditor/js/ckeditor.es6.js, right there in the Drupal.ckeditor.openDialog() method. Adding a @see to that might be helpful.

  11. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +      $form_state->set('hasCaption', $has_caption);
    +      $form_state->setCached(TRUE);
    

    These calls can be chained.

  12. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +    if (!empty($editor_object['hostEntityLangcode']) && $media->hasTranslation($editor_object['hostEntityLangcode'])) {
    +      $media = $media->getTranslation($editor_object['hostEntityLangcode']);
    +    }
    

    I think this could use a comment. Also, we can do this inside the if statement that follows it (if ($image_field = ...)), since we don't need the translation for anything except getting the default alt text.

  13. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +        $edit_url = $format->toUrl('edit-form')->toString();
    +        $tparams = [
    +          '@warning' => $warning,
    +          '@edit_url' => $edit_url,
    +          '%format' => $format->label(),
    +        ];
    

    Nit: $edit_url does not need to be its own variable.

  14. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +      '#attributes' => [
    +        'tabindex' => -1,
    +      ],
    

    This needs a comment.

  15. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +  /**
    +   * Get image field from source config.
    

    This is not accurate. How about "Gets the name of an image media item's source field", instead?

  16. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +   * @param \Drupal\media\MediaInterface $media
    +   *   Embedded media.
    

    How about "The media item being embedded" instead?

  17. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +   * @return string|null
    +   *   "The name of the image source field configured for the media item, or
    +   *   NULL if the source field is not an image field.
    

    The return description starts with a quotation mark, but it shouldn't :)

  18. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,248 @@
    +  protected function getMediaImageSourceField(MediaInterface $media) {
    

    Let's rename this to more accurately reflect what it does. How about getMediaImageSourceFieldName()?

  19. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -26,7 +26,7 @@
    - *   description = @Translation("Embeds media items using a custom HTML tag. If used in conjunction with the 'Align/Caption' filters, make sure this filter is configured to run after them."),
    + *   description = @Translation("Embeds media items using a custom tag, &lt;drupal-media&gt;. If used in conjunction with the 'Align/Caption' filters, make sure this filter is configured to run after them."),
    

    👍

  20. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -414,6 +414,13 @@ protected function applyPerEmbedMediaOverrides(\DOMElement $node, MediaInterface
    +        // Allow the display of the image without an alt tag in special cases.
    +        // Since setting the value in the EditorMediaDialog to an empty string
    +        // restores the default value, this allows special cases where the
    +        // alt should not be set to the default value but should be empty.
    

    I'd like a minor rephrasing at the end: "...this allows special cases where the alt text should not be set to the default value, but should be explicitly empty instead."

  21. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
    @@ -414,6 +414,13 @@ protected function applyPerEmbedMediaOverrides(\DOMElement $node, MediaInterface
    +          $node->setAttribute('alt', NULL);
    

    Is there any reason not to use removeAttribute() here?

  22. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -44,6 +49,20 @@ class CKEditorIntegrationTest extends WebDriverTestBase {
    +  /**
    +   * The javascript char code for the return key.
    +   *
    +   * @var int
    +   */
    +  const RETURN_KEY = 13;
    +
    +  /**
    +   * The javascript char code for the space bar.
    +   *
    +   * @var int
    +   */
    +  const SPACE_BAR = 32;
    

    🤓 Well actually...as I understand it, these are ASCII character codes that aren't JavaScript-specific. So let's just say "The character code" instead of "The JavaScript char code". 🤓 (Citation: http://www.asciitable.com)

  23. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -244,13 +263,58 @@ public function testPreviewUsesDefaultThemeAndIsClientCacheable() {
    +    $this->assertNotEmpty($button = $assert_session->waitForButton('Edit media'));
    

    I'm not seeing where $button is subsequently used...?

  24. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -244,13 +263,58 @@ public function testPreviewUsesDefaultThemeAndIsClientCacheable() {
    +    // Assert that figcaption element exists within the drupal-media element.
    +    $this->assertNotEmpty($figcaption = $assert_session->waitForElement('css', 'figcaption'));
    

    Wouldn't we want to wait for drupal-media figcaption instead, as per the comment?

  25. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -244,13 +263,58 @@ public function testPreviewUsesDefaultThemeAndIsClientCacheable() {
    +    $this->assertNotEmpty($drupal_media = $assert_session->waitForElementVisible('css', 'drupal-media', 2000));
    

    Is there any real reason to pass a timeout to waitForElementVisible()? What was wrong with the default timeout of 10 seconds?

  26. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -244,13 +263,58 @@ public function testPreviewUsesDefaultThemeAndIsClientCacheable() {
    +    // Type in the widget's CKEDITOR.editable element for the caption.
    

    I don't think we need to mention that this is a CKEDITOR.element. We can just say "Type into the widget's caption element."

  27. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +426,340 @@ public function testEditableCaption() {
    +    $this->grantPermissions(Role::load(RoleInterface::AUTHENTICATED_ID), ['administer filters']);
    

    There's actually no need to call $this->grantPermissions(). You can accomplish exactly the same thing with the slightly-easier-to-read Role::load(RoleInterface::AUTHENTICATED_ID)->grantPermissions(['administer filters'])->save().

  28. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +426,340 @@ public function testEditableCaption() {
    +    $assert_session->linkExists('text format');
    

    We should be asserting the full text of the link: "Edit the text format Test format".

  29. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +426,340 @@ public function testEditableCaption() {
    +    // Test that setting the media image field to not display alt field also
    +    // disables it in the dialog.
    +    FieldConfig::loadByName('media', 'image', 'field_media_image')
    +      ->setSetting('alt_field', FALSE)
    +      ->save();
    +    // @todo This manual cache clearing should not be necessary, fix in
    +    // https://www.drupal.org/project/drupal/issues/3076544
    +    $this->container
    +      ->get('cache.discovery')
    +      ->delete('entity_bundle_field_definitions:media:image:en');
    +    // Wait for preview.
    +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', 'drupal-media', 2000));
    +    $page->pressButton('Edit media');
    +    $this->waitForMetadataDialog();
    +    $assert_session->fieldNotExists('attributes[alt]');
    +    $page->pressButton('Close');
    +    $this->getSession()->switchToIFrame('ckeditor');
    

    Let's also assert that the alignment and caption fields are still visible.

  30. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +426,340 @@ public function testEditableCaption() {
    +    // Test that enabling the alt field on the media image field restores
    +    // the field in the dialog.
    +    FieldConfig::loadByName('media', 'image', 'field_media_image')
    +      ->setSetting('alt_field', TRUE)
    +      ->save();
    +    // @todo This manual cache clearing should not be necessary, fix in
    +    // https://www.drupal.org/project/drupal/issues/3076544
    +    $this->container
    +      ->get('cache.discovery')
    +      ->delete('entity_bundle_field_definitions:media:image:en');
    +    // Wait for preview.
    +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', 'drupal-media', 2000));
    +    $page->pressButton('Edit media');
    +    $this->waitForMetadataDialog();
    +    $assert_session->fieldExists('attributes[alt]');
    +    $page->pressButton('Close');
    +    $this->getSession()->switchToIFrame('ckeditor');
    

    Same here.

  31. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +426,340 @@ public function testEditableCaption() {
    +    $this->assertSourceAttributeSame('alt', NULL);
    +    $this->leaveSourceMode();
    

    There's a little bit of "magic" here that affects readability. Ideally we'd see something like:

    $this->enterSourceMode();
    $this->assertSourceAttributeSame();
    $this->leaveSourceMode();
    

    If enterSourceMode() and leaveSourceMode() are too short to justify having their own helper methods, then maybe replacing them with calls to $this->pressEditorButton('source') is sufficient. Either that, or we could have assertSourceAttributeSame() leave source mode before returning.

  32. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +426,340 @@ public function testEditableCaption() {
    +    $this->assertNotEmpty($assert_session->waitForElementVisible('xpath', '//img[contains(@alt, "default alt")]'));
    

    I'm not sure how I feel about this selector; we're not checking if the img is inside a drupal-media element. What if we changed this to $assert_session->waitForElementVisible('css', 'drupal-media img[alt*="default alt"]'), which should have the same effect?

    If we change this here, let's change it everywhere in the test that is modified by this patch.

  33. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +426,340 @@ public function testEditableCaption() {
    +    // Assert that the img within the media embed within the CKEditor contains
    +    // the overridden alt text set in the dialog.
    +    $this->assertNotEmpty($assert_session->waitForElementVisible('xpath', '//img[contains(@alt, "' . $who_is_zartan . '")]'));
    

    Same thing here.

  34. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +426,340 @@ public function testEditableCaption() {
    +    // Note: intentionally adding a space after the two double quotes to test
    +    // the string is trimmed to two quotes.
    

    Minor rephrase: "We intentionally add a space..."

  35. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +426,340 @@ public function testEditableCaption() {
    +    $this->assertNotEmpty($img = $assert_session->waitForElementVisible('css', 'drupal-media img[alt=""]'));
    +    $this->assertTrue($img->hasAttribute('alt'));
    +    $this->assertEmpty($img->getAttribute('alt'));
    

    The assertTrue() and assertEmpty() calls are not necessary. I think the CSS selector is already implicitly asserting both of these things. (If we remove these, we can also remove the inline $img assignment.)

  36. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +426,340 @@ public function testEditableCaption() {
    +    $this->grantPermissions(Role::load(RoleInterface::AUTHENTICATED_ID), ['translate any entity']);
    

    This can also use Role::load()->grantPermissions()->save().

  37. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -362,6 +426,340 @@ public function testEditableCaption() {
    +    $assert_session->fieldExists('attributes[alt]')->getValue();
    

    This line is useless; the return value from getValue() is not used, and the subsequent elementAttributeContains() assertion checks for the attributes[alt] field anyway.

  38. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,34 +1009,176 @@ public function previewAccessProvider() {
    +    $result = $page->waitFor(10, function () use ($page) {
    +      $metadata_editor = $page->find('css', 'form.editor-media-dialog');
    +      return !empty($metadata_editor);
    +    });
    +    $this->assertTrue($result);
    

    The waitFor() callback function doesn't need to use ($page); the function is given the element that waitFor() was called on. So this can become:

    $result = $page->waitFor(10, function ($page) {
      // ...
    });
    
  39. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,34 +1009,176 @@ public function previewAccessProvider() {
    +   * Fills in field with specified locator in EditorMediaDialog form.
    

    I think it would be clearer to say "Fills in a field in the metadata dialog for an embedded media item."

  40. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,34 +1009,176 @@ public function previewAccessProvider() {
    +   * @param string $locator
    +   *   The input id, name or label.
    

    Should be "The field ID, name, or label."

  41. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,34 +1009,176 @@ public function previewAccessProvider() {
    +    $this->assertNotEmpty($this->assertSession()->waitForElementVisible('css', 'drupal-media', 2000));
    +    $page->pressButton('Edit media');
    

    For better scoping, we should do this:

    $this->assertNotEmpty($embedded_media = $this->assertSession()->waitForElementVisible('css', 'drupal-media'));
    $embedded_media->pressButton('Edit media');
    
  42. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,34 +1009,176 @@ public function previewAccessProvider() {
    +    $this->assertNotEmpty($button = $this->assertSession()->waitForButton('Edit media'));
    +    $button->press();
    

    Same idea here.

  43. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,34 +1009,176 @@ public function previewAccessProvider() {
    +    $this->assertNotEmpty($button = $this->assertSession()->waitForButton('Edit media'));
    +    $button->keyDown($char);
    

    And, similarly, here:

    $assert_session = $this->assertSession();
    $this->assertNotEmpty($embedded_media = $this->assertSession()->waitForElementVisible('css', 'drupal-media');
    $assert_session->buttonExists('Edit media', $embedded_media)->keyDown($char);
    

    In some of $assert_session's methods, we can take advantage of an optional $container parameter, which lets us "scope" the assertion to a particular element. Really useful way to increase explicitness and specificity.

  44. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -611,34 +1009,176 @@ public function previewAccessProvider() {
    +    $result = $page->waitFor(10, function () use ($page) {
    +      $metadata_editor = $page->find('css', 'form.editor-media-dialog');
    +      return empty($metadata_editor);
    +    });
    +    $this->assertTrue($result);
    

    Again, no need to use ($page) here.

  45. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -754,4 +1292,26 @@ protected function clickPathLinkByTitleAttribute($text) {
    +    if (!empty($list[0])) {
    +      $drupal_media = $list[0];
    +      $this->assertInstanceOf(\DOMNode::class, $drupal_media);
    +      return $drupal_media;
    +    }
    +    return NULL;
    

    No need for the assertInstanceOf(). DOMXPath::query() will always return a set of DOMNodes, so this whole bit can just be:

    return count($list) > 0 ? $list[0] : NULL;
    
  46. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -141,6 +140,53 @@ public function testConfigurationValidation() {
    +    // Now test adding a new format.
    +    $this->drupalGet('/admin/config/content/formats/add');
    +    $this->showHiddenFields();
    +    $page->fillField('name', 'sulaco');
    +    $page->fillField('format', 'sulaco');
    +    $page->checkField('roles[authenticated]');
    +    $page->selectFieldOption('editor[editor]', 'ckeditor');
    +    $this->assertNotEmpty($target = $assert_session->waitForElementVisible('css', 'ul.ckeditor-toolbar-group-buttons'));
    +    $this->assertNotEmpty($buttonElement = $assert_session->elementExists('xpath', '//li[@data-drupal-ckeditor-button-name="DrupalMediaLibrary"]'));
    +    $buttonElement->dragTo($target);
    +    $page->pressButton('Save configuration');
    +    $assert_session->pageTextContains('The Embed media filter must be enabled to use the Insert from Media Library button.');
    +    $page->checkField('filters[media_embed][status]');
    +    $page->pressButton('Save configuration');
    +    $assert_session->pageTextContains('Added text format sulaco.');
    +    // Test that DrupalMediaLibrary button drupal-media adds allowed tags.
    +    $this->drupalGet('/admin/config/content/formats/manage/sulaco');
    +    $page->checkField('filters[filter_html][status]');
    +    $expected = 'drupal-media data-entity-type data-entity-uuid data-align data-caption alt title';
    +    $allowed_html = $assert_session->fieldExists('filters[filter_html][settings][allowed_html]')->getValue();
    +    $this->assertContains($expected, $allowed_html);
    +    $page->pressButton('Save configuration');
    +    $assert_session->pageTextContains('The text format sulaco has been updated.');
    +    // Test that the saved filter format has the new allowed html tags on the
    +    // <drupal-media> tag.
    +    $format = FilterFormat::load('sulaco');
    +    $allowed_html = $format->filters('filter_html')->settings['allowed_html'];
    +    $this->assertContains($expected, $allowed_html);
    +    // Test that config form allows removing non-required attributes from
    +    // the <drupal-media> tag.
    +    $this->drupalGet('/admin/config/content/formats/manage/sulaco');
    +    $allowed_html_field = $assert_session->fieldExists('filters[filter_html][settings][allowed_html]');
    +    $allowed_html = $allowed_html_field->getValue();
    +    $search = 'drupal-media data-entity-type data-entity-uuid data-align data-caption alt title';
    +    $replace = 'drupal-media data-entity-type data-entity-uuid';
    +    $allowed_html = str_replace($search, $replace, $allowed_html);
    +    $this->assertFalse($allowed_html_field->isVisible());
    +    $page->clickLink('Limit allowed HTML tags and correct faulty HTML');
    +    $this->assertTrue($assert_session->waitForElementVisible('named', ['field', 'filters[filter_html][settings][allowed_html]']));
    +    $allowed_html_field->setValue($allowed_html);
    +    $page->pressButton('Save configuration');
    +    $assert_session->pageTextContains('The text format sulaco has been updated.');
    +    $format = FilterFormat::load('sulaco');
    +    $allowed_html = $format->filters('filter_html')->settings['allowed_html'];
    +    $this->assertContains($replace, $allowed_html);
    +    $this->assertNotContains($search, $allowed_html);
    

    This test coverage is much more extensive than what's in the related, blocking issue -- what's up with that? :)

oknate’s picture

Status: Needs work » Needs review
FileSize
32.35 KB
72.73 KB

Addressing #134:
2. removing $this->assertFalse($allowed_html_field->isVisible()); I left that in there because I thought it made it clear why we’re doing the next line, which makes it visible. Removing per your suggestion it’s not really needed.
3. ✅ Updated, that is cleaner.
4. I don’t think it’s underhanded. What I’m trying to test is that nothing, no JavaScript, no PHP code adds these attributes back. I have just entered the $allowed_value in the field, so there’s no point checking that the field contains the $allowed_value at this point in the test Perhaps this whole section should be removed. But we don’t have test coverage other than this that a site builder can remove these automatically-added attributes.
Addressing #135:
1. ✅ Added a comment.
2. ✅ Added path to filter.caption.css.
3. ✅ Changed “buttons” to “edit button”.
5. ✅ Added theme function. It seems like the right thing to do.
6. ✅ Added a comment explaining what’s going on.
7. ✅ Added some comments around the keydown event opening the dialog.
8. ✅ Removed the check if $context[‘items’] is valid. There is a chance another hook could remove $context[‘items’], because it’s passed by reference and not value, but that’s be pretty foolish. I guess it’s pretty safe to assume it will be there.
9. ✅ Thanks added your comment.
10. ✅ Removed “by convention” and added a @see statement.
11. ✅ chained $form_state method calls.
12. ✅ moved the loading of the translation into the if statement for the alt field, and added a brief comment. I think it’s pretty self-explanatory.
13. ✅ Removed the variable $edit_url.
14. ✅ added a comment explaining why tabindex -1 is there.
15, 16, 17. ✅, ✅, ✅ All good points about the docblock for getMediaImageSourceField().
18. ✅ Renamed the method, per your recommendation.
20. ✅ Updated comment.
21. Assistive technologies such as screenreaders require an alt tag or it will just read out “img”. A blank or NULL alt tag signals to the screenreader to not say “img”.
22. ✅ Changed to “character code”.
23. ✅ Removed the $button variable.
24. ✅ Changed the selector from ‘figcaption’ to ‘drupal-media figcaption’.
25. ✅ Removed the timeout. I don’t know what I was thinking there. I know i had added a 2 second timeout when I was expecting it not to be there, some other place, and I must have copied it over.
26. ✅ Updated comment.
27. ✅ Removed call to $this->grantPermissions() and used your code instead, slightly modified. RoleInterface has grantPermission() (singular) not grantPermissions().
28. ✅ Updated the linkExists to check the full text.
29, 30. ✅ ✅ Added assertions that those two fields are still present.
31. ✅ Removed $this->pressEditorButton('source'); from ::getDrupalMediaFromSource() to make things less magical.
32, 33. ✅ ✅ That’s better, although I think what I was doing was fine, given that the test is a controlled environment.
34. ✅
35. ✅ removed the additional checks around the alt attribute.
36. ✅ Updated.
37. ✅ Removed the getValue() that’s not being used. Good catch.
38. ✅ Ah, I didn’t know that, about it being given the object it’s called on. Cool.
39. ✅ Updated comment.
40. ✅ Updated comment.
41, 42. ✅ ✅ ✅ Updated to use calls on node element.
45. ✅ Nice. Much more succinct.
46. The test coverage in the blocking issue came first, and I created that issue before writing the test coverage here. Also, the test coverage in the blocking issue doesn’t cover adding the attributes automatically. In order to add all the test coverage here to the blocking issue, we’d have to make the changes to the drupalmedialibrary js to automatically add alt, title, data-caption and data-align. That would be out of scope for the regression bug fix. Also, now that it’s RTBC, I’d rather not update that issue to add the additional test coverage that I’d have to strip down to remove the new functionality.

oknate’s picture

Fixing a minor capitalization inconsistency.

oknate’s picture

Some updates based on feedback Wim added on the blocking issue #3078161: DrupalMediaLibrary plugin breaks things when adding a new text editor. One comment was garbled with an extra word and the other needed 'HTML' capitalized. Also, another place 'Sulaco' should be capitalized now that the label is capitalized.

oknate’s picture

Reroll, and fixing this wording of a comment a bit.

phenaproxima’s picture

Title: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption` » [PP-1] Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`
Status: Needs review » Postponed

These are all fixable on commit. Blocking on #3078161: DrupalMediaLibrary plugin breaks things when adding a new text editor, but otherwise, consider this a postponed RTBC. We have put this patch very thoroughly through its paces.

  1. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -237,6 +247,10 @@
    +            // any align classes from the widget wrapper.  These classes
    

    Supernit: There's an extra space before "These classes..."

  2. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -394,14 +409,25 @@
    +              //The character code for the return key.
    +              const returnKey = 13;
    

    The comment needs a space after the //.

  3. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -394,14 +409,25 @@
    +                  // clicks the edit button that triggered the 'keydown'
    +                  // event.
    

    Should be 'Click', not 'clicks'.

oknate’s picture

Addressing #140
1. Removed extra space.
2. Added space.
3. Changed to uppercase the word “Clicks”.

Also, pulled over the latest feedback from the blocker issue.

Wim Leers’s picture

  • #126/#127/#128/#132: 👍
  • #130 🥳 Thanks for validating this!
  • #135.10: No, it is by convention. There's a subtle but important thing you missed 🙂Note that EditorMediaDialog works for any text editor. You're right that ckeditor.es6.js explicitly puts the data under a editor_object key. The convention is that all text editor plugins (other than the CKEditor one) should do the same. This needs to be restored.
  • Great teamwork here, @phenaproxima & @oknate! 👏

#138 + #141: It's confusing to see that this patch needs to be updated when the blocking issue is updated. Apparently this completely includes the blocking issue's changes, instead of having a separate do-not-test patch that shows what the scope is of this issue. I know this is kind of a bother in a patch-based workflow, but it's not that hard to do. git rebase -i is your friend :)

phenaproxima’s picture

#135.10: No, it is by convention. There's a subtle but important thing you missed 🙂Note that EditorMediaDialog works for any text editor. You're right that ckeditor.es6.js explicitly puts the data under a editor_object key. The convention is that all text editor plugins (other than the CKEditor one) should do the same. This needs to be restored.

I'm okay with restoring the "by convention" wording in the comment. However, unless I'm misunderstanding the word "convention", and I might be, this feels more like an API.

"Convention" makes it sound optional, as if there is wiggle room in the way the data is named. But if an editor does not pass a data package called editor_object, the dialog will break. That seems like an undocumented API to me.

But, we are quibbling over words in a comment, so this is not terribly important. If we want to debate and/or iterate on this this later, we would want to correct it in both EditorImageDialog and EditorMediaDialog (and any other relevant dialogs). So, +1 for restoring the wording, but I wanted to clarify the reason I requested the change in the first place.

oknate’s picture

Here's the patch broken up as requested with a do-not-test patch Also, per Wim's request restoring the controversial word "convention" to the comment.

Wim Leers’s picture

@oknate said in Slack he was surprised at the passionateness with which @phenaproxima and I are discussing that editor_object detail.

I don’t feel particularly passionate about that, but I’m really careful about tying things to CKEditor. The Drupal community is super sensitive about tying our fate to one particular library. So I want to ensure we’re not tying this new thing (EditorMediaDialog) to CKEditor-specific things (like @see core/modules/ckeditor/js/something) because that will surely result in Drupal drama in the future
So it’s really about being precise and cautious, not passionate 😊

Wim Leers’s picture

Title: [PP-1] Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption` » Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`
Status: Postponed » Needs work

And yay, #3078161: DrupalMediaLibrary plugin breaks things when adding a new text editor just landed!

One more review of the code changes/additions since this needs a reroll anyway thanks to the blocker having landed. Basically only extreme nits:

  1. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -152,6 +189,13 @@
    +          // @see media_field_widget_form_alter().
    

    🤓 Übernit: the trailing period can be removed.

  2. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -160,12 +204,35 @@
    +            // The server-side text filter plugin treats both an empty
    +            // `data-caption` attribute and a non-existing one the same: it
    +            // does not render a caption. But in the CKEditor Widget, we
    +            // need to be able to show an empty caption with placeholder
    +            // text using CSS even when technically there is no `data-caption`
    +            // attribute value yet. That's why this CKEditor Widget has an
    +            // independent `hasCaption` boolean (which is not an attribute)
    +            // to know when to generate a non-empty `data-caption`
    +            // attribute when the content creator has enabled caption: this
    +            // makes the server-side text filter render a caption, allowing
    +            // the placeholder-rendering CSS to work.
    +            // @see core/modules/filter/css/filter.caption.css
    +            // @see ckeditor_ckeditor_css_alter()
    

    🤓 80 cols

  3. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -179,6 +246,17 @@
    +            // If data-align property is removed, we have to manually remove
    

    🤓 Nit: s/we have to manually remove/remove/

  4. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -179,6 +246,17 @@
    +            // any align classes from the widget wrapper. These classes
    +            // are moved to the wrapper so that the alignment will still
    

    Again, 80 cols.

  5. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +302,136 @@
    +            // Some browsers will add a <br> tag to a newly created DOM
    +            // element with no content. Remove this <br> if it is the only
    +            // thing in the caption. Our placeholder support requires the
    +            // element be entirely empty.
    

    Again, 80 cols.

  6. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +302,136 @@
    +           * Determine if a node is an element node.
    

    🤓 Übernit: s/Determine/Determines.

  7. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
    @@ -224,7 +302,136 @@
    +          // If there is a link, the top-level element is the `a tag,
    +          // and the embedded media will be within the `a` tag.
    

    Again, 80 cols.

  8. +++ b/core/modules/media/media.module
    @@ -496,3 +496,15 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
    +  // Add an attribute so that drupalmedia's JavaScript can pass the host
    

    s/drupalmedia's JavaScript/the "drupalmedia" CKEditor plugin"/

  9. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
    @@ -0,0 +1,257 @@
    +      // Prevent this hidden element from being tabbable.
    +      '#attributes' => [
    +        'tabindex' => -1,
    +      ],
    

    Could you open an issue to fix this in EditorLinkDialog and EditorImageDialog too? 🙏 That's a simple but effective accessibility improvement! :)

phenaproxima’s picture

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

This was manually tested, had its accessibility reviewed, had its code scrutinized and comes with a lot of test coverage. I think this is ready :)

phenaproxima’s picture

Adding credit where credit is due.

webchick credited ckrina.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Also crediting people from the UX meeting.

Adam walked me through this patch over Zoom and everything looks great! The changes the UX team asked for have also been reflected (shorter filter description, default format settings, and the remove buttons removed).

However, when I went to commit it, encountered some problems from our coding standards checks:

/Users/angie.byron/Sites/drupal/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
  106:65  error  Replace `'Edit·media'` with `⏎······'Edit·media',⏎····`                                                              prettier/prettier
  361:66  error  Replace `Drupal.theme('mediaEmbedEditButton')` with `⏎············Drupal.theme('mediaEmbedEditButton'),⏎··········`  prettier/prettier
  421:37  error  Expected '!==' and instead saw '!='                                                                                  eqeqeq

✖ 3 problems (3 errors, 0 warnings)
  3 errors, 0 warnings potentially fixable with the `--fix` option.
/Users/angie.byron/Sites/drupal/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
  106:65  error  Replace `'Edit·media'` with `⏎······'Edit·media',⏎····`                                                              prettier/prettier
  361:66  error  Replace `Drupal.theme('mediaEmbedEditButton')` with `⏎············Drupal.theme('mediaEmbedEditButton'),⏎··········`  prettier/prettier
  421:37  error  Expected '!==' and instead saw '!='                                                                                  eqeqeq

✖ 3 problems (3 errors, 0 warnings)
  3 errors, 0 warnings potentially fixable with the `--fix` option.

So a quick "needs work" to fix those small problems, and then go ahead and set back to RTBC.

webchick’s picture

Oopsie, one more.

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
69.91 KB
2.1 KB

And, done. I installed @alexpott's d8githooks so that I will be able to catch these things in the future, too. Restoring RTBC per #155.

  • webchick committed 5b9bb16 on 8.8.x
    Issue #2994702 by oknate, Wim Leers, phenaproxima, kcolaers, webchick,...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Patch came back green, no yarn complaints, sooooo....

COMMITTED THE VERY LAST FEATURE PATCH FOR MEDIA LIBRARY TO CORE, WOOHOO!!!!!!!!

GREAT work, everyone!! :D

HeikkiY’s picture

Are there any plans to take decorative images into account in the future? At the moment I don't think it is possible to leave the alternative text field empty. But according to WCAG this is the recommended behavior for decorative images (https://www.w3.org/WAI/tutorials/images/decorative/) which don't include any valuable information for screen readers. Should this be opened as a separate issue or more related to the original issue https://www.drupal.org/project/drupal/issues/3023807?

Wim Leers’s picture

Issue tags: +Needs followup

#161: Excellent question!

+++ b/core/modules/media/src/Form/EditorMediaDialog.php
@@ -0,0 +1,257 @@
+        '#required_error' => $this->t('Alternative text is required.<br />(Only in rare cases should this be left empty. To create empty alternative text, enter <code>""< /code> — two double quotes without any content).'),

That is already supported: enter "" as the alt text, just like in the existing image dialog :)

But … somewhere along the way, we made it so that this #required_error is not ever triggerable, so it's much harder to discover this capability than it is in EditorImageDialog.

I think we need a follow-up for that: "Improve usability for embedding decorative image media".

Status: Fixed » Closed (fixed)

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