WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years 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 3 years ago.
Two buttons at same time
20855.custom-header-upload-ux.patch (212.9 KB) - added by tommcfarlin 3 years 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 3 years 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 3 years 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 3 years ago.
20855.2.diff (3.7 KB) - added by SergeyBiryukov 3 years ago.
Refreshed after [21592]
20855.3.diff (1.9 KB) - added by SergeyBiryukov 3 years ago.
20855.4.diff (5.4 KB) - added by SergeyBiryukov 3 years ago.
20855.5.diff (5.4 KB) - added by SergeyBiryukov 3 years ago.
Minor tweak to button ID in plugin-install.php for consistency
20855.6.diff (5.3 KB) - added by SergeyBiryukov 2 years ago.
20855.7.diff (4.9 KB) - added by SergeyBiryukov 2 years ago.
20855.8.diff (5.8 KB) - added by kovshenin 2 years ago.
Works with importers upload form, also using .on() instead of .change() in common.js
20855.9.diff (461 bytes) - added by SergeyBiryukov 2 years ago.
20855.10.diff (983 bytes) - added by SergeyBiryukov 2 years ago.
20855.11.diff (929 bytes) - added by SergeyBiryukov 2 years ago.
20855.12.diff (999 bytes) - added by SergeyBiryukov 2 years ago.
20855.13.diff (1016 bytes) - added by kovshenin 2 years ago.
Concatenates all file fields when disabling/enabling the submit buttons.
20855.14.diff (1.0 KB) - added by SergeyBiryukov 2 years ago.
Same as 20855.13.diff, just different formatting

Download all attachments as: .zip

Change History (62)

@jane3 years ago

Two buttons at same time

comment:1 @kovshenin3 years ago

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

comment:2 @jane3 years ago

  • Milestone changed from Future Release to 3.5

@tommcfarlin3 years 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.

@tommcfarlin3 years ago

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

@tommcfarlin3 years 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: @tommcfarlin3 years 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: @helenyhou3 years 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: @tommcfarlin3 years 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 @SergeyBiryukov3 years 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.

@kovshenin3 years ago

comment:7 @kovshenin3 years 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 @lessbloat3 years 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).

@SergeyBiryukov3 years ago

Refreshed after [21592]

comment:9 @SergeyBiryukov3 years ago

#21908 was marked as a duplicate.

comment:10 @MikeHansenMe3 years 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 @nacin3 years 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 3 years ago by nacin (previous) (diff)

comment:12 @SergeyBiryukov3 years ago

20855.3.diff disables the button instead of hiding.

@SergeyBiryukov3 years ago

comment:13 @helenyhou3 years ago

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

@SergeyBiryukov3 years ago

comment:14 @SergeyBiryukov3 years ago

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

comment:15 @helenyhou3 years ago

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

@SergeyBiryukov3 years ago

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

comment:16 @helenyhou2 years ago

  • Keywords needs-refresh added

@SergeyBiryukov2 years ago

comment:17 @SergeyBiryukov2 years ago

  • Keywords needs-refresh removed

@SergeyBiryukov2 years ago

comment:18 @SergeyBiryukov2 years ago

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

@kovshenin2 years ago

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

comment:19 @helenyhou2 years 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 @SergeyBiryukov2 years ago

  • Keywords commit added

comment:21 follow-up: @lessbloat2 years 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: @SergeyBiryukov2 years 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 @lessbloat2 years 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: @helenyhou2 years 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 @lessbloat2 years 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 @nacin2 years ago

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

comment:27 @nacin2 years 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.

@SergeyBiryukov2 years ago

comment:28 @SergeyBiryukov2 years 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 @SergeyBiryukov2 years 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 @nacin2 years ago

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

comment:31 @MikeHansenMe2 years ago

  • Cc mdhansen@… added

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

@SergeyBiryukov2 years ago

comment:32 @SergeyBiryukov2 years 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 @MikeHansenMe2 years ago

20855.10.diff works well for me.

comment:34 @nacin2 years 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.

@SergeyBiryukov2 years ago

comment:35 follow-up: @SergeyBiryukov2 years 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.

@SergeyBiryukov2 years ago

comment:36 in reply to: ↑ 35 ; follow-up: @kovshenin2 years ago

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

comment:37 @SergeyBiryukov2 years ago

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

Version 0, edited 2 years ago by SergeyBiryukov (next)

comment:38 in reply to: ↑ 36 ; follow-up: @SergeyBiryukov2 years 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: @kovshenin2 years 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?

@kovshenin2 years ago

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

@SergeyBiryukov2 years ago

Same as 20855.13.diff, just different formatting

comment:40 in reply to: ↑ 39 @SergeyBiryukov2 years 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 @helenyhou2 years ago

  • Component changed from UI to Administration

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

comment:42 @nacin2 years 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 @nacin2 years 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 @nacin2 years 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.