WordPress.org

Make WordPress Core

Opened 23 months ago

Closed 17 months ago

Last modified 17 months ago

#20855 closed task (blessed) (fixed)

Don't show Upload button until file has been selected.

Reported by: jane Owned by: kovshenin
Milestone: 3.5 Priority: normal
Severity: minor Version: 3.4
Component: Administration Keywords: has-patch commit
Focuses: Cc:

Description

There's an awkward UI situation in places (such as header image upload) where we show two secondary buttons at the same time, one to choose file and one to upload the file you will have theoretically already chosen before clicking upload.

It would be better ux to show the choose file button by default, and then only show the Upload button after a file has been selected.

Konstantin made this work in a project we're doing, have asked him to turn it into a patch for core.

Attachments (18)

Screen shot 2012-06-06 at 10.04.05 AM.png (24.7 KB) - added by jane 23 months ago.
Two buttons at same time
20855.custom-header-upload-ux.patch (212.9 KB) - added by tommcfarlin 21 months ago.
Patching the functionality and described in the ticket. Hides the 'Upload' button until a file has been provided. Created necessary dev files and updated minified files, as well. Register scripts using the script loader.
20855.custom-header-upload-ux.2.patch (213.9 KB) - added by tommcfarlin 21 months ago.
Neglected to properly include the new JavaScript files in the initial patch. Providing the full working copy in this patch.
20855.custom-header-upload-ux.3.patch (217.8 KB) - added by tommcfarlin 21 months ago.
Adding the fully working copy for improvements to both the user experience of the Custom Header page and the Custom Background page.
20855.diff (3.7 KB) - added by kovshenin 21 months ago.
20855.2.diff (3.7 KB) - added by SergeyBiryukov 20 months ago.
Refreshed after [21592]
20855.3.diff (1.9 KB) - added by SergeyBiryukov 19 months ago.
20855.4.diff (5.4 KB) - added by SergeyBiryukov 19 months ago.
20855.5.diff (5.4 KB) - added by SergeyBiryukov 19 months ago.
Minor tweak to button ID in plugin-install.php for consistency
20855.6.diff (5.3 KB) - added by SergeyBiryukov 18 months ago.
20855.7.diff (4.9 KB) - added by SergeyBiryukov 18 months ago.
20855.8.diff (5.8 KB) - added by kovshenin 18 months ago.
Works with importers upload form, also using .on() instead of .change() in common.js
20855.9.diff (461 bytes) - added by SergeyBiryukov 17 months ago.
20855.10.diff (983 bytes) - added by SergeyBiryukov 17 months ago.
20855.11.diff (929 bytes) - added by SergeyBiryukov 17 months ago.
20855.12.diff (999 bytes) - added by SergeyBiryukov 17 months ago.
20855.13.diff (1016 bytes) - added by kovshenin 17 months ago.
Concatenates all file fields when disabling/enabling the submit buttons.
20855.14.diff (1.0 KB) - added by SergeyBiryukov 17 months ago.
Same as 20855.13.diff, just different formatting

Download all attachments as: .zip

Change History (62)

jane23 months ago

Two buttons at same time

comment:1 kovshenin23 months ago

  • Owner set to kovshenin
  • Status changed from new to accepted

comment:2 jane22 months ago

  • Milestone changed from Future Release to 3.5

tommcfarlin21 months ago

Patching the functionality and described in the ticket. Hides the 'Upload' button until a file has been provided. Created necessary dev files and updated minified files, as well. Register scripts using the script loader.

tommcfarlin21 months ago

Neglected to properly include the new JavaScript files in the initial patch. Providing the full working copy in this patch.

tommcfarlin21 months ago

Adding the fully working copy for improvements to both the user experience of the Custom Header page and the Custom Background page.

comment:3 follow-up: tommcfarlin21 months ago

The current patches (*.3.patch being the full working copy) that I've contributed provide a solution for hiding the 'Upload' button until the 'Choose Image' input element values have changed for both the Custom Header and Custom Background pages.

One concern that I have is that there is code duplication between custom-header.dev.js and custom-background.dev.js as well as their minified counterparts:

Namely (the unminified code), this exists in both files:

// Watch for the value of the "Choose Image" field to change...
$('#upload').change(function() {
			
	// If the upload field is empty, we'll hide te upload button,
	// Otherwise, let's show it.
	if( '' === $('#upload').val() ) {
		$('#submit').hide();
	} else {
		$('#submit').show();
	} // end if/else
			
});

Ideally, I'd like to collapse this into a single JavaScript file (perhaps custom-upload.dev.js?) so that the code isn't duplicated; however, I'm unfamiliar with what's considered a best practice in this case.

Secondly, I've manually updated the minified versions of the related stylesheets and JavaScript, too. Is this typical practice or do you look for *.dev.js and *.dev.css to be contributed in patches and then run minification on your end?

Looking to you @kovshenin and @jane for advice on this - thanks!

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

  • Keywords has-patch added; needs-patch removed

