Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#36316 closed defect (bug) (fixed)

Image editor: improve form validation errors handling

Reported by: afercia's profile afercia Owned by: afercia's profile afercia
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-screenshots
Focuses: ui, javascript Cc:

Description

When editing images and entering values for Scale, Crop ratio/selection in the Image Editor form, the check for entered values could be improved a bit. I've noticed this because a plugin was giving a JavaScript error and as a consequence, when entering new scale width and height, in the input field a "NaN" (literally) value appeared :)
Even with no plugin activated and no JS errors, when entering non-numbers, "Nan" is displayed to users, see below:

https://cldup.com/saCp6vVPpJ.png

Users would probably be a bit confused by seeing "NaN", I'd say it could be improved a bit. Worth noting the same thing doesn't happen in the Crop input fields: when entering non-number values, the field gets emptied.

Not pretending to completely refactor the code behind the Image Editor, which is a bit old but still works. I'd propose to keep things simple and:

  • use the same check for a numeric value used on the Crop fields on all the other fields: don't display "NaN", just empty the field
  • remove the inline script that runs the initialisation of the Image Editor and call it after the Editor UI is fully ready; this would probably help a bit to mitigate lack of data necessary for a correct initialisation

(technical detail: the plugin JS error caused the imageEdit.hold object to be empty because not initialised)

Attachments (2)

36316.patch (6.2 KB) - added by afercia 9 years ago.
36316.2.patch (6.6 KB) - added by afercia 9 years ago.

Download all attachments as: .zip

Change History (14)

@afercia
9 years ago

#1 @afercia
9 years ago

  • Keywords has-patch has-screenshots added

First pass. Any thoughts more than welcome.

#2 @afercia
9 years ago

  • Owner set to afercia
  • Status changed from new to assigned

#3 @afercia
9 years ago

  • Milestone changed from Awaiting Review to 4.6

@afercia
9 years ago

#4 @afercia
9 years ago

  • remove the inline script that runs the initialisation of the Image Editor

Looks like I forgot this part :) Patch refreshed.

This ticket was mentioned in Slack in #core-editor by afercia. View the logs.


9 years ago

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


9 years ago

This ticket was mentioned in Slack in #core-images by afercia. View the logs.


9 years ago

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


9 years ago

#9 @afercia
9 years ago

Note: about the inline event handlers, the patch just uses the existing pattern. Changing them would be good but maybe a bit out of the scope of this ticket. As mentioned in the ticket description:

Not pretending to completely refactor the code behind the Image Editor, which is a bit old but still works

This ticket was mentioned in Slack in #core-editor by afercia. View the logs.


9 years ago

#11 @afercia
9 years ago

It was suggested to use type="number" for the input fields, it would probably be a good thing even if not all browsers prevent to type non-numerical characters in those fields. By the way, worth mentioning the onkeypress event in imageEdit.init() currently triggers a function that doesn't allow to use the arrow keys on those fields when using Firefox. Worth reminding onkeypress works differently in different browsers and in Firefox it fires when using the arrow keys so the fields will be blurred because of:

if ( 36 < k && k < 41 ) {
	$(this).blur();
}

Thinking this part and all the events in the Image Editor should be completely reviewed, not sure if in this ticket.

#12 @ocean90
9 years ago

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

In 37966:

Media: Improve form validation errors handling when editing images.

  • Use the same check for a numeric value used on the crop fields on all the other fields: don't display "NaN", just empty the field.
  • Remove the inline script that runs the initialization of the image editor and call it after the editor UI is fully ready.

Props afercia.
Fixes #36316.

Note: See TracTickets for help on using tickets.