Make WordPress Core

Opened 10 years ago

Closed 9 months ago

Last modified 4 months ago

#26381 closed defect (bug) (fixed)

Scaling Image Up Fails Silently

Reported by: brookedot's profile brookedot Owned by: joedolson's profile joedolson
Milestone: 6.3 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch needs-testing has-unit-tests 2nd-opinion
Focuses: Cc:

Description (last modified by SergeyBiryukov)

The ability to scale images up was removed (I think in 3.5) and the text was changed in r24562 per this ticket #23713.

However, when using the Image editor to scale an image if you enter a value larger than the image we still attempt to re-size the image and the "Restore Original link appears"

To Reproduce

1) Select an image from the media library and click the "Edit Image" button
https://i.cloudup.com/FI6vrPykAw-3000x3000.png

2) Enter dimensions larger than the original dimensions. You will see a red "!" as a warning but ignore this and click the "Scale" button
https://i.cloudup.com/00DyBO7Rb5-3000x3000.png

3) The site will try to re-size the image and gives a success message.
https://i.cloudup.com/VLRLKcN8N5-3000x3000.png

The only problem is that no new image was created. To fix this we either need to give a warning and not produce a "Restore Original Image" link or prevent the user from clicking the link at all.

My vote is since we already have JavaScript that detects when the image is too large the simplest solution would be to disable the "Scale" button when the warning "!" shows.

Attachments (4)

26381.diff (867 bytes) - added by desrosj 19 months ago.
26381.2.diff (2.6 KB) - added by desrosj 19 months ago.
26381.3.diff (3.9 KB) - added by Mista-Flo 13 months ago.
Adding some unit test
26381.4.diff (6.7 KB) - added by joedolson 9 months ago.
Adds accessibility improvements

Download all attachments as: .zip

Change History (44)

#1 @bandonrandon
10 years ago

#26382 was marked as a duplicate.

#2 in reply to: ↑ description @bandonrandon
10 years ago

related: #23713

#3 @SergeyBiryukov
10 years ago

  • Description modified (diff)
  • Version changed from trunk to 3.5

#4 @markoheijnen
10 years ago

It wasn't remove in 3.5. I think it was never really possible. The code changes made in 3.5 had nothing to do with scaling restrictions.

#6 @wonderboymusic
9 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to markoheijnen
  • Status changed from new to assigned

markoheijnen - please take a look again and see if this needs to be dealt with

#7 @kirasong
9 years ago

Seems like it might make sense to disable the Scale button if we know we can't resize. We're already showing an indicator, but there's no reason to allow a user to attempt to resize if it's just going to fail.

#8 @markoheijnen
9 years ago

Another option is to revert the given size to the original size when it's to big.

#9 @kirasong
9 years ago

Yeah, just changing the field to match the current dimension (and hence max resize value) when a user attempts to set it higher would make sense.

#10 @chriscct7
8 years ago

  • Keywords needs-patch added

@desrosj
19 months ago

#11 @desrosj
19 months ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Milestone set to 6.1

I did some testing and this is still present today in 6.0.x.

26381.diff disables the scale button when the entered dimensions would result in upscaling. But it feels confusing not to present more information to the user.

Additionally, I think that wp_save_image() (which is the function called for scale AJAX requests) could probably be updated to return an error when attempting to enlarge an image. I'll upload a separate patch that includes that change, but this may cause some problems in other locations, so may need more testing.

This ticket was mentioned in PR #3054 on WordPress/wordpress-develop by desrosj.


19 months ago
#12

This adjusts the logic for the edit image screen in the admin to prevent an image from being scaled larger than the original.

Trac ticket: https://core.trac.wordpress.org/ticket/26381.

@desrosj
19 months ago

#13 @desrosj
19 months ago

  • Owner markoheijnen deleted

26381.2.diff is just the patch from the pull request I've created above.

I'm also going to reset the owner field on this ticket. It's been 8+ years since this one has received attention. Sometimes contributors shy away from tackling tickets with an owner. This will make it more clear that no one is actively taking lead on this one.

@markoheijnen if you are still interested in this, feel free to re-assign.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


19 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


17 months ago

