WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#31608 closed enhancement (fixed)

Shiny Updates: fine-tune UI for filesystem credentials request modal

Reported by: ericlewis Owned by: ericlewis
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.2
Component: Upgrade/Install Keywords: has-patch ui-feedback needs-testing
Focuses: ui, accessibility, javascript Cc:

Description

The filesystem credentials request when updating or installing a plugin has been merged.

@melchoyce has previously drafted a mockup for a filesystem credentials request.

We opted for a modal for the UI of this interaction because a modal is more portable, and avoids customization of the same UI in multiple locations it will be used in (e.g. plugin cards, plugin list table, theme list table, theme cards, core upgrade screen).

The FS creds form is typically plopped on its own screen, and has as much room to breathe as any other Settings page, which it shares likeness in both markup and layout. Mel's mockup tightens the request filesystem credentials UI in a constrained space without complicating anything. Let's implement.

Attachments (12)

31608.diff (5.7 KB) - added by ericlewis 6 years ago.
Screen Shot 2015-03-19 at 9.39.11 AM.png (83.4 KB) - added by Ipstenu 6 years ago.
There's no cancel option!
31608.2.diff (7.4 KB) - added by ericlewis 6 years ago.
31608.2.cred-request-modal.png (157.5 KB) - added by ericlewis 6 years ago.
31608.2.cred-request-modal-with-ssh.png (176.7 KB) - added by ericlewis 6 years ago.
31608.2.cred-request-in-page.png (70.8 KB) - added by ericlewis 6 years ago.
31608.3.patch (11.9 KB) - added by afercia 6 years ago.
Accessibility treatment first pass
31608.3.diff (13.4 KB) - added by ericlewis 6 years ago.
31608.4.diff (14.3 KB) - added by ericlewis 6 years ago.
31608.5.diff (13.4 KB) - added by ericlewis 6 years ago.
31608.6.diff (14.3 KB) - added by jorbin 6 years ago.
31608.7.diff (14.7 KB) - added by ericlewis 6 years ago.

Download all attachments as: .zip

Change History (41)

@ericlewis
6 years ago

#1 follow-up: @ericlewis
6 years ago

attachment:31608.diff is a work in progress.

We should take this opportunity to refactor the filesystem request form markup. Its currently in a table layout, similar to a Settings page. Started breaking things out to that affect.

Keep in mind, this markup will support both the creds request UI in both the modal context and the single page context, we should style accordingly.

#2 in reply to: ↑ 1 @afercia
6 years ago

  • Focuses accessibility added

Replying to ericlewis:

We should take this opportunity to refactor the filesystem request form markup. Its currently in a table layout, similar to a Settings page. Started breaking things out to that affect.

Totally agree, thanks very much :) adding accessibility focus since there's room for a11y improvements.

Last edited 6 years ago by afercia (previous) (diff)

#3 @DrewAPicture
6 years ago

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

This ticket was mentioned in Slack in #core by drew. View the logs.


6 years ago

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


6 years ago

This ticket was mentioned in Slack in #core by jorbin. View the logs.


6 years ago

@Ipstenu
6 years ago

There's no cancel option!

#7 @Ipstenu
6 years ago

Not 100% sure this shouldn't be it's own ticket, but right now there's no way to cancel out of the modal. There's no button and ESC didn't work.

See the image I attached (above) for how it look.s

Last edited 6 years ago by Ipstenu (previous) (diff)

This ticket was mentioned in Slack in #core by jorbin. View the logs.


6 years ago

#9 @jorbin
6 years ago

@ipstenu - I don't think that needs to be it's own ticket. I think it's part of us improving the UI and UX which is what this ticket aims for.

@ericlewis
6 years ago

#10 @ericlewis
6 years ago

In attachment:31608.2.diff,

  • Add a "Cancel" button which closes the modal on click.
  • Add bottom margin to form elements to match <p> tag margins.
  • Add margin in between connection types.

See previous screenshots for the current state of styles.

I am touching age-old markup and CSS and would love someone familiar with our current CSS best-practices to either provide feedback or take over the implementation here.

Last edited 6 years ago by ericlewis (previous) (diff)

#11 @DrewAPicture
6 years ago

  • Keywords has-patch ui-feedback added

#12 follow-up: @joedolson
6 years ago

  • Keywords has-patch ui-feedback removed

Quick accessibility note - the cancel button should also handle focus assignment, and set focus back to the referring control.

#13 @DrewAPicture
6 years ago

  • Keywords has-patch ui-feedback added

:)

This ticket was mentioned in Slack in #core by eric. View the logs.


6 years ago

#15 in reply to: ↑ 12 @afercia
6 years ago

Replying to joedolson:

Quick accessibility note - the cancel button should also handle focus assignment, and set focus back to the referring control.

The "update link" gets removed from the DOM as soon as the modal open so we should chose a different element where to set focus back, maybe the "Select" checkbox? Please let me know your thoughts. See screenshot.

https://cldup.com/niHEV56oOz.png

Not sure the "Updating" message should kick in at this point, same for the "Install" button in the plugin cards that changes in "Installing" even before you confirm or cancel your credentials.

#16 @joedolson
6 years ago