Replying to tommcfarlin:

Patches should only be for *.dev.* files, using the SCRIPT_DEBUG constant in your dev environment to test those changes. bumpbot will take care of the minification of files when needed, post-commit :)

comment:5 in reply to: ↑ 4 ; follow-up: tommcfarlin21 months ago

Replying to helenyhou:

Replying to tommcfarlin:

Patches should only be for *.dev.* files, using the SCRIPT_DEBUG constant in your dev environment to test those changes. bumpbot will take care of the minification of files when needed, post-commit :)

Helen - thanks! First time applying a patch to core so just trying to make sure I get it correct.

I'd like to back and make this patch proper. What's typically considered best etiquette - provide a new patch with a full working copy or a patch that essentially rolls back the above patches in addition to the new work?

comment:6 in reply to: ↑ 5 SergeyBiryukov21 months ago

Replying to tommcfarlin:

What's typically considered best etiquette - provide a new patch with a full working copy or a patch that essentially rolls back the above patches in addition to the new work?

A new patch similar to your latest one, but excluding the minified files, is all that's needed.

kovshenin21 months ago

comment:7 kovshenin21 months ago

In 20855.diff

Using hide-if-js class to hide the submit button only if js is working. Moved action and nonce fields above the file field so that the file field and submit field are immediate siblings, and thus jQuery.next() can be used.

I don't really like the way hide-if-js is added to the button, since that argument is actually a "type" and not "class". Maybe we should wrap the submit button in a span with a hide-if-js class (and another class to target in js) instead. Thoughts?

comment:8 lessbloat21 months ago

I tested 20855.diff, and it looks good. Though this select box UI may change with the media manager overhaul (just a heads up).

SergeyBiryukov20 months ago

Refreshed after [21592]

comment:9 SergeyBiryukov19 months ago

#21908 was marked as a duplicate.

comment:10 MikeHansenMe19 months ago

Instead of using show / hide maybe we should use disabled so the button appears but is disabled until there is a file.

$( '#upload' ).change( function() {
		if( '' === $( '#upload' ).val() ){
			$( '#upload-form #submit' ).attr( 'disabled', 'disabled' );
		}else{
			$( '#upload-form #submit' ).removeAttr( 'disabled' );
		}
	})

comment:11 nacin19 months ago

.prop() needs to be used (jQuery 1.6+), rather than .attr(). Something like this would work:

$('#upload-form #submit').prop('disabled', '' === $('#upload').val() );
Last edited 19 months ago by nacin (previous) (diff)

comment:12 SergeyBiryukov19 months ago

20855.3.diff disables the button instead of hiding.

SergeyBiryukov19 months ago

comment:13 helenyhou19 months ago

I would imagine this same disabling should be done in similar places, e.g. uploading a plugin or theme.

SergeyBiryukov19 months ago

comment:14 SergeyBiryukov19 months ago

20855.4.diff does the same when uploading a plugin or theme.

comment:15 helenyhou19 months ago

Looks lovely. Left comments about things this patch made me notice about the buttons over on #21598.

SergeyBiryukov19 months ago

Minor tweak to button ID in plugin-install.php for consistency

comment:16 helenyhou18 months ago

  • Keywords needs-refresh added

SergeyBiryukov18 months ago

comment:17 SergeyBiryukov18 months ago

  • Keywords needs-refresh removed

SergeyBiryukov18 months ago

comment:18 SergeyBiryukov18 months ago

20855.7.diff is an attempt to use more generic JS.

kovshenin18 months ago

Works with importers upload form, also using .on() instead of .change() in common.js

comment:19 helenyhou18 months ago

20855.8.diff is looking good to me. Tested header, background, add plugin, add theme, and import screens with and without JS.

comment:20 SergeyBiryukov18 months ago

  • Keywords commit added

comment:21 follow-up: lessbloat18 months ago

Tested in Mac FF & Chrome, Win FF & Chrome & IE7. Works in all except IE7, but I think we can let that slide...

+1 to 20855.8.diff.

comment:22 in reply to: ↑ 21 ; follow-up: SergeyBiryukov18 months ago

Replying to lessbloat:

Tested in Mac FF & Chrome, Win FF & Chrome & IE7. Works in all except IE7, but I think we can let that slide...

Tested in IE7 and IE8, seems to work in both.

comment:23 in reply to: ↑ 22 lessbloat18 months ago

Replying to SergeyBiryukov:

Tested in IE7 and IE8, seems to work in both.

Maybe I should clarify. The upload functionality worked for me in IE7, but the "upload" button was not disabled.

comment:24 follow-up: helenyhou18 months ago

Also works in IE7 for me. Was JS perhaps off in your IE7? It won't be disabled if JS is off.

comment:25 in reply to: ↑ 24 lessbloat18 months ago

Replying to helenyhou:

Also works in IE7 for me.

What do you know... Hard refresh and it works.

