Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
See #2994696-214: Render embedded media items in CKEditor, patch review point 2:
🤔 In #25, this used to be more abstract:
// Allow entity_embed.editor.css to respond to changes (for example in alignment). this.element.setAttributes(this.data.attributes);It was more abstract for a reason: custom and contrib modules may layer additional behavior, styling or semantics on top of existing HTML elements by adding more
data-attributes. For example,data-highlight,data-editor-comments,data-invert-colors,data-mirror,data-geolocation, and so on. That's exactly whatFilterAligndoes viadata-alignandFilterCaptionviadata-caption. ThisThe current code hardcodes only behavior for
data-align. Yes, I know this plugin has special behavior fordata-caption. There's no other way for that one. Butdata-alignwas implemented generically, and at least some otherdata-somethings could achieve the same.In my opinion, this is a regression compared to earlier patch iterations. But I won't block the patch over this.
In #216 of that issue, we brought that back, to not break that use case. But that means the align-* classes are no longer absolutely necessary, they're just the equivalent of syntactic sugar.
Proposed resolution
Remove the align-* classes, i.e. remove this code:
if (this.data.attributes.hasOwnProperty('data-align')) {
this.element
.getParent()
.addClass('align-' + this.data.attributes['data-align']);
}
Remaining tasks
TBD
User interface changes
TBD
API changes
TBD
Data model changes
TBD
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | 3072279-17.patch | 6.74 KB | oknate |
| #10 | 3072279-10--combined-w-3084340-2.patch | 9.48 KB | oknate |
| #10 | 3072279-10--do-not-test.patch | 6.77 KB | oknate |
Comments
Comment #2
phenaproximaComment #3
oknateComment #4
oknateI tested this out this morning, and it's jarring to set it right-aligned and then see it display in the WYSIWYG as centered. While I think this would simplify the code and reduce maintenance on the drupalmedia plugin.js, I think the UX problem of not having some sort of alignment preview in the WYWIWYG is too great. So I'm voting against this until I can be convinced otherwise.
I'm updating this comment 9 days later: The ask here is not to stop aligning the media in the WYSIWYG, but to go back to using the data-align attribute.
Comment #5
oknateI'm not sure I understand this:
You mean, we could use the data-align for the css?
Comment #6
Wim LeersIf we did this, then #3081983: Double Alignment classes on widget wrapper for embedded media in CKEditor would not have been necessary.
Comment #7
Wim LeersBut that's because we chose to make the CSS selectors rely on classes instead of
data-attributes. That's easily changed. If we change that, then everything continues to function exactly the same.Comment #8
Wim Leers#6 said:
Nor would #3081357: Media alignment mismatch between the WYSIWYG and rendered media -- drupalmedia.js shouldn't convert data-align properties when Filter Align disabled be necessary.
Comment #9
lauriiiThank you @Wim Leers for walking through this issue with me on a call. I think we should solve this rather than #3081357: Media alignment mismatch between the WYSIWYG and rendered media -- drupalmedia.js shouldn't convert data-align properties when Filter Align disabled. @Wim Leers explained that this is currently added to an element that is added by CKEditor only in CKEditor. This helped me understand that only CKEditor specific CSS should be able to change styling of this element. I'm sorry to cause us to go back and forth on this, but I do think this is a better approach. This allows us to close #3081357: Media alignment mismatch between the WYSIWYG and rendered media -- drupalmedia.js shouldn't convert data-align properties when Filter Align disabled and should also simplify some of the code in #3078287: Constrain the width of aligned images, media, blockquotes etc to a max of 75%.
Comment #10
oknateHere's an initial patch for this. Unfortunately, I found a bug that wasn't an issue when we were adding our own classes, but now is getting in the way of this issue. #3084340: drupalmedia plugin.js doesn't delete attributes from drupal-media element when unset.
Other than that, this seems ready to me. Perhaps I'll test more, for example with Umami. But it works fine.
Comment #11
phenaproximaNot postponed anymore!
Comment #12
oknateActually, I postponed it five minutes ago, because I think #3084340: drupalmedia plugin.js doesn't delete attributes from drupal-media element when unset. is a blocker.
Comment #13
phenaproximaWhoops, sorry! Let's get that one fixed.
Comment #14
oknateOne coding standard warning:
Comment #15
oknateDropping "Consider" from issue title.
Comment #16
larowlanBlocker is in
Comment #17
oknateThanks, @larowlan! Here's a reroll of #10.
Comment #18
Wim LeersManually tested.
Works perfectly.
Comment #19
Wim LeersThis blocks #3078287: Constrain the width of aligned images, media, blockquotes etc to a max of 75%, which is , so matching that priority.
Comment #20
webchickAwesome, this ends up being a nice simplification of the code, in addition to fixing up things!
Committed and pushed to 8.8.x. Thanks!
Comment #22
Wim LeersYay, #3078287: Constrain the width of aligned images, media, blockquotes etc to a max of 75% is now unblocked!