I'm not hugely bothered by the 'Updating' message - it's not totally inaccurate; the "Updating" process has begun, even if no actual files are being written yet. But I can see an argument for an intermediate notice at this stage of 'Requesting authentication' or something like that.

So, thinking about my own process, if I cancel from updating a plug-in in a situation like this, it's usually because I'm intending to switch contexts to find my FTP info for the site. If this is common, it probably doesn't matter too much exactly where the focus goes, as long as it's proximate to the initiation. The checkbox for the plug-in would probably work.

#17 @afercia
6 years ago

  • Focuses javascript added
  • Keywords dev-feedback added

Thanks @joedolson :) Updated patch is a first accessibility treatment pass:

  • add role=dialog and aria-labelledby on the modal container
  • add aria-describedby to the relevant form field, targeting the existing descriptions
  • I'd suggest to change "e.g." in "example:" for better readability
  • JavaScript: move focus to the modal first input field when the modal opens
  • JavaScript: constrain keyboard navigation inside the modal
  • JavaScript: make the "close modal" function reusable
  • JavaScript: close the modal when pressing Escape or clicking the overlay
  • JavaScript: set focus back to the previously active element (if any) or a reasonable element ("Select" checkbox)

Would recommend a JavaScript review from some lead dev, when they have a chance :)

@afercia
6 years ago

Accessibility treatment first pass

#18 @afercia
6 years ago

Outstanding issues:

  • check headings hierarchy

bit tricky because the same output is used for the modal (where the first heading should be H1) and "in page" when JavaScript is off.

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


6 years ago

@ericlewis
6 years ago

#20 @ericlewis
6 years ago

Thanks for the help @afercia!

In attachment:31608.3.diff, riffing off attachment:31608.3.patch:

  • Fix a bug that left/right keypress events triggered closing the modal.
  • Move object property definitions to the top of the file, group methods.
  • Simplify/combine logic for storing the jQuery element to return focus to after closing the request for credentials modal in wp.updates.$elToReturnFocusToFromCredentialsModal
  • Not head-over-heels for passing a jQuery event object into wp.updates.requestFilesystemCredentials( e ), but I'm on the fence about separating the logic requiring the event into its own method.

#21 @afercia
6 years ago

Thanks very much @ericlewis!
Noticed a small thing, the modal should close also when clicking on the overlay, see:

$( '#request-filesystem-credentials-dialog [data-js-action="close"], #request-filesystem-credentials-dialog' ).on( 'click', function() {
...

@ericlewis
6 years ago

@ericlewis
6 years ago

#22 follow-up: @ericlewis
6 years ago

Gotcha @afercia. I missed the utility intended by that selector, and saw that it closed the modal when using the keyboard to toggle Connection Types. Let's use .notification-dialog-background to target.

In attachment:31608.5.diff, bring back @afercia's functionality to close the modal when clicking on the backdrop.

#23 in reply to: ↑ 22 @afercia
6 years ago

Replying to ericlewis:

and saw that it closed the modal when using the keyboard to toggle Connection Types

And I missed that :) Thanks very much @ericlewis

@jorbin
6 years ago

#24 follow-up: @jorbin
6 years ago

This is getting pretty close. A couple of changes that I made:

  • On small windows (those with a height less than 480px which is the size of the window ), the modal is now fullscreen to be more usable. https://cldup.com/OAmoX5eh_W.png
  • Canceling now restores the original HTML instead of having "updating..." stay there

More work remains on making sure that canceling doesn't block you from trying again. Some flows that need to be refined and could use some more testing include:

  • Click Update, cancel, click update again
  • Click Update, enter bad credentials, cancel, click update again
  • Click Update, cancel, click update on a different plugin

I think the error state modal (what comes back when a user has entered the wrong credentials) needs an a11y review.

#25 @DrewAPicture
6 years ago

#31802 was marked as a duplicate.

#26 @DrewAPicture
6 years ago

  • Keywords needs-testing added; dev-feedback removed

@jorbin @ericlewis, what's left here? We need to wrap this up.

@ericlewis
6 years ago

#27 in reply to: ↑ 24 @ericlewis
6 years ago

In attachment:31608.7.diff, follow-up on jorbin's notes:

  • Click Update, cancel, click update again

Works.

  • Click Update, enter bad credentials, cancel, click update again

Works.

  • Click Update, cancel, click update on a different plugin

Works.

#28 @jorbin
6 years ago

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

In 31949:

Refine UI for FTP modal and shiny updates

Numerous changes to make the FTP modal experience a good one. These include:

  • Update HTML used by both the form here and the form on the standalone screen
  • Allow users to cancel FTP install
  • Focus locking in the modal
  • Focus on modal form on load
  • ARIA Attributes
  • Style Enhancements
  • Add low screen height (such as phone and some tablets) friendly experience for entering credentials

Props ericlewis, afercia
Fixes #31608

#29 @jorbin
6 years ago

In 31969:

Fix colors for activated and updated plugins

If we don't have the updated class on the TR for he update message, then the message is red and red is scary and they just updated, they should be happy and celebrating, not scared.

See #31608

Note: See TracTickets for help on using tickets.