Sorry for the false alarm. :-)

comment:26 nacin18 months ago

common.js changes seem like they need a bit of cleanup, but otherwise, this is good.

comment:27 nacin17 months ago

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

In 22459:

Forms with the class .wp-upload-form will now have their submit button disabled until a file is selected. props kovshenin, helenyhou, lessbloat, SergeyBiryukov, tommcfarlin. fixes #20855.

SergeyBiryukov17 months ago

comment:28 SergeyBiryukov17 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

20855.9.diff adds the same behaviour to the browser upload form on wp-admin/media-new.php.

comment:29 SergeyBiryukov17 months ago

Not sure if the form in the old media modal should also be patched. Will it be removed in RC?

Could not test the form in the new media modal yet: #22444

comment:30 nacin17 months ago

With 20855.9.diff, the button doesn't seem to re-enable after a file is selected.

comment:31 MikeHansenMe17 months ago

  • Cc mdhansen@… added

I am seeing the same problem on Ubuntu + FF16. The upload button does not become enabled after selecting a file.

SergeyBiryukov17 months ago

comment:32 SergeyBiryukov17 months ago

Turned out the screen also has the Plupload form (invisible if browser uploader is displayed):

<input type="file" multiple="multiple" accept="" style="font-size: 999px; position: absolute; width: 100%; height: 100%;" id="p17c0hrb00la712kf81o1t0svoi0_html5">

20855.10.diff adds filter(':visible'). The reason for it to be inside the function is to make sure it works when the Plupload form is initially displayed.

comment:33 MikeHansenMe17 months ago

20855.10.diff works well for me.

comment:34 nacin17 months ago

This is pretty much the main fear here, right? Something designed to make this a better experience ends up trapping the user into being unable to upload? There are hooks in media_upload_form() that could be used to render additional form controls, so I'm wondering if this needs more poka yoke than just :visible.

SergeyBiryukov17 months ago

comment:35 follow-up: SergeyBiryukov17 months ago

20855.11.diff is an attempt to use button.prev('input[type="file"]') to get the closest input element. Doesn't work for wp_import_upload_form() though.

SergeyBiryukov17 months ago

comment:36 in reply to: ↑ 35 ; follow-up: kovshenin17 months ago

SergeyBiryukov: sounds like you're looking for jQuery.closest()?

comment:37 SergeyBiryukov17 months ago

20855.12.diff uses the closest element if it exists and falls back to form.find() otherwise.

Version 0, edited 17 months ago by SergeyBiryukov (next)

comment:38 in reply to: ↑ 36 ; follow-up: SergeyBiryukov17 months ago

Replying to kovshenin:

SergeyBiryukov: sounds like you're looking for jQuery.closest()?

No, jQuery.closest() won't find a sibling element.

comment:39 in reply to: ↑ 38 ; follow-up: kovshenin17 months ago

Replying to SergeyBiryukov:

Oh, that's true, gotcha! 20855.12.diff looks reasonable and works in this case, but I think it's pretty fragile, since it will break if there's any other element between the file input and the submit button, because of the way .prev() works. The media upload form has two file inputs, and two submit buttons, which makes me think: what if they were all visible? :)

We can concatenate all the file input values with .map and check for an empty string. That will enable all submit buttons in a form if at least one file input has a value, which sounds like it could break less. Besides, we'd rather "not disable the submit button when we need to" than "not enable the submit button when we need to." Thoughts?

kovshenin17 months ago

Concatenates all file fields when disabling/enabling the submit buttons.

SergeyBiryukov17 months ago

Same as 20855.13.diff, just different formatting

comment:40 in reply to: ↑ 39 SergeyBiryukov17 months ago

Replying to kovshenin:

We can concatenate all the file input values with .map and check for an empty string. That will enable all submit buttons in a form if at least one file input has a value, which sounds like it could break less.

Sounds like a great idea to me.

comment:41 helenyhou17 months ago

  • Component changed from UI to Administration

20855.14.diff seems sane to me, and appears to work in all places.

comment:42 nacin17 months ago

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

In 22663:

Re-enable a .wp-upload-form submit button whenever any file input in that form has a file selected, to minimize the potential for issues. props kovshenin. fixes #20855. see [22459].

comment:43 nacin17 months ago

[22663] broke media-new.php. "Save All Changes" remains disabled despite the fact that the browser uploader is hidden.

To test this, you must be on r22750, prior to the changes in media-new.php, and upload one or more files using Plupload.

I'm going to revert a piece of [22663] to avoid breaking anything crazy being done by plugins on this page. Other forms are far simpler and should be okay.

comment:44 nacin17 months ago

In 22880:

Don't label the form on media-new.php an upload form to avoid esoteric breakage. In past lives, the form has been more complicated than the standard .wp-upload-form use case, with submit buttons that are not just for the uploader, which means we really can't disable them here.

Reverts part of [22663]. see #20855.

Note: See TracTickets for help on using tickets.