#16 @antpb
17 months ago

  • Milestone changed from 6.1 to 6.2

With consideration to the feedback needed around Jonathan's points about the confusing experience of seeing the button disable without context, it might be better to move this issue to the next release of WordPress. Beta 1 is hours away so it would be better to solve the UX issues here before we merge any changes.

@Mista-Flo
13 months ago

Adding some unit test

#17 @Mista-Flo
13 months ago

  • Keywords has-unit-tests added

Hey there, just coming back to this ticket to help making it on time for 6.2.

I took @desrosj last patch and added a dedicated unit test function te test the new behaviour, working fine on my end.

Also tested the functionality in the backend, that also works fine to me.

#18 @brookedot
13 months ago

This is just an administrative note that since I reported this 9 years ago I now use the W.org username of brookedot not bandonrandon so if props are given to the reporter please give them to me as brookedot. Thanks!

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


13 months ago

#20 @antpb
13 months ago

  • Owner set to antpb
  • Reporter changed from bandonrandon to brookedot

This ticket was mentioned in Slack in #core by costdev. View the logs.


12 months ago

#22 @azaozz
12 months ago

  • Keywords close 2nd-opinion added

Just a note here that the old image editor is considered "in maintenance mode only" just like the old post editor. There is better alternative in the Image block.

For now the (new) image editor in the Image block lacks some features, but has much better UI and works quite better code-wise. For example it creates new attachment posts for edited images which removes many of the problems and edge cases when using the old editor.

In that terms not sure if this is worth fixing after 9 years...

Last edited 12 months ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


12 months ago

#24 @costdev
12 months ago

The discussion in Slack showed some disagreement with closing this ticket, particularly as the image editor in question is in maintenance mode only, and this is something to be fixed.

As this is still failing silently, and we have patches and a test, the work is pretty much already done here. If this didn't have a patch, I might have agreed with closing this ticket, but I think it's worth finalizing this work with final reviews and commit.

While the test in 26381.3.diff (which is newer than the other patches and PR) needs a little work to meet standards, this whole test file needs the same improvements. IMO, the test could be committed as-is and we could follow up to fix this whole file in one go later on.

Last edited 12 months ago by costdev (previous) (diff)

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


12 months ago

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


12 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


12 months ago

#28 @antpb
12 months ago

  • Milestone changed from 6.2 to 6.3

It was discussed and agreed in recent a recent Media meeting that this fix is desireable even if in maintenance mode as this is essentially a bug that needs to be fixed. This will be top of my list in 6.3 to finish up. We just need to get the coding standards cleared and it is good to go.

#29 @kirasong
9 months ago

Just a quick note that I agree that it makes sense to fix this -- especially since it's already got a patch.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


9 months ago

#31 @joedolson
9 months ago

  • Keywords close removed
  • Owner changed from antpb to joedolson
  • Status changed from assigned to accepted

This ticket was mentioned in PR #4468 on WordPress/wordpress-develop by @joedolson.


9 months ago
#32

Tests on 26381.4.diff

Trac ticket: https://core.trac.wordpress.org/ticket/26381

#33 @joedolson
9 months ago

PR includes the above, plus:

  • Change input to type=Number and added min/max values to set guardrails
  • Add onchange so input type's arrows trigger events
  • Made scale warning visible with JS warning text, adjusted contrast to meet WCAG guidelines
  • Associated warning with input fields using aria-describedby

@joedolson
9 months ago

Adds accessibility improvements

This ticket was mentioned in Slack in #accessibility by joesimpsonjr. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-media by joedolson. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-media by joedolson. View the logs.


9 months ago

This ticket was mentioned in Slack in #core by joedolson. View the logs.


9 months ago

#38 @antpb
9 months ago

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

In 55859:

Media: Prevent scaling up of images in the Image Editor.

Previously, when scaling an image larger than the source size in the image edit states the image would silently fail the scaling action. This patch provides an error when someone attempts to scale an image larger than the source size while also disabling the button to initiate the action.

Props brookedot, joedolson, markoheijnen, mikeschroder, desrosj, Mista-Flo, costdev.
Fixes #26381.

Note: See TracTickets for help on using tickets.