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

[jane]

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.

[jane]

Two buttons at same time

[tommcfarlin]

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.

[tommcfarlin]

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

[tommcfarlin]

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

[tommcfarlin]

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!

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

[tommcfarlin]

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?

[SergeyBiryukov]

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.

[kovshenin]

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?

[lessbloat]

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

[SergeyBiryukov]

Refreshed after [21592]

[SergeyBiryukov]

#21908 was marked as a duplicate.

[MikeHansenMe]

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' );
		}
	})

[nacin]

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

$('#upload-form #submit').prop('disabled', '' === $('#upload').val() );

[SergeyBiryukov]

20855.3.diff​ disables the button instead of hiding.

[helenyhou]

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

[SergeyBiryukov]

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

[helenyhou]

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

[SergeyBiryukov]

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

[SergeyBiryukov]

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

[kovshenin]

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

[helenyhou]

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

[lessbloat]

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.

[SergeyBiryukov]

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.

[lessbloat]

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.

[helenyhou]

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

[lessbloat]

Replying to helenyhou:

Also works in IE7 for me.

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

Sorry for the false alarm. :-)

[nacin]

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

[nacin]

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.

[SergeyBiryukov]

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

[SergeyBiryukov]

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

[nacin]

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

[MikeHansenMe]

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

[SergeyBiryukov]

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.

[MikeHansenMe]

20855.10.diff works well for me.

[nacin]

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.

[SergeyBiryukov]

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.

[kovshenin]

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

[SergeyBiryukov]

20855.12.diff​ uses the closest element if it exists and falls back to form.find() otherwise. Seems to work for all current use cases.

[SergeyBiryukov]

Replying to kovshenin:

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

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

[kovshenin]

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?

[kovshenin]

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

[SergeyBiryukov]

Same as 20855.13.diff, just different formatting

[SergeyBiryukov]

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.

[helenyhou]

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

[nacin]

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].

[nacin]

[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.

[nacin]

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.