Inserted images in some cases get the wrong size

[jond3r]

This is a bug I have not seen reported yet, and I think is of some severity.

It manifests itself for smaller images that don't have all the standard sizes (thumbnail, medium and large),
and is dependent on the user's previous choice of image size (which I believe is stored in a cookie).

Here's a scenario:

• The user's latest inserted image was medium sized.
• Next he selects an image that is around 200x200 px, so that it has a thumbnail, and a full size, but no medium size.
• In the size drop-down box, is shown "Thumbnail". The user doesn't change this, and presses "Insert into post"
• The result is that the full-size image is inserted, and the image class gets a value of 'size-medium', i.e. two wrongs.

You can come up with several similar scenarios, involving different image sizes and previous choices.

What I think happens is that if the size value in the drop-down box is not changed actively by the user, the previous value is reported back to the server. A value that might not be valid for the current image, and is also not shown in the UI.

This might need some JavaScript programming to fix. I'm not sufficiently familiar with the code in question, so no patch is provided.
Sorry for the late submission.

[koopersmith]

Haven't investigated, but this sounds valid. Fixing might be as simple as defining selected on the full size in the settings template, since it will always exist.

[koopersmith]

Patch defaults the size dropdown to the full size, and the JS now checks to see if the dropdown value it's looking for is actually present in the modal.

[cdog]

Replying to koopersmith:

Patch defaults the size dropdown to the full size, and the JS now checks to see if the dropdown value it's looking for is actually present in the modal.

Tested the patch and seems to work well. Just a small suggestion. When handling dropdowns you should also clear the current selection and set only the needed value.

[nacin]

Replying to cdog:

Tested the patch and seems to work well. Just a small suggestion. When handling dropdowns you should also clear the current selection and set only the needed value.

I don't think that's how dropdowns work. This isn't a multiple-selection dropdown.

I will say that this should be .prop() rather than .attr(), presumably.

[nacin]

I did this:

• Inserted a "Medium" image.
• Insert an image that is too small for Medium, but large enough to have a thumbnail. (I used an image that was 138px width x 197px height.)

Without the patch:

• "Thumbnail" is selected, size-medium is the class, and the full-size image was inserted.

With 22693.2.diff​:

• "Full" is selected, size-full is the class, and the full-size image was inserted.

[nacin]

jond3r: By the way, this is an excellent bug report. Thank you!

[ryan]

I had the same findings as nacin. Looks good.

[nacin]

In 23008:

Media: When an image does not have all image sizes available, make sure we insert the requested size into the editor. props koopersmith. fixes #22693.

[jond3r]

Replying to nacin:

Thanks for the "credit".

It looks good to me too. Glad it didn't take too much trouble to fix.

However... I did test a few other cases and think that I have found a related but slightly different bug. It is less severe, and will show up in fewer cases. I will investigate further and possibly file a new bug report later today (I live in another time zone you know :-)).

[cdog]

Replying to nacin:

I don't think that's how dropdowns work. This isn't a multiple-selection dropdown.

However, selecting an option then another will add the 'selected' attribute to each of it.

<select>
	<option selected="selected" value="thumbnail">
		Thumbnail
	</option>
	<option selected="selected" value="medium">
		Medium
	</option>
	<option value="large">
		Large
	</option>
	<option selected="selected" value="full">
		Full Size
	</option>
</select>

Then $setting.find('[selected]').val() is used. It will return more than just one result.

[dd32]

However, selecting an option then another will add the 'selected' attribute to each of it.

Browsers only allow one <option> to be selected within a <select> (Unless the multiple attribute is set), Selecting one option will cause all others to be deselected.

[dd32]

Replying to dd32:

However, selecting an option then another will add the 'selected' attribute to each of it.

Browsers only allow one <option> to be selected within a <select> (Unless the multiple attribute is set), Selecting one option will cause all others to be deselected.

Now that I've said that, and actually tested it.. I see what you mean.
The DOM tree shown by an inspector shows the selected attribute on each element, This could be due to the usage of $value.attr() in the commit rather than $value.prop(), but regardless, jQuery( ..element.. ).val() always returns a single string of the actual selected element in my test.

[cdog]

Replying to dd32:

Browsers only allow one <option> to be selected within a <select> (Unless the multiple attribute is set), Selecting one option will cause all others to be deselected.

I'm inspecting the DOM with Firebug and it's not really what I see: attachment:22693.jpg​.

It does work in this particular case because the selected value is also stored in a variable and the dropdown is refreshed when selecting another image causing only the default value to be selected.

But not handling it how it should may cause unexpected results.

[dd32]

I'm inspecting the DOM with Firebug and it's not really what I see

Right, I jumped the gun, I understand why browsers do it that way, but it's the first time i'd ever run into this case (and I'm no JS god :) ).

22693.4.diff​ handles the case for me, and .find('[selected]').length is no longer > 1, but it's not actually extensively tested other than on the attributes and JS return values of that <select> being right.

re-opening purely for koop/nacin to weigh in on it.

[cdog]

Replying to dd32:

22693.4.diff​ handles the case for me

It should do the trick. In fact, it's exactly what I've suggested in attachment:22693.3.diff​.

[dd32]

It should do the trick. In fact, it's exactly what I've suggested in attachment:22693.3.diff​​.

This is proof that nacin/koop operate better than I do with lack of sleep :)

[cdog]

The 'selected' attribute, when present, specifies that an option should be pre-selected when the page loads.

What we do is to simulate the 'selected' property using this attribute (which is not its purpose).

A better approach would be to handle dropdowns using ​.select() to trigger the event on the required element and ​:selected selector to match the selected element as suggested in attachment:22693.5.diff​. This way it's also possible to let the default value set to 'medium'.

[koopersmith]

attachment:22693.5.diff​ is solid. +1.

[jond3r]

Replying to jond3r:

I will investigate further and possibly file a new bug report later today

This resulted in #22738.

[ryan]

In 23033:

Better dropdown select handling.

Props cdog, koopersmith
fixes #22693

[koopersmith]

I lied, that patch had a bug. The .select() method only triggers the event, it doesn't update the property.

[nacin]

In 23037:

Media: Properly select the select element's option in the attachment display settings. props koopersmith. fixes #22693.

[cdog]

Replying to koopersmith:

I lied, that patch had a bug. The .select() method only triggers the event, it doesn't update the property.

Sorry for that. Thanks for fixing it.