Media filter sometimes sets incorrect image widths [#841116]

This code in _media_markup() is already problematic, as described in the TODO:

    // @TODO: What case does this provide for?  Can we add this logic in JS when we embed it?
    // This doesn't look great to me.  Also won't work if the style has anything
    // between width and height (or if they are in reverse order).
    if (isset($settings['style'])) {
      if (preg_match('@width: (.+?)px; height: (.+?)px;@i', $settings['style'], $matches)) {
        $settings['width'] = $matches[1];
        $settings['height'] = $matches[2];
      }
    }

However, it's even more buggy than that :) For example, if you set the width and height of an image in CKEditor and also have a border on that image, the style attribute you get looks like this:

"border-style: solid; border-width: 8px; height: 656px; width: 315px;"

So not only are the width and height backwards from what the above regular expression is looking for (an issue raised in the TODO), but also, the "width" part of "border-width" actually matches the regular expression, so you wind up with the image erroneously set to 8 pixels wide...

Comments

Comment #1

JacobSingh CreditAttribution: JacobSingh commented 16 February 2011 at 21:24

Status:

Active

» Closed (cannot reproduce)

I believe this is outdated.

Comment #2

David_Rothstein CreditAttribution: David_Rothstein commented 16 February 2011 at 21:55

Status:

Closed (cannot reproduce)

» Active

No, the bug is still easy to reproduce. Although trying it again with CKEditor, the bug has changed a bit. The style attribute I got was this:

"height: 239px; width: 120px; border-width: 8px; border-style: solid;"

For whatever reason, the order has switched. So it no longer matches the regular expression incorrectly; it now doesn't match it at all :) Which means that as described in the TODO, the width and height set by CKEditor don't get noticed and it simply fails to respect them - your image remains the same size.

Comment #3

David_Rothstein CreditAttribution: David_Rothstein commented 16 February 2011 at 22:07

Actually, shouldn't #841104: File styles suppress most image attributes have removed the need for all this - i.e., shouldn't $settings['style'] be respected directly, without having to parse and pull things out of it?

I can't remember how all that worked now and the interaction between Media and Styles, but after that issue I think it used to be possible to set style attributes via right-clicking on an image in CKEditor and have the media filter respect those, but now it doesn't seem to be working anymore.

Comment #4

David_Rothstein CreditAttribution: David_Rothstein commented 24 March 2011 at 13:55

The patch at #1043570: Floating images left and right likely fixes this, although I'm holding off on marking this a duplicate for now.

Also, the regression with the Styles stuff I mentioned above (where setting style attributes via right-clicking an image in CKEditor used to work but doesn't anymore) is basically being handled here: #1094016: Allow Wysiwyg styles

Comment #5

JacobSingh CreditAttribution: JacobSingh commented 11 April 2011 at 06:30

Priority:

Normal

» Major

I feel like we should consolidate the related issues here, but since @effulgentsia is looking into the styles / media confusion in a more holistic way, I don't know where to right now. Still, marking as major since it sucks.

Comment #6

effulgentsia CreditAttribution: effulgentsia commented 10 May 2011 at 04:08

Status:

Active

» Fixed

Committed this: http://drupalcode.org/project/media.git/commitdiff/22a9e88?hp=e153e4b294.... Sorry, broke protocol and didn't submit it for review first, but I feel pretty confident about it. Feel free to re-open this issue with a critique if necessary.

#1094016: Allow Wysiwyg styles remains a separate issue.

I'm not yet sure about #1043570: Floating images left and right, and if anything useful remains there once #1094016: Allow Wysiwyg styles is fixed. In any case, let's follow up on that one there.