Problem/Motivation
We went through a few iterations here. The solution landed on is a filter format. See comments in #8 and #10
Proposed resolution
Add a new filter format that replaces the functionality already added to editor_file_reference and move lazy loading logic into it. Enable this new filter, but only if editor_file_reference
was enabled. This gives all sites that currently have lazy loading, that same ability. But now gives them the ability to disable lazy loading if they do not want it. This new filter only adds loading="lazy" if the markup on the <img>
tag doesn't already designate something else.
History:
In #3167034: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core, lazy loading was added for editor file referenced files. Back at that point, the understanding from many in the industry was that lazy loading was a great win. But then some things were learned and in certain situations, it would be great to disable that. That is what this new filter does. And it does it by moving that feature out of editor_file_reference into a dedicated editor_image_lazy_load filter.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
A new text filter was added to support <img loading="lazy">
. For any sites that previously had Track images uploaded via a Text Editor
enabled, this filter will be enabled by default.
Comment | File | Size | Author |
---|
Issue fork drupal-3247795
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
heddnComment #3
Wim LeersSame questions as I asked at https://github.com/ckeditor/ckeditor5/issues/10784#issuecomment-961700192 apply here.
I asked:
You responded:
The viewport size and theme determines where "below the fold" occurs. So … I have doubts about this. Strong doubts. Discussion needed! 😊
Only one thing I'm certain about: the current patch would complicate the UX for 100% of sites. That means this can't land with the current UX.
Comment #4
heddnI can't find it in D9/D9, but there used to be a trim indicator in D7 (maybe it was a contrib module?) that you could place on the body to tell where to trim the body field. It was for use with trim/summary in teaser view modes. I could see us doing something similar here. Where we tell ckeditor 600 characters as a default. Then gave the editor an option to insert a marker/indicator where the fold "really" is if they need to tweak the page in one-off situations. Anything above the indicator would be eager. Below, it would be lazy.
Comment #5
heddn@Wim Leers any feedback on #4?
Comment #6
heddnDiscussed this with Fabian Franz and upstream w/ Piotrek Koszuliński this morning on a call. We finally came up with a suggestion direction, which I'm documenting here to get wider feedback.
The suggestion is:
The benefit to an input filter is that the markup stays very clean. It just has the markup for image tags, and nothing else. The benefit to not providing a UI widget to disable the filter's effect are that this is an advanced feature-set that could be confusing to many editors. One note on overriding the input filter, the allowed markup of the text format would need to be updated in Drupal so that
loading="lazy|eager"
is allowed.Comment #7
Wim Leers#6 addresses the concerns I had on #4 😊
But how would the
@Filter
plugin work? What heuristics would it use? Or would it simply make every image haveloading=lazy
?Note that extra care will need to be applied when the image is not using
<img>
markup but<drupal-media>
, i.e. when it is an image from Drupal's Media Library. In that case, you'll need to ensure this new filter runs late enough. Probably setting a very high default weight is sufficient.Comment #8
heddnre #7: the filter as we envisioned it would be called "Lazy load images". It would check markup for
<img>
tags and addloading="lazy"
to them. But it wouldn't overwrite any existing loading="*" designation. This lets folks that have a real need to manually effect lazy loading to 1) adjust the allowed markup then 2) inspect markup 3) manually add the designation to the image tag(s) on individual pages.This feature would be a core filter. It doesn't stop contrib from trying to do something more heuristic. In our discussions, it was very complicated on mobile vs desktop vs widescreen vs responsive image vs (you get the picture) to figure out a deterministic way to decide when to mark something as lazy. Especially when there are teaser, hero, full page, etc view modes. The edge case of turning off lazy load is for Knowledgeable Content Editors who have been directed to improve their SEO Lighthouse/Speedtest scores and are given defined directions how to improve the LCP for specific pages. For those edge cases, they can decide if it is easier to 1) disable the filter and add
loading="lazy"
to a few pages that need it still or 2) keep the filter enabled and setloading="eager"
. Alternatively, they could write a custom or develop a contrib module that has some heuristics to only add loading="lazy" based on some type of indeterminate criteria.Comment #9
benmorss CreditAttribution: benmorss commentedI'd also be curious to learn why you decided not to work within CKEditor itself, but to focus on a filter on the Drupal side instead.
I haven't hacked on CKEditor in a while. Did it simply seem easier to work in an area where the Drupal community has more control... within Drupal?
Comment #10
heddnre #9: we don't want to adjust the actual markup of the ckeditor backed body field. That makes for changing/altering the markup globally at a later point complicated. Ergo, enter Drupal's filter system. It has long been a golden rule of body field markup to keep clean markup. And use filters to augment the data. Having this be a filter, we can easily enable/disable lazy loading across an entire drupal site with a simple toggle and flushing cache.
If we make the change in ckeditor directly, we could not do that. It doesn't have the concept of filters. The lazy loading directive gets directly sprinkled in each and every body field.
As a parallel, we discussed a ckeditor plugin that adds
target="_blank"
to all links. This core ckeditor plugin muddies the markup (no filter). And it is one we as a Drupal community are actively looking to block. For all the reasons I just expounded upon.CKEditor is open to adding it to its core at a later date, similar to the link plugin just mentioned. But since
loading="lazy"
is so new on the market, they want to get more industry input on its addition before implementing. If it did get implemented, Drupal would still block its usage (similar to the link plugin just mentioned) and recommend usage of a Drupal filter plugin instead.Comment #11
benmorss CreditAttribution: benmorss commentedThat makes sense to me. The more complex the markup you inherit from CKEditor, the harder it is to change attributes or undo features you didn't want. I've dealt with that
target="_blank"
issue before in content generation, so I get what you mean!Of course I do still like the idea of CKEditor adding this an option anyway. But that's a separate issue :)
Comment #12
heddnNo interdiff as this is an entirely new direction. This is the filter. We should consider if we want to add this filter to the default text formats provided by core's install profile.
Comment #13
heddnHere we add the filter to the install profiles.
Comment #14
heddnComment #15
heddnComment #17
Wim Leers#11:
It's not just that. Complexity of markup isn't the problem. Embedded assumptions in the content is the problem.
The content in entities (here it's specifically markup in body fields) should always be as reusable as possible, and definitely not be coupled to (because tuned for) single-observer specifics.
If a content author optimizes the "lazy vs eager" flags for images in his markup, it may be coupled to:
This gets in the way when reusing content in multiple channels (it may be served on multiple websites, consumed through APIs, announcement screens … — IIRC the NYC subway's info screens run on Drupal for example, so this isn't too far-fetched!). IOW: the author should never tweak their content to look perfect on their device. That's the mantra in Drupal.
This tends to be called structured content. I wrote about it a very long time ago: https://wimleers.com/article/drupal-8-structured-content-authoring-exper..., and also wrote about a great podcast episode about this topic: https://wimleers.com/blog/eaton-urbina-structured-intelligent-adaptive-c....
You're right that it still makes sense to add to CKEditor! Many sites use CKEditor to generate the majority of their markup, with no post-processing. And they often do not aim to have reusable content. In that case, that's totally reasonable :)
Comment #18
Wim LeersThorough review:
-1 to adding this to the
plain_text
format 😅 Makes no sense because no HTML is allowed, hence also not images:."),
"Direct" is ambiguous. I think "Instruct" would be clearer?
Needs docblock.
🤩👍 — super elegant, perfect!
Nit: We don't need the leading backslash.
Solid basic test 👍
Missing test where it is verified that this also correctly combined with
media_embed
.Nit: return type hint could be added :)
We generally describe our test cases in data providers. That makes debugging failing tests easier.
If we do that, then we can also get rid of these comments: the case labels become the comments.
No upgrade path concerns here because this is a demo-only install profile. 👍
This should have a higher weight than the
media_embed
filter. Because only then can it also mark media-that-are-images to load lazily.This looks sensible to me. 👍
But … shouldn't we try to get this to work on every Drupal 9 site out-of-the-box? IOW: shouldn't we provide an upgrade path?
At minimum, we should do that for sites who haven't modified the default
basic_html
orfull_html
text formats.Comment #19
heddn18.10 (basic_html): media toggles lazy loading already. Until #3173180: Add UI for 'loading' html attribute to images lands, the default is for all images to lazy load. So the order of the lazy loading vs media embed doesn't matter. After that issue lands, it still won't matter the order. Because the media item will already to lazy load or not.
:thinking: should _that_ issue change to have 3 options? 1) loading="lazy", 2) loading="eager" 3) not specified? Actually, that feels like scope creep for _that_ issue. Another follow-up to add another option to debate that is in order. I can see pros/cons of a not specified option. Regardless, we can move the order of lazy_loading after media embed, just know that it won't really do anything. At least not right away.
:thinking: another thought, what if we put the lazy_loading _before_ media embed so it injects the attribute for lazy loading and media then just takes the values passed to it from the media markup? that seems complicated and scope creep for _this_ issue. I think I'm going to _not_ change anything on filter order to help media embed for now. Too many edge cases.
18.11 (default OOB upgrade path): We forced that on site owners in #3167034: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core. And it was mildly successful. But it also lead to increases in LCP scores. I'm not sure we really want to do that again. Add this feature to release notes and give site owners the option to enable it. They should do it, but then again, maybe they don't want to.
18.6 (embed testing): not added, see long stream of conscience above in response to 18.10.
Attached new patch with test fixes. It also addresses all feedback in #18.
Comment #20
Wim LeersWell … it'd be great if that issue had been linked from the issue summary since day one then. That's crucially important information for reviewing this issue!
Comment #21
Wim LeersHuh, that issue expands on what #3167034: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core already landed in Drupal core … and I'm very surprised that #3167034 managed to modify the
EditorFileReference
filter to also setloading="lazy"
, even though that is 100% unrelated to what it says it does.It also means that the original and current issue summary are extremely misleading. 😔 Because … it is already supported, just in a really confusing way, through a completely unrelated filter, whose description does not mention it at all! The description of this filter does what #3167034 made another filter do! The difference is that this filter works for any/all images, the other filter only works for Drupal-owned and Drupal-uploaded images.
Another consequence: the test coverage here is not remotely sufficient, my concern in #18.6 is just a subset of all the combinations that should be tested now.
I'm even more confused now because that was careful to only set the default of "lazy" if an image has dimensions to avoid CLS (Content Layout Shift), but here there's no mention of that.
Are you suggesting to set
<drupal-media loading="lazy">
— and then have themedia_embed
filter respect that?Sorry, but the implementation details of #3173180: Add UI for 'loading' html attribute to images do not really matter here — we just need to verify that these different filters work as expected when combined. If they are touching the same attribute, there's no way around that, that is essential to guarantee correct behavior.
Because you argued that the filter order should not matter, that also implies even more test coverage is needed: we need to verify that both filters work correctly together regardless of their order.
So … the replies to @xjm's concerns in #3167034-54: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core about disruption were actually inaccurate then? 😨 There was disruption?! But there's nothing in that issue about it. Where can I read more about this?
Comment #22
Wim LeersIMHO the changes made in #3167034: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core that were made in
EditorFileReference
should be reverted from (I think its impact was not yet fully understood) and moved into this new filter. #3167034 made it impossible to not setloading="lazy"
on images, by doing it on a required filter (required if you are using image uploads). It made it have unexpected and undocumented side effects.P.S.: I'm sorry for the pushback 😞 I was eagerly reviewing this because I really want to see this happen in Drupal. 😄 But then I just learned that this sort of already landed in Drupal 9.1 😳 (not explained in the issue summary at all), this is just different (in ways not explained anywhere), in a way that makes it impossible to opt out 👎and introduced regressions 😱. So I find myself in a place where I have to push back, even though I don't want to, but it's crucial for the maintainability (and understandability) of this functionality. 😬
Comment #23
heddnI've been playing with things for the past little bit and thought before I move much further, I should post some notes. Some of what was done in #3167034: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core is starting to come back. Pulling back the layers, not having a managed file it becomes complicated to get the dimensions for images.
The above code works to get a path for locally hosted files that are placed in a standard location without any htaccess rewritting of paths. Meaning, it works because
/sites/default/files/inline-images/foo%20bar%20baz.png
removes the leading slash and becomessites/default/files/inline-images/foo bar baz.png
. But what if the file has special characters or isn't a local image? Or is rewritten via htaccess from sites/default/files to some other location on the server? Then we run into lots of fun when we attempt to gather the dimensions:In the case of remote images, we can get errors like:
Warning: filesize(): stat failed for http://placekitten.com/200/300 in Drupal\Core\Image\Image->__construct() (line 54 of core/lib/Drupal/Core/Image/Image.php).
.What this tells me, is we really don't want to have lazy load images that aren't entity references. Or at least, if we don't want to experience CLS. And we are back to the entity reference filter again. But we should probably untangle lazy loading from that filter. For all the reasons already mentioned. And re-add it to a new lazy load filter for managed files, so we can calculate image dimensions off the managed file.
And maybe we leave a dumb "lazy loading" filter that doesn't do any image dimension logic. And leave a note on that filter that it could lead to CLS if no dimensions are specified.
Comment #24
heddnThis removes lazy loading from
EditorFileReference
. All that logic is added into _this new_ filter.Still remaining:
EditorFileReference
is enabledI've been thinking about it and I'm not really sure I see a need for a dumb text filter for remote and non managed files (in core). These aren't as common and could be handled by a contrib module. Distinguishing when to use it and the possible CLS implications if we can't determine the dimensions are complicated.
Comment #25
heddnComment #27
heddnthis should be green again.
Comment #29
heddnThis responds to my personal review of next steps in #24. All those actionable items are completed in this upload.
Comment #30
heddnUpdated tags, IS and title. Ready for another round of reviews.
Comment #32
heddnComment #33
Fabianx CreditAttribution: Fabianx at Tag1 Consulting for Google commentedBack to RTBC - nice cleanup
Some things:
- Can we add lazing=loady for images that have width and height attributes set, but no loading attribute?
e.g. two ways for automatically make images lazy loading:
- One when they have a data-uuid
- Two when they already have width and height attributes set
That said:
Setting width and height attributes is usually always a good idea, so maybe it might be best to make a follow-up to set width and height always (when available) and then have the filter piggy back from that?
Comment #35
heddnRe-rolled due to #3259174: Add missing CKE5 SmartDefaultSettings test coverage (wildcard tag with unsupported attribute) landing.
Comment #36
Wim LeersSo now this is affecting the CKEditor 5 upgrade path too!
Interesting! 😄
🤞 Is there a CKEditor 5 plugin that supports opting in to this?
🤔 Overall, what do you intend the UX to be? Do you expect users to set this flag in the "source" view in CKEditor 4 and 5? Is this obscure enough that it warrants not being exposed in the UI (
EditorImageDialog
for CKEditor 4, the image balloon/popover toolbar for CKEditor 5)? Did that get approved by the Usability topic maintainers?👏 YES!
We don't add these leading backslashes in Drupal. They're in the global namespace, this is just noise.
Why 15?
Nit: the namespace should be omitted here.
s/plugin_id/plugin ID/
Missing
{@inheritdoc}
Should we have a follow-up to expand this to also happen for embedded
image
Media
entities?An
@see
for this would be nice :)Leading backslash again.
The comment literally says what the method name says … let's delete this.
10 does not match the default weight of 15.
Does that mean that existing sites just checking the checkbox to enable this filter will get the results they expected?
Perhaps we need to add explicit validation for this, just like we did for the
media_embed
filter?Comment #37
heddnThis picks up feedback from 36. Namely, 36.3, 36.5, 36.6, 36.7, 36.10, 36.11.
Items from 36 not needing addressing: 36.2.
36.1 (expected UX in ckeditor):
See findings in #6 - #8
36.4 (why weight 15) &
36.12 (weight mismatch): I don't recall. I guess it is related to attempting to make sure it runs after track uploaded files and not trigger the (new) validation logic just added. There is test coverage of this though, so the weights seems correct.
36.8 (follow-up for image media embed):
media embed items are rendered via media's view mode. its rendering and whether lazy load is added is handled by the configuration on the media. ckeditor has zero impact on that.
36.9 (@see):
See what? I can't tell from context where someone should be directed.
The interdiff is a little noisy as I've moved some hunks around from filter to editor module to better align functionality. The main changes are the nit fixes and changes to editor.module. All feedback in #36 are addressed.
Comment #38
heddnOops, And here are the missing files.
Comment #40
heddnComment #42
heddnComment #43
Fabianx CreditAttribution: Fabianx at Tag1 Consulting for Google commentedApproved, changes look good back to RTBC for another round of maintainer reviews! :)
Comment #45
heddnDue to this getting mixed up into the ckeditor5 upgrade path, we get to keep enhancing test coverage as more of ckeditor5 gets converted. That's what this latest re-roll is about.
Comment #46
Fabianx CreditAttribution: Fabianx at Tag1 Consulting for Google commentedChanges look good, back to RTBC
Comment #47
heddnNeeded a re-roll. Diff attached for the re-roll. Mainly we are still chasing CKEditor5 test changes.
Comment #48
Fabianx CreditAttribution: Fabianx at Tag1 Consulting for Google commentedBack to RTBC
Comment #49
Wim LeersI'd like the opportunity to sign off on this again. Will try to review by the end of the week. This has not had a green for well over a month, so fortunately you've not been blocked on me :)
Comment #50
heddnCan we leave this in the RTBC queue so re-tests get triggered while reviewing this?
Comment #51
yogeshmpawarRerolled patch & added reroll diff for reviewers.
Comment #53
heddnComment #54
Fabianx CreditAttribution: Fabianx at Tag1 Consulting for Google commentedBack to RTBC, leaving assigned to Wim
Adding tag for subsystem maintainer review (that Wim will perform)
Comment #60
heddnAnd back to RTBC with same caveats as #54. Note, there are 2 branches here to support 9.4 and 10.0.
Comment #61
Wim LeersNit: let's keep the order the same.
loading
was the third attribute (aftersrc
andalt
), no need to change that.🤓 Indentation issue.
🐛 This comment does not quite make sense I think? I think it meant to say
editor_image_lazy_load
andeditor_file_reference
?🐛 Isn't it
editor_image_lazy_load
?🤔 Why is the post-update hook in the
editor
module if the new filter is provided by thefilter
module?🤔 Nothing else in Drupal core uses a leading backslash before
assert()
.🤔 Why is the filter called
editor_image_lazy_load
if it's provided by thefilter
module?And … why then not just call it
image_lazy_load
?🤔 This
editor
prefix definitely does not make sense.🤔 This filter is completely unrelated to a text editor? So why is it then labeled as such?
Either it should not mention that, or it should be moved into the
editor
module. Right now it's very confusing.🐛 Name mismatch.
🐛 Extraneous empty line.
🤓 Not exit, but return.
🐛 Incomplete documentation.
🙏 Let's add an
@see
to explain what "CLS" is.That's not what this is actually testing.
🤔 This name is also not conveying what it's actually testing.
🐛 Apparently this used to be in the
filter
module and now it's in theeditor
module. Out of sync.🤔 That's not what this tests.
🤔 Why 10 and not also 15?
Comment #62
Wim LeersThe issue summary could also use an update, to make this actually reasonable to review and commit.
This definitely also needs a change record and a release note. Because this will be a high-impact change! 😄
Comment #63
catchI haven't been following this particular issue very closely, but I just committed #3173180: Add UI for 'loading' html attribute to images and also keeping an eye on #3245720: [drupalMedia] Support choosing a view mode for <drupal-media> and related issues.
<drupal-media>
is embedding media (which might contain an image, but isn't just the image), with a defined view mode.The media display mode configuration will include whether the image is rendered lazily or not (responsive image formatter still needs #2947796: Responsive image format for media).
If we do this issue and the others, but no specific follow-up, then it looks like this:
- If you add an image directly in the editor, you either specify the loading attribute, or this filter makes it lazy if nothing is specified.
- If you add an image via media embed, the media view display configuration determines the loading attribute
The responsibility for the loading attribute (and everything else about how the embedded media is displayed) is therefore on the media display configuration. This means that care needs to be taken about where any particular media view mode is likely to be used, since it may or may not appear above or below the fold, but that is the case for non-editor media references too.
There seem to be a couple of other options:
1. Make this filter apply the same change to images rendered via media embed too - however since every image rendered via media view mode will have a loading attribute of either
lazy
oreager
, it would be a no-op.2. Add configuration on media embed to specify the loading attribute
<drupal-media loading="lazy">
, as mentioned in #3247795-21: Add text filter plugin to support <img loading="lazy"> and remove it from editor_file_reference, this would have to rewrite the rendered media entity HTML to override the attribute.3. Take an approach closer to #2947796: Responsive image format for media: allow the media reference to show media as a 'thumbnail' instead of with a view mode, and then make the image style/responsive image style and loading attribute configurable directly in the editor
<media-embed thumbnail responsive_image_style="wide">
or something like that. This would have to be an option in the media embed widget in that case, although that allows the loading attribute to be unspecified, then this filter could then kick in on those images.Option #2. seems possible, but counter-intuitive to have the rendered entity HTML rewritten by the filter.
Option #3 seems like it might be viable, although it adds a lot of options and complexity to the media embed widget.
Comment #64
Wim Leers#3173180: Add UI for 'loading' html attribute to images made it possible to configure it site-wide (for a formatter of a specific field, for all entities containing that field), not per use (i.e. in the widget to configure it per entity).
Which indeed leads to
I already wrote concerns about option 3 elsewhere, quoting the relevant bits here:
Option 2 would be very different if #3173180: Add UI for 'loading' html attribute to images had made it possible to choose the laziness per entity/use. IMHO that is the logical next step. Then what I wrote above in response to option 3 would become trivial to do: expand what
\Drupal\media\Plugin\Filter\MediaEmbed::applyPerEmbedMediaOverrides()
does to allow overriding not justalt
andtitle
but alsoloading
.Curious what you think 🤓
Comment #65
catchI don't think we should allow configuration in the field widget (i.e. per-entity), seems like the wrong place.
Let's say I have an article content type, with a media image field, it's shown in the following situations:
1. A hero image on the front page of my site (because it's in a layout via views/entityqueue, or some kind of entity reference block thing).
2. A wide image at the top when the article is shown at node/n just under the title.
3. A card at the bottom of a different article as 'related content'
4. Additionally, the media image gets re-used in a different article via ckeditor.
Per entity doesn't help here at all, it makes things less flexible than view mode, because at least the node view display configuration can handle different media view display configurations.
I could see layout and views (and paragraphs) potentially providing some kind of override mechanism (per-use), but that doesn't play well with render caching of the view mode, and not sure we want to get into layout and views doing DOM-based post-processing of HTML.
There's also an existing usability issue open for the media override logic: #3084011: The source of alt text in embedded image media is not clear.
The idea with 'thumbnail' or if not thumbnail then some kind of 'directly render the image without using a view mode' option was to more cleanly separate the two cases, so that it's not actually overriding anything but just a different way of doing it. The issue with that though is it doesn't provide you the option of 'view mode + tweaks' which the current logic does, but it's closer to the views 'fields' mode (not that I'm a fan of that one though outside tables, but again it's clear what's happening).
Comment #66
heddnAdditional point for media embed, for videos that are loaded in an iframe and oembed, we have #3212351: Lazy load OEmbed formatter. Which also would get the lazy load treatment based on the view mode configuration.
Comment #67
heddnHere's the start of fixes from the feedback over the past few days. But first 1000+ thanks for posting a review. I really appreciate it.
editor_file_reference
. The filter is in editor module.Why is the filter added to editor module?
Because it depends on file module. Filter doesn't have that dependency. But editor does (added in 2013, see #2119775: EditorImageDialog assumes File module is enabled: add dependency on File module). All the same reasons that
editor_file_reference
are in editor module apply here. This issue just pulls the bits that were doing lazy loading out of that filter and moves them into a dedicated purposed filter.Comment #68
heddnComment #70
heddnTo make it easier to review, I've closed the 10.x branch. Too hard to keep things in sync and review different MRs. If we need a distinct MR for 10.x, we can make it later.
Comment #73
heddnMy promised response to #61.1:
Media entities embedded in ckeditor have a view mode with a configured lazy/eager loading attribute defined in that view mode. I consider that to be the first phase of adoption for configuring the loading attribute. In the short term, folks can apply the work from #3245720: [drupalMedia] Support choosing a view mode for <drupal-media> and fork their view modes to have a lazy and eager option. And we can learn some things. We don't have to know everything before we adopt the first phase. If we need to add more phases to override media embed values (captions, loading, view mode, etc), that can all be done in a follow-up that takes the entire body of knowledge that is present at that time and adopts a reasonable path forward. So I guess I push back on us needing to fully fleshed out plan for media embed. Many other things have to line up and it is hard to predict the future.
Comment #74
Graber CreditAttribution: Graber at Tag1 Consulting for American Federation of Teachers commentedTried to test this on demo_umami profile and all images added in wysiwyg don't have the height, width or loading attributte. Those are responsive images with srcset. I think it's worth to support those as well.
Comment #75
heddnResponsive images from embedded media are going to be handled by the view mode selected for the media. It isn't in scope of this issue to solve that. Rather, that is in #3192234: Apply width and height attributes to allow responsive image tag use loading="lazy". Back to NR.
Comment #76
Graber CreditAttribution: Graber at Tag1 Consulting for Google commentedGood :+1:
Comment #78
heddnSwitching issue version as well as a rebase of MR.
Comment #79
lauriiiIt looks like the CI is failing.
Is there a specific reason for restricting the filter for tracked images? Wouldn't it be possible to enable that across all images now that it's in a separate filter?
Comment #80
lauriiiResponding to myself; of course because we couldn't retrieve the dimensions for other images. Marking NW for addressing the CI failures.
Comment #82
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedI've tried to rebase the current MR as it was not mergeable, please review.
Comment #83
heddnBack to RTBC after updating the fixture path from 9.3 => 9.4.
Comment #84
edurenye CreditAttribution: edurenye at Digitalist commentedMR is not mergeable.
Comment #85
heddnBack to RTBC. It didn't need a rebase. I did refresh the MR with 10.1.x, but there were no conflicts.
Comment #86
alexpottAdded a code review in gitlab. Everything seems to be in place. A couple of nits and some missing test coverage.
Comment #87
heddnAll feedback from MR (hopefully) addressed.
Comment #89
Graber CreditAttribution: Graber at Tag1 Consulting for Google commentedA few things to address 👆
Comment #90
heddnThe deprecation using DeprecatedServicePropertyTrait seems to be a topic of question. There doesn't seem to be existing test coverage for it. Not sure the right path forward on it.
Comment #91
alexpott@heddn re #90 I think what you've done is fine. We should file a follow-up to discuss how to improve this - but it should not block this issue.
Comment #92
heddnOpened #3315816: Improve deprecated service trait as a follow-up. I've requeued tests as I am pretty sure the js test failures are randoms.
Comment #93
Graber CreditAttribution: Graber at Tag1 Consulting for Google commentedTests passed - random test fails in core are a different issue, the solution here is ok. Back to RTBC.
Comment #94
alexpottUpdating issue credit to credit constructive and detailed comments plus code review.
Comment #95
Wim LeersLeft a review on the merge request.
High-level observations:
#3247795
(this issue). Not great 😔Comment #96
heddnI opened #3319514: Handle overriding loading attribute in embedded media to handle ckeditor loading attribute overrides.
Comment #97
heddnComment #98
Wim LeersI assume you marked this as needing review because you addressed all feedback? I can't know for sure because due to the current d.o GitLab instance infra problems, it's impossible to see commits until >10 hours (maybe even >48) after they've been pushed 😅
In any case, the one thing you pushed back on means there's still at least one bug present, so marking
for that.Comment #99
heddnComment #100
Wim LeersComment #101
Graber CreditAttribution: Graber at Tag1 Consulting for Google commentedSwitching to RTBC as this needs a maintainer response / decision on one unresolved thread.
Comment #102
Wim LeersSorry, I think this is still not in a consistent state: https://git.drupalcode.org/project/drupal/-/merge_requests/2096#note_130460 😅
Comment #103
heddnAdded some comments to the MR. This is a tough space. Not sure that either option is ideal. But I've proposed to keep lazy_load in editor. And move dimension setting into
editor_file_reference
.Comment #104
heddnDiscussed this with Marcin and Nat today and the tentative plan is to move dimensions into
editor_file_reference
and movelazy_load
into filter.module. The requirements check will disappear and we'll add description wording similar tohtml_corrector
thatlazy_load
filter should execute after all other image filters. We can leave it to #3314972: Filter plugin requirements handling to handle warning messaging in a more clear way.Comment #105
heddnComment #106
Graber CreditAttribution: Graber at Tag1 Consulting for Google commentedA few nits.
Comment #107
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedResolved a few threads on MR, still needs work for the remaining threads.
Comment #108
heddnSorry, this should be NR.
Comment #109
Graber CreditAttribution: Graber at Tag1 Consulting for Google commentedAll addressed, thanks!
Comment #110
alexpottCommitted a266ba6 and pushed to 10.1.x. Thanks!
Comment #112
heddnThank you everyone for all your assistance in getting this to land!
Comment #113
Wim Leers👏
The end result looks great!