WordPress.org

Make WordPress Core

Opened 17 months ago

Closed 17 months ago

Last modified 17 months ago

#22693 closed defect (bug) (fixed)

Inserted images in some cases get the wrong size

Reported by: jond3r Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch commit
Focuses: Cc:

Description

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.

Attachments (7)

22693.diff (623 bytes) - added by nacin 17 months ago.
22693.2.diff (1.7 KB) - added by koopersmith 17 months ago.
22693.3.diff (1.9 KB) - added by cdog 17 months ago.
22693.jpg (56.9 KB) - added by cdog 17 months ago.
22693.4.diff (499 bytes) - added by dd32 17 months ago.
22693.5.diff (643 bytes) - added by cdog 17 months ago.
22693.6.diff (545 bytes) - added by koopersmith 17 months ago.

Download all attachments as: .zip

Change History (32)

comment:1 koopersmith17 months ago

  • Keywords needs-patch needs-body added
  • Milestone changed from Awaiting Review to 3.5

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.

nacin17 months ago

koopersmith17 months ago

comment:2 follow-up: koopersmith17 months ago

  • Keywords has-patch needs-testing added; needs-patch needs-body removed

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.

cdog17 months ago

comment:3 in reply to: ↑ 2 ; follow-up: cdog17 months ago

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.

comment:4 in reply to: ↑ 3 ; follow-up: nacin17 months ago

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.

comment:5 nacin17 months ago

  • Keywords commit added; needs-testing removed

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.

comment:6 follow-up: nacin17 months ago

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

comment:7 ryan17 months ago

I had the same findings as nacin. Looks good.

comment:8 nacin17 months ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

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.

comment:9 in reply to: ↑ 6 ; follow-up: jond3r17 months ago

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 :-)).

comment:10 in reply to: ↑ 4 cdog17 months ago

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.

comment:11 follow-ups: dd3217 months ago

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.

comment:12 in reply to: ↑ 11 dd3217 months ago

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.

cdog17 months ago

comment:13 in reply to: ↑ 11 cdog17 months ago

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.

dd3217 months ago

comment:14 follow-up: dd3217 months ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:15 in reply to: ↑ 14 cdog17 months ago

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.

comment:16 dd3217 months ago

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 :)

cdog17 months ago

comment:17 cdog17 months ago

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 it's 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'.

Version 4, edited 17 months ago by cdog (previous) (next) (diff)

comment:18 cdog17 months ago

  • Cc catalin.dogaru@… added

comment:19 follow-up: koopersmith17 months ago

  • Keywords commit added

comment:20 in reply to: ↑ 9 jond3r17 months ago

Replying to jond3r:

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

This resulted in #22738.

comment:21 ryan17 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 23033:

Better dropdown select handling.

Props cdog, koopersmith
fixes #22693

koopersmith17 months ago

comment:22 in reply to: ↑ 19 ; follow-up: koopersmith17 months ago

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

comment:23 koopersmith17 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:24 nacin17 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 23037:

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

comment:25 in reply to: ↑ 22 cdog17 months ago

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.

Note: See TracTickets for help on using tickets.