WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#34376 closed defect (bug) (fixed)

Review the FTP credentials form

Reported by: afercia Owned by: mte90
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.2
Component: Administration Keywords: has-patch has-screenshots
Focuses: ui, accessibility, javascript Cc:
PR Number:

Description

Note: to make the FTP credentials form appear and test it I'm using the Core Control plugin.

In the FTP credentials form there's still some inline JavaScript to toggle the visibility of an element with ID "ssh_keys". That element was a table row for the SSH authentication keys and the table was removed in [31949]. The SSH fields are still there, but not inside a table.

Checking how it worked in WordPress 4.0, the SSH fields were initially hidden with some inline CSS if 'ssh' != $connection_type and then the visibility toggled when clicking the FTP or FTPS radio buttons:

https://cldup.com/Pdi5p9wBiU.png

https://cldup.com/VYV5bh6oHK.png

I think the outdated part of the inline JavaScript should be tweaked or better removed in favor of what's in updates.js that currently works only in the modal.

I'd recommend also to move the "Connection type" radio buttons above all the other fields: for accessibility the revealed elements should be after the control that revealed them.

Attachments (5)

34376-fix.patch (530 bytes) - added by Mte90 4 years ago.
fieldset with class and hidden with js working
34376-connectiontype.patch (2.2 KB) - added by Mte90 4 years ago.
moved connection type after the hostname
34376.2.patch (3.7 KB) - added by Mte90 3 years ago.
compatible with 4.6, js removed from file.php and moved on updates.js
34376.3.patch (4.4 KB) - added by Mte90 3 years ago.
improvement after dev-feedback
34376.4.patch (5.0 KB) - added by afercia 3 years ago.

Download all attachments as: .zip

Change History (18)

#1 in reply to: ↑ description @afercia
4 years ago

I'd recommend also to move the "Connection type" radio buttons above all the other fields: for accessibility the revealed elements should be after the control that revealed them.

Or maybe just after the "Hostname" field.

@Mte90
4 years ago

fieldset with class and hidden with js working

@Mte90
4 years ago

moved connection type after the hostname

#2 @Mte90
4 years ago

  • Keywords has-patch added

I've created two patch, the first have the same effect of the old table system. The fieldset that contain the ssh part have a class and is hidden on start if is not picked the ssh connection and js now work again.
The second move the connection type after the hostname, i think that is the better position and at the start of the form is better, the user fill the form following the fields order.

#3 @afercia
4 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to afercia
  • Status changed from new to assigned

#4 @Mte90
3 years ago

  • Keywords dev-feedback added

There are any plan for the 4.6?

#5 @Mte90
3 years ago

  • Keywords needs-refresh added; has-patch removed

As I can see the patch need a refresh for the 4.6 because the line numbers are changed but the code to edit it is the same.

#6 @afercia
3 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Future Release to 4.6
  • Owner changed from afercia to mte90

The JS part needs to be reviewed a bit too :)

@Mte90
3 years ago

compatible with 4.6, js removed from file.php and moved on updates.js

#7 @Mte90
3 years ago

  • Keywords has-patch added; needs-refresh removed
  • The radio button are at the top of the fields
  • jQuery code in file.php moved to updates.js
  • Added a check to hide the fieldset for the ssh2 fields

#8 @afercia
3 years ago

Thanks very much @Mte90. Some feedback on the patch.

My bad, maybe in my ticket description the words "above all the other fields" are a bit confusing, I meant to move the "Connection Type" radio buttons above the "Authentication Keys" fields but below the Username and Password fields. I'd recommend this order:

  • Hostname
  • Username
  • Password
  • Connection Type
  • and if SSH2 is selected: Authentication Keys

This way, when users change the selected radio buttons, the Authentication Keys fields will collapse/expand below the radio buttons

The toggled element should include the span with the text "Enter the location on the server where the public ..." so maybe wrap everything in a div?

https://cldup.com/foYgmVGutV.png

https://cldup.com/1jv2EAMIuP.png

About the JS part, there are now both click and change events attached on the radio buttons and they call functions meant to do the same thing

https://cldup.com/bchVuFZpkL.png

The click event comes from the old inline JS now moved in updates.js

jQuery("#ssh").click(function () {
	jQuery("#ssh_keys").show();
});
jQuery("#ftp, #ftps").click(function () {
	jQuery("#ssh_keys").hide();
});
jQuery('#request-filesystem-credentials-form input[value=""]:first').focus();

The change event comes from

// Hide SSH fields when not selected
$( '#request-filesystem-credentials-dialog input[name="connection_type"]' ).on( 'change', function() {
	$( this ).parents( 'form' ).find( '#private_key, #public_key' ).parents( 'label' ).toggle( ( 'ssh' == $( this ).val() ) );
}).change();

I'd keep the change event and in the first block keep just the line to set initial focus on the first form field and remove all the rest.
The second block should be adjusted to toggle the entire fieldset and the description text (it currently targets only the public and private fields labels).
Also, it currently works only in the modal, the jQuery selector for on() should be adjusted in order to work also in the Updates page. I'd probably just remove #request-filesystem-credentials-dialog unless I'm missing something.

Lastly, it would be a nice opportunity for some coding standards, but only on the changed lines please :)

#9 @Mte90
3 years ago

I will look for this improvements/suggestions in the next week :-)

This ticket was mentioned in Slack in #design by afercia. View the logs.


3 years ago

@Mte90
3 years ago

improvement after dev-feedback

#11 @Mte90
3 years ago

The version 3 contain:

  • Js improvement to simplify the code that works in both (modal/page) and with less events
  • Moved the connection box as examplained
  • Moved also the label "Enter the location on the server where the public ..."

@afercia
3 years ago

#12 @afercia
3 years ago

  • Keywords has-screenshots added

Patch looks good. In 34376.4.patch added some JavaScript and CSS tweaking. Screenshots with patch applied:

https://cldup.com/mCgYbAYrNe.png

https://cldup.com/fli3X4fQXV.png

#13 @afercia
3 years ago

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

In 37467:

Refine the FTP credentials form interaction.

Properly toggle SSH2 Authentication Keys fieldset visibility.
JavaScript and CSS clean-up.

Props Mte90.
Fixes #34376.

Note: See TracTickets for help on using tickets.