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
The functionality that replaces an <img> element in a submitted comment when it is malformed is a bit buggy.
What happens is that the original <img> tag that the user typed in their comment is replaced by the parser with an "error-not-found" image.
The problem is that newly inserted image still uses the width and height attributes that were attached to the original image. On inspecting the error-image's properties, I see that it's a totally new image - this is indicated by the fact that the alt attribute says, "Only local images are allowed." when the original code I'd typed was empty.
Please view the attached screenshot: buggy_non-local-image_error-placeholder.png.
Proposed resolution
It would be great if the parser (be it a text-filter or whatever) didn't reuse the width and height properties from the original post's image and instead used the real properties of the error-placeholder image. See before and after code below:
Before
<img src="/misc/watchdog-error.png" width="933" height="681" alt="Only local images are allowed." title="Only local images are allowed.">
After
<img src="/misc/watchdog-error.png" width="18" height="18" alt="Only local images are allowed." title="Only local images are allowed.">
Remaining tasks
| Task | Novice task? | Contributor instructions | Complete? |
|---|---|---|---|
| Update the issue summary | Instructions | Completed in #6 | |
| Add automated tests | Instructions | #11 | |
| Manually test the patch | Novice | Instructions | Completed in #9 |
| Embed before and after screenshots in the issue summary | Novice | Instructions | Completed in #9 |
| Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions | Completed in #9/#12 |
User interface changes
The inserted error image will be its natural size.
API changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | filter_html_image_secure-2125769-height-width.patch | 1.42 KB | Steven Jones |
| #11 | width-height-2125769-11.patch | 1.67 KB | c31ck |
| #9 | after.png | 41.21 KB | mErilainen |
| #9 | before.png | 64.76 KB | mErilainen |
| buggy_non-local-image_error-placeholder.png | 357.53 KB | anandkp | |
Comments
Comment #1
anandkp CreditAttribution: anandkp commentedComment #2
drummComment #3
drummThis issue should be tackled in core, then backported.
Comment #4
Wim LeersComment #5
c31ck CreditAttribution: c31ck commentedWrote a patch that determines the width and height of the error image and sets them.
Comment #6
Steven Jones CreditAttribution: Steven Jones commentedComment #7
Wim Leers@c31ck: thanks! Looking good :)
Please also update
FilterHtmlImageSecureTest.Extraneous newline.
Comment #8
c31ck CreditAttribution: c31ck commentedComment #9
mErilainen CreditAttribution: mErilainen commentedRemoved empty line.
Added before and after images.
Updated issue queue.
Maybe c31ck is working on the tests?
Comment #10
c31ck CreditAttribution: c31ck commentedHi mErilainen, yes I'll work on the tests sometime this week.
Comment #11
c31ck CreditAttribution: c31ck commentedChanged the test so that the width and height are checked as well.
Comment #12
Wim LeersLooks good, thanks!
Comment #13
Steven Jones CreditAttribution: Steven Jones commentedComment #15
alexpottCommitted 2d4cdae and pushed to 8.x. Thanks!
Comment #17
tvn CreditAttribution: tvn commentedThis should be backported now to D7?
Comment #18
drummYes.
Comment #19
Steven Jones CreditAttribution: Steven Jones commentedHere's a patch for the D7 version of the module.
Comment #21
sunThanks, committed (for now).
However, technically, this is an API change for the D7 module, since existing alter hooks for the placeholder image will need to be adjusted.
I wonder why we went through the extra processing logic of determining the width/height of the placeholder image, instead of simply removing the width/height attributes?
I also don't see that option in the discussion above?
Comment #22
Wim LeersPerhaps it was not considered because images should always have
widthandheightattributes specified, because if they're missing, then that may cause content to be rerendered (and hence it may "jump") once the image is downloaded.Comment #23
millerbennett CreditAttribution: millerbennett commentedWhile width and height are not required according to spec, I do agree with Wim- not supplying image dimensions causes browsers to re-flow the content around the image which is quite an expensive operation and also causes a visual jump. I believe we can mark this as fixed for now.