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:
Override metadata modal proposal:
Also made a proposal on what the image with the buttons could look like:
Remaining tasks
- Reviews!
- UX sign-off
- 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
anddata-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 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...
Click "Insert selected", you should now see
Comment | File | Size | Author |
---|---|---|---|
#158 | interdiff-2994702-147-158.txt | 2.1 KB | phenaproxima |
#158 | 2994702-158.patch | 69.91 KB | phenaproxima |
#147 | interdiff-2994702-144-147.txt | 6.45 KB | phenaproxima |
#147 | 2994702-147.patch | 69.86 KB | phenaproxima |
#144 | 2994702-144--combined--3078161-31.patch | 72.69 KB | oknate |
Comments
Comment #3
Wim LeersThinking 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.Comment #4
Wim LeersApparently there's an issue for what I described in #3: #3023807: Override media fields from the reference field.
Comment #5
Wim LeersWe 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.
Comment #6
phenaproximaI 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.
Comment #7
seanBAgreed! 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 :)
Comment #8
webchickI 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:
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.
Comment #9
webchickOops, guess we still need "more info" from the a11y folks.
Comment #10
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedI 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:
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.
Comment #11
Wim LeersThanks, @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 :)
Comment #12
Wim LeersQuoting one very relevant bit from #2998005-51: [PP-1] Support Drupal core's Media Library:
#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
anddata-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 thatdata-view-mode
will get descoped, and at that point we won't have to deal with that here anymore either.Comment #13
Wim Leers@seanB has posted #3039829-34: Remove link to media item from media library view. whose CSS we can reuse to achieve this:
Here's a first iteration that deals only with
data-align
anddata-caption
!Comment #14
Wim LeersAnd one more iteration, this time with
alt
support and eventitle
support.(The "Image" button in CKEditor and the corresponding
EditorImageDialog
do not yet supporttitle
!).This changes that last screenshot to something that looks exactly like
EditorImageDialog
! 🤩 Well, with two differences:input[type=file]
🥳placeholder
attribute containing thealt
value stored in the Media Library, which signifies that this is the default.Comment #15
Wim LeersA 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. 🥳
Comment #16
Wim LeersApparently I never marked #3039829: Remove link to media item from media library view. as a related issue, oops.
Comment #17
Wim LeersRerolled #15 to incorporate match #2940029-97: Add an input filter to display embedded Media entities's marking of
data-view-mode
and #2994696-23: Render embedded media items in CKEditor's and #2994699-24: Create a CKEditor plugin to select and embed a media item from the Media Library's corresponding updates.Comment #18
phenaproximaI 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:
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.All of these things can be fleshed out in separate issues.
Comment #19
Wim LeersFirst: 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:
So I removed
data-caption
, see #2994699-25: Create a CKEditor plugin to select and embed a media item from the Media Library.Rerolled patch for #18.2.
Comment #20
kcolaers CreditAttribution: kcolaers at Cegeka commentedFilename:
MediaLibrary*Cke*ditorOpener.php
Classname:
MediaLibraryCKEditorOpener
Our build server does not like this.
Comment #21
Wim Leers#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 toeditor.module
.Comment #22
Wim Leers#2940029: Add an input filter to display embedded Media entities landed. Rerolled, on top of #2994696-25: Render embedded media items in CKEditor and #2994699-25: Create a CKEditor plugin to select and embed a media item from the Media Library.
Comment #23
Wim Leers🍽 d.o ate my patch for #22.
Comment #24
Wim LeersThis addresses #20 + #21.
Comment #25
Wim LeersActually, #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
.Comment #26
Wim Leers#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:
I don't know what this was doing here. This file does not exist. Omitted it.
Comment #27
Wim Leers🤓
Comment #28
oknateI suggest we change "Delete media" to something less scary sounding, maybe:
or
Even though I knew it wasn't going to delete the media, I was still a little nervous to push the button. 😱
Comment #29
oknateTodo:
1) change to use .'media-embed-error' class rather than checking the filename.
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:
The editor is only used two places in the form, and both times it's to get the filter format:
and
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:
Unless I'm missing a good reason for them to be different.
Ah, I see, it's following a pattern:
Comment #30
oknateFixing test failure in #29.
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).
Comment #31
oknateOops, forgot to post the patches in the last comment.
Comment #32
oknateI 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:
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:
Changing the code slightly (using array_keys) preserved the original structure (minus the button we're trying to delete):
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.
Comment #33
oknateRemoving test code.
Comment #34
Wim Leers#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!
🤔 Does this truly make sense? Why shouldn't this get buttons? How would you then remove missing media?
🤔 This should indeed use the class to detect missing media, like you wrote in #29.
🦅👀👏
🤓 Übernit: s/image/media/. Also: I introduced this! Sorry :)
🤓 Debug leftover :)
Comment #35
oknateIn response to #34.1:
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.
Comment #36
phenaproximaThe blocker is in, so this is no longer postponed. Avanti! Marking "needs work" to address Wim's feedback in #34.
Comment #37
phenaproximaWhy does this care if this.oldData.hasCaption is true? Seems like, if this.data.hasCaption is false, the data-caption should always be deleted.
Why was this added? If it's necessary, seems like something that might benefit from a comment.
I understand that we need this hack (ugh), but is there any external issue or URL we could reference as well?
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. :)
What is an "element node"? What will this predicate match? Could use a comment.
This could also use a comment, to explain why the presence of a link means that the element upon which we call getFirst() changes.
Should we put this in ckeditor.drupalmedia.css, rather than using an inline style?
Nit: There should be an empty line between these two, IMHO.
I agree that "Remove media" is better terminology here than "delete". Let's change it.
!!
? Why would we want to use that? That seems very odd. If we need to do it this way, a comment might be wise.We should expand the
@internal
to include the messaging we've added to other internal classes.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?
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.IMHO, this should probably be
elseif ($form_state->has('media_embed_element'))
. Then we should add an additionalelse
case which throws a BadRequestException. Just in case.Where is the 'src' attribute set? I didn't see any mention of it in the JavaScript. Is this a holdover from drupalimage?
Why is this half the allowed length of 'title'?
This can be shortened:
empty($media_embed_element['data-align']) ? 'none' : $media_embed_element['data-align']
.Why do we need to set both #attributes and #wrapper_attributes?
This seems a bit strange. Won't submitForm() run when the form is submitted?
The hasValue() call is superfluous and can be removed.
Same here. Also, why do we need to set the empty alt to FALSE? Can't we just use $form_state->unsetValue()?
I could be wrong, but I think we can just use
is_a()
here instead of two conditions ORed together.Comment #38
oknateAdding "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.
Comment #39
oknateJust a reroll and an interdiff to back before this weekend, so all the changes so far can be reviewed.
Comment #40
oknateResponses 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:
I found a better way to do this. In the EditorMediaDialog:
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:
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.
Comment #41
Wim LeersIt'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.\Drupal\editor\Form\EditorImageDialog::buildForm()
.EditorImageDialog
. It probably needs tweaking for the difference in context (<drupal-media>
versus<img>
).FALSE
is not the same as a non-existing value.Patch review
EditorMediaDialog
seem to not have been tested. Best indicator of this: there's a PHP syntax error on line 151…🤓 s/non-node element/non-element node/
🤓s/the a tag/the tag/
s/position/positioned/
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.
👎 The if-test ensured this code only ran during changes. It needed comments added, not to be deleted.
👎 This change broke the editing of the metadata pretty much completely 😞
👎 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.
🤔 I'm not sure this change is correct. Also see my response to #37.15 and #40.15.
👎 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.
Comment #42
oknate1. 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:
This was breaking the test this morning and I couldn't get it to work.
Restoring this code:
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:
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.
This completely broke the form, as a string cannot be evaluated as an ImageItem object.
Comment #43
oknateRe: #37.12 and #41.7:
Restoring this change:
It was not this that was breaking the form. It was this:
That needed the TRUE set in the third parameter to work with a string.
Comment #44
oknateMore on #41.10:
when "values" is what's returned from the editor:
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:
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.
Comment #45
Wim LeersYes, 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 whatckeditor.drupalmedia.css
's.cke_widget_drupalmedia.align-center {…}
style is targeting!Comment #46
phenaproximaAnother review (full read-through, including tests). This is shaping up nicely, I think.
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. 👍
Nit: I think
classes[i].indexOf('align-') === 0
might be clearer.This could be
classes.remove(classes[i])
."non-node none"? :)
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] => { ... })
.Should we explicitly require the POST method for this route?
I think this could be $form_state->has('media_embed_element').
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 ansrc
attribute to be here? If so, where would it be coming from?— 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.
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:
We should still remove one of these, as per Wim's +1.
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."
This needs a comment.
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 fortitle
and leave it to contrib.Technically, these sorts of changes are out of scope, but I'm not hugely concerned. :)
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. :)
It seems like this might be better done with FieldConfig::loadByName()->setSetting()->setSetting()->save().
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. :)We also need to assertNotEmpty($img).
We should be using assertSame() here.
We should use $assert_session->fieldExists() to get the alt and title, or we could end up calling getValue() on null.
Nit: > 80 characters.
Why did this change?
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.)
This can be collapsed to one line: $value = $assert_session->elementExists()->getValue().
Shouldn't we be collecting and asserting the result of waitFor()?
This can also be collapsed to one line.
I think we can use $this->assertFalse($drupal_media->hasAttribute()) here.
This can be one line: $this->assertSession()->elementExists()->pressButton().
Comment #47
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI just now started testing #43 and noticed that in the dialog, the Align radio buttons and the Caption checkbox appear even if
data-align
anddata-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?Comment #48
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOh, 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>
?Comment #49
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI also just now tested it with the
alt
attribute. I removed thealt
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.Comment #50
Wim Leers#48: That's indeed what it boils down to. This is also how
EditorImageDialog
handlesdata-align
anddata-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 botEditorImageDialog
andEditorMediaDialog
?Comment #51
effulgentsia CreditAttribution: effulgentsia at Acquia commentedMaybe 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>
)?Comment #52
oknateWe'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:
Comment #53
phenaproximaI 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:
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!role="group"
attribute to thefigure
tag it creates. It would be more accessible to have that either berole="figure"
, or (even better) just removed entirely. We'll do that in a follow-up.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.Comment #54
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedRe. #53:
<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 ofrole=group
in our filter figure probably arose because it was created prior to ARIA 1.1 - so that's really a modernization issue.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.
Comment #55
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedMy roadmap triage for the accessibility bit, whether they get fixed here or in follow-ups.
<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.Comment #56
oknateResponses 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 ofsubstr(0, 6) === 'align-‘
.3. 👎If it makes no difference
removeClass(classes[i]);
is shorter thanclasses.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:
but it results in all this code in the javascript:
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:
nor this:
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.
Comment #57
Wim Leers#51:
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
anddata-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 theallowed_html
setting of@Filter=filter_html
would be: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! 👏
Comment #58
phenaproximaFiled all required follow-ups arising from the UX review.
Comment #59
Wim LeersEditorImageDialog
andEditorMediaDialog
👍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.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.Comment #60
Wim Leersd.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 :)
Comment #61
oknateAddressing Effulgentsia's issue raised in #47 - 51:
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.
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:
It can be
if (!empty($allowed_attributes) && empty($allowed_attributes['alt'])) {
etc.Comment #62
phenaproxima@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
, anddata-caption
attributes if the media is an image, filter_align is enabled, and filter_caption is enabled, respectivelyPros: 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
, anddata-caption
attributes, and hide the Edit button if it doesn'tPros: 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 thedrupal-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:
Comment #63
phenaproximaRemoving 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.
Comment #64
phenaproximaClosing in, I think! This looks really, really good!!
s/dom/DOM
There is no more remove button. :)
Nit: A blank line before this one would be useful.
s/buttons/button
This can all become one line:
'#access' => !empty($settings['alt_field']) && !empty($allowed_attributes['alt'])
Same here.
And here.
It's not clear how the FALSE signifies "use the default value". A @see referencing the relevant JavaScript would be useful.
We don't need this anymore :)
This should have a blank line above it; it's a new "phase" of the test.
These can be one line. Also, let's add a blank line below it.
This test definitely needs some well-placed empty lines in it; it's hard to parse right now.
These changes seem out of scope...
s/dom/DOM
I think we can use the static FilterFormat::load() here. :)
Why is this needed? A comment might be useful here.
Let's also assert that the alt text field is still present.
This test method is a wall of text. Can we break it up a bit with some empty lines? :)
Why do we have $widget as its own variable? Doesn't look like we're using it...?
I suspect this could be a lot simpler -- one line, actually:
$this->assertNotEmpty($assert_session->waitForElementVisible('css', 'drupal-media:not([data-align])', 2000))
.We could use a similar approach here too.
:not()
is a pretty nice CSS feature :)Comment #65
oknateAddressing 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:
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.
Comment #66
oknateComment #67
phenaproximaEverything in here is a nitpick. Feel free to disregard any of them.
Nit: Can we do
$filters = $editor->getFilterFormat()->filters()
above this section, and reuse that?I think we also need a
else { $allowed_attributes = []; }
, no?Not a pattern I've seen elsewhere in core, but it should work :)
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.
This seems oddly phrased...
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.
This does not need the
@throws
, because this method does not explicitly throw an exception.Same here.
And here.
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.Comment #68
oknate1. ✅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.
Comment #69
phenaproximaOne 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.
Comment #70
seanBFinally 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.
alt
,data-align
, anddata-caption
on thedrupal-media
tag. That could use some more guidance in the interface, I wouldn't have figured that out without someone telling me.Comment #71
oknate1. ✅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:
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.
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".
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.
Comment #72
oknateIn 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.
Comment #73
oknateMarking back as needs review. I think I've addressed all of the latest feedback.
Comment #74
Wim Leers#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 fromEditorImageDialog
in #14. You're absolutely right! 👍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?
I think this can be removed?
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.
"equal to 1" is meaningless here.
s/non-node element/non-element node/
(Already said this in #41.)
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.This should load the
alt
value that matches the language of the host entity.Do we have test coverage for that?
Why 512?
EditorImageDialog
uses 2048. If we deviate from that, this needs a comment."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.
This is not handling the
'""'
case. See\Drupal\editor\Form\EditorImageDialog::submitForm()
.This is dead code.
title
is gone as of some comments ago.. 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/ordata-caption
attributes are allowed on the<drupal-media>
tag. If you would like users to be able to override the alt text on image media, add thealt
attribute as well to the<drupal-media>
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.
This requires expanded test coverage in
\Drupal\Tests\media\Kernel\MediaEmbedFilterTest
.Comment #75
phenaproximaAgreed 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.
Comment #76
oknateResponding 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,
but when I try to assert that it’s equal to NULL, it fails.
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.
Comment #77
oknateForgot to commit one change, deleting the assertion about a drupal-media element without data-align attribute, related to #74.2.
Comment #78
oknateAddressing #74.7
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 thedata-langcode
as the language. I added an issue for it: #3075593: Allow MediaEmbed filter to use data-langcode to set media translation.Comment #79
oknateUPDATE: 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.
On the form route editor.media_dialog I don't think that would detect the current language in all language negotiation configurations.
Comment #80
oknateContinuing 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.
Comment #81
oknateRemoving a line of test code.
Comment #82
oknateRemoving unused use statements.
Comment #83
phenaproximaI 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. :)
Comment #84
phenaproximaAll aboard the review train! 🚂
This could use a @see referencing where the attribute is added.
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...
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.This assumes that $form['alt'] exists at all, which it might not if the media being embedded is not an image.
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.
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: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'], '')))
.The comment still mentions the 'title' attribute. :) Also, supernit: the word "We" is capitalized, but shouldn't be.
"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."
Let's also assert that our error text does not appear.
The word "setting" is superfluous here.
This should be doing
$alt = $assert_session->fieldExists('attributes[alt]')->getValue()
, or we risk calling getAttribute() on NULL.We don't need the
$img =
assignment here, or anywhere else that we're not actually using the variable for anything. :)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.
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.)I think we should probably use $this->resetAll(), rather than $this->rebuildContainer().
As a lapsed non-native francophone, I approve this message. 👍
Do we actually need to set $translation->body->lang? As far as I can tell, that is not an actual property of text fields.
This also needs to use the $assert_session->fieldExists()->getAttribute() pattern.
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().
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.
...er, did we enter source mode or leave it? The comments contradict each other.
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.'title' is overridable? It's...not, though. We don't support it. Why do we need this test case?
Comment #85
oknateResponding to feedback in #84:
1. ✅ Added a note.
2. ✅ Removed the entity check and added a check that getEntity isn’t NULL.
3. ✅ Combined the conditions.
4. ✅ Adding an empty check on $form[‘alt’], good catch there.
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:
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.
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.
Comment #87
oknateFixing 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().
Comment #88
oknateI was missing the latest commits in my working branch. Rerolling.
Comment #89
phenaproximaSuperdupernit: 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.
Comment #90
oknateOne 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?
Comment #91
oknateRemoving the extra space (#89.1).
Comment #92
Wim LeersThis is looking very close now!
data-align
is not an override at all.This should use
$format->access('update', …)
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'sdata
is for (the canonical representation/storage of the current state).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
🤔 I think we should rename this from
data.langcode
to the more verbosedata.hostEntityLangcode
, to avoid somebody potentially misreading this asdata-langcode
, which is not supported (see #3075593: Allow MediaEmbed filter to use data-langcode to set media translation for that).This is not wrong, but I think this would be clearer:
(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!)
There is only one button now! So perhaps this should be renamed to
setUpEditButton()
?👎 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.
. 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/ordata-caption
attributes are allowed on the<drupal-media>
tag. If you would like users to be able to override the alt text on image media, add thealt
attribute to the<drupal-media>
tag as well."),👍
👍 Excellent, this is the high-level dialog test coverage: which form fields appear, and which don't.
🤔 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']);
.👍 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 analt
attribute set — that is what would conclusively prove that theplaceholder
attribute for thealt
form field in the dialog was in fact loaded directly from the underlyingMedia
entity, and not from an existing override. This is in principle not necessary to check, but it gives us extra protection against future regressions.TIL 😆
🤓 Nits:
👏👏👏
🤓 Übernit: extraneous empty line.
🤓🤓 I think it's weird that we have
`data-align`
yet alsodrupal-media
. It should be either<drupal-media>
or`drupal-media`
.🤓 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
).🤔 This would be better I think:
Parses the <drupal-media> element from CKEditor's "source" view.
Comment #93
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIs 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.
Comment #94
oknateI found this comment in the original issue from me a few months ago:
#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.
Comment #95
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis 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).Comment #96
oknateFeedback 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
None of the above worked as a replacement. I ended up with a slight improvement:
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.
Comment #97
Wim Leers\Drupal\editor\Form\EditorImageDialog::buildForm()
. Why do we need validation here and not inEditorImageDialog
?Interdiff review
👎 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>
.#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.Nit: there's no need for the
$field
variable, this can be entirely chained.👍 This is the test coverage I was looking for in #92.8.
This is also a good addition! 👍
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!
Comment #98
oknateResponding 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.
Comment #99
oknateOn slack, phenaproxima wrote:
This patch does that.
Comment #100
oknateAdding an interdiff for #98 to #99.
Comment #101
phenaproximaThis 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.
Comment #102
oknateRemoving magical code (#101).
Comment #104
oknateUgh. 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.
Comment #105
Wim LeersThis 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
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()
: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.)
Comment #106
oknateI looked through the accessibility issues raised and here's a little summary:
Accessibility issues addressed:
Accessibility follow-ups files:
Comment #107
rainbreaw CreditAttribution: rainbreaw as a volunteer and commentedReviewed this on a screen share call with @oknate and @phenaproxima, and here is what we uncovered that are ongoing accessibility concerns:
Comment #108
bwebster719 CreditAttribution: bwebster719 commentedIn 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.
Comment #109
oknateWorking 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.
Comment #110
oknateAdding 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.
Comment #111
oknateAddressing #108. Fail patch shows the bug. It's the same as the other patch, minus this:
There's very similar code elsewhere in plugin.js:
But this only works when there is this.oldData. It doesn't handle the case where someone manually adds data-caption="" in source mode.
Comment #112
Wim Leers#107 Thanks so much for your accessibility review! 🙏🥳
EditorImageDialog
.#108: 🙏👏 NICE FIND! It results in
data.hasCaption=true
anddata.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.
Comment #113
phenaproximaCrediting @bwebster719 and @rainbreaw for reviews and finding bugs.
Comment #114
phenaproximaOh, and @andrewmacpherson.
Comment #115
oknateI'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:
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.
Comment #116
Wim LeersRE: myself in #112: — 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"
?Comment #117
oknatetabindex="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.
Comment #118
oknate@wim-leers came up with this solution.
This fixes the tabbing issue in #109 and #115:
@todo It needs test coverage that the tabbing works.
@todo Still some issues in #107 to look into.
Comment #119
oknateI forgot to change this in the last patch:
Comment #120
oknateResponding 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.
Comment #121
Wim Leers#120.2: I think Seven matters more than Bartik. Content editing usually happens in Seven.
Comment #122
oknateYeah, 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.
Comment #123
oknate👍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.
Comment #124
Wim LeersThis issue is blocked on accessibility review. Test coverage is abundantly present.
Comment #125
phenaproximaWe 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:
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: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.
Comment #126
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #127
phenaproximaThat'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.
Comment #128
webchickRe: #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.
Comment #129
oknatePosting a FAIL patch first before working on the auto-population of the additional attributes in the Allowed HTML settings for FilterHtml.
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
Comment #130
rainbreaw CreditAttribution: rainbreaw as a volunteer and commentedPerformed 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
Comment #131
oknateThank you, @rainbreaw, for your accessibility feedback. Removing "needs accessibility review" tag based on #130:
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.
Comment #132
oknateAdding 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).
Comment #133
oknateThis 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:
See
https://ckeditor.com/docs/ckeditor4/latest/guide/dev_allowed_content_rul...
Comment #134
phenaproximaI 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. 🎉
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 👍
Do we need this assertion? Not sure what it's adding, really...
This might be a little cleaner as:
That way, we don't need to copy and paste the selector.
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.
Not only is this faster, this syntax is more consistent with what's going on in the drupalimage plugin. 👍
Comment #135
phenaproximaI 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.
I think this could use a comment.
Can we change "See filter-caption.css" to just a "@see path/to/filter-caption.css"?
The name of the function is _setUpEditButton (singular), but the comment says "buttons" (plural).
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. 👍
Should this be a JavaScript theme function? (Not blocking feedback; just thought I'd call it out.)
Can this get a short comment explaining what it's doing and why?
This could use a comment as well.
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.
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:
"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.
These calls can be chained.
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.Nit: $edit_url does not need to be its own variable.
This needs a comment.
This is not accurate. How about "Gets the name of an image media item's source field", instead?
How about "The media item being embedded" instead?
The return description starts with a quotation mark, but it shouldn't :)
Let's rename this to more accurately reflect what it does. How about getMediaImageSourceFieldName()?
👍
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."
Is there any reason not to use removeAttribute() here?
🤓 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)
I'm not seeing where $button is subsequently used...?
Wouldn't we want to wait for
drupal-media figcaption
instead, as per the comment?Is there any real reason to pass a timeout to waitForElementVisible()? What was wrong with the default timeout of 10 seconds?
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."
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()
.We should be asserting the full text of the link: "Edit the text format Test format".
Let's also assert that the alignment and caption fields are still visible.
Same here.
There's a little bit of "magic" here that affects readability. Ideally we'd see something like:
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.
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.
Same thing here.
Minor rephrase: "We intentionally add a space..."
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.)
This can also use Role::load()->grantPermissions()->save().
This line is useless; the return value from getValue() is not used, and the subsequent elementAttributeContains() assertion checks for the attributes[alt] field anyway.
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:I think it would be clearer to say "Fills in a field in the metadata dialog for an embedded media item."
Should be "The field ID, name, or label."
For better scoping, we should do this:
Same idea here.
And, similarly, here:
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.
Again, no need to
use ($page)
here.No need for the assertInstanceOf(). DOMXPath::query() will always return a set of DOMNodes, so this whole bit can just be:
This test coverage is much more extensive than what's in the related, blocking issue -- what's up with that? :)
Comment #136
oknateAddressing #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.
Comment #137
oknateFixing a minor capitalization inconsistency.
Comment #138
oknateSome 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.
Comment #139
oknateReroll, and fixing this wording of a comment a bit.
Comment #140
phenaproximaThese 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.
Supernit: There's an extra space before "These classes..."
The comment needs a space after the //.
Should be 'Click', not 'clicks'.
Comment #141
oknateAddressing #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.
Comment #142
Wim LeersEditorMediaDialog
works for any text editor. You're right thatckeditor.es6.js
explicitly puts the data under aeditor_object
key. The convention is that all text editor plugins (other than the CKEditor one) should do the same. This needs to be restored.#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 :)Comment #143
phenaproximaI'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.
Comment #144
oknateHere'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.
Comment #145
Wim Leers@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 futureSo it’s really about being precise and cautious, not passionate 😊
Comment #146
Wim LeersAnd 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:
🤓 Übernit: the trailing period can be removed.
🤓 80 cols
🤓 Nit: s/we have to manually remove/remove/
Again, 80 cols.
Again, 80 cols.
🤓 Übernit: s/Determine/Determines.
Again, 80 cols.
s/drupalmedia's JavaScript/the "drupalmedia" CKEditor plugin"/
Could you open an issue to fix this in
EditorLinkDialog
andEditorImageDialog
too? 🙏 That's a simple but effective accessibility improvement! :)Comment #147
phenaproximaFixed the nitpicks, and filed #3078966: Remove hidden tabbable focus area from editor dialogs.
Comment #148
Wim LeersThis was manually tested, had its accessibility reviewed, had its code scrutinized and comes with a lot of test coverage. I think this is ready :)
Comment #149
phenaproximaAdding credit where credit is due.
Comment #150
phenaproximaNote to committers: the PostgreSQL failure in #147 is due to #3048348: Denormalizing NULL for an optional @FieldType=address or @FieldType=geolocation field fails due to either no main property name or computed read-only main property, and is being dealt with in #3078639: PostgreSQL automated test failing for JsonApiRegressionTest after latest changes on json_api FieldItemNormalizer.
Comment #155
webchickAlso 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:
So a quick "needs work" to fix those small problems, and then go ahead and set back to RTBC.
Comment #157
webchickOopsie, one more.
Comment #158
phenaproximaAnd, done. I installed @alexpott's d8githooks so that I will be able to catch these things in the future, too. Restoring RTBC per #155.
Comment #160
webchickPatch came back green, no yarn complaints, sooooo....
COMMITTED THE VERY LAST FEATURE PATCH FOR MEDIA LIBRARY TO CORE, WOOHOO!!!!!!!!
GREAT work, everyone!! :D
Comment #161
HeikkiY CreditAttribution: HeikkiY commentedAre 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?
Comment #162
Wim Leers#161: Excellent question!
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 inEditorImageDialog
.I think we need a follow-up for that: "Improve usability for embedding decorative image media".