#26381 closed defect (bug) (fixed)
Scaling Image Up Fails Silently
Reported by: | brookedot | Owned by: | 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 )
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
2) Enter dimensions larger than the original dimensions. You will see a red "!" as a warning but ignore this and click the "Scale" button
3) The site will try to re-size the image and gives a success message.
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)
Change History (44)
#2
in reply to:
↑ description
@
11 years ago
related: #23713
#4
@
11 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
@
10 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
@
10 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.
#9
@
10 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.
#11
@
2 years 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.
2 years 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.
#13
@
2 years 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.
2 years ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
2 years ago
#16
@
2 years 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.
#17
@
20 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
@
20 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.
20 months ago
This ticket was mentioned in Slack in #core by costdev. View the logs.
20 months ago
#22
@
20 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...
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
20 months ago
#24
@
20 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.
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 by mukeshpanchal27. View the logs.
19 months ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
19 months ago
#28
@
19 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
@
17 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.
17 months ago
#31
@
17 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.
17 months ago
#32
Tests on 26381.4.diff
Trac ticket: https://core.trac.wordpress.org/ticket/26381
#33
@
17 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
#26382 was marked as a duplicate.