Problem/Motivation

When testing a patch for #1493324: Inline form errors for accessibility and UX a test for the set shortcut set form failed. Upon investigation I found issues with the form itself.

  1. Selecting new set and attempting to submit the form with no label is confusing in Chrome and FF because the machine name is hidden but required. Chrome tries to show a popup on the field itself but can't because it is hidden.
  2. The label for the label field should not be hidden, it is unclear what the text area is for.
  3. When you remove the required attribute on the machine name and submit the form with it blank, the form is not validated correctly and an exception occurs. This is because it tries to make an entity with no machine name.
  4. The label field doesn't get marked as having an error.

Steps to reproduce:
As a logged in user visit /user and click the shortcuts tab.
Shows shortcut tab on user page

Highlights

Select new set, leave text field blank and click change set. Observe error message in corner on firefox
Highlights firefox warning

Proposed resolution

  1. Possibly require the label field.
  2. Unhide the label for the label field.

Remaining tasks

  1. Mark the label input field as having an error when there is an error for it.

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because it posses accessibility and usability problems
Issue priority Major because it blocks another major #1493324: Inline form errors for accessibility and UX
Unfrozen changes Unfrozen because it only changes markup by adding a required attribute to an input field label for required errors to attach
Prioritized changes The main goal of this issue is usability/accessibility

Comments

crasx’s picture

Issue summary: View changes
StatusFileSize
new14.89 KB
new20.69 KB
new46.93 KB

added screenshots and steps to reproduce

nitesh pawar’s picture

Tested with Safari and Chrome on Mac OS X Yosemite. Issue not replicated.

tstoeckler’s picture

Status: Active » Needs review
StatusFileSize
new635 bytes

So in the meantime #2427161: shortcut.admin.js broken, replace with #states actually made the label of that field visible. The misplaced HTML5 required notification was still valid, though. I could fix this locally by adding an additional required state. Apparently, #states support HTML5. Yay!

Please test.

dmsmidt’s picture

Issue summary: View changes
Status: Needs review » Needs work
StatusFileSize
new30.28 KB

The patch successfully applies and adds the HTML5 required state.

However, when there is no HTML5 required checks and the form submits, the element is not marked as having an error.
This currently still blocks #1493324: Inline form errors for accessibility and UX, there is no inline message and makes the tests fail.

tstoeckler’s picture

Assigned: Unassigned » tstoeckler
Issue tags: +drupaldevdays

Taking a look at this...

Thanks for the review, I should have tested the non HTML5 version, but I tend to forget. Much appreciated.

nlisgo’s picture

Assigned: tstoeckler » nlisgo

I'm going to try an help out on this issue this afternoon. I will unassign myself at the end of the day if I have been unable to assist.

nlisgo’s picture

Assigned: nlisgo » Unassigned
Status: Needs work » Needs review
StatusFileSize
new746 bytes
new1.09 KB
dmsmidt’s picture

Status: Needs review » Reviewed & tested by the community

Works great, also in combination with inline errors.

Lets get it in!

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I don't think we can yet test the #states change, but we absolutely can test that validate change.

nlisgo’s picture

Assigned: Unassigned » nlisgo

I'm going to take on the writing of this test.

nlisgo’s picture

Assigned: nlisgo » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new812 bytes
new812 bytes
new1.88 KB

This test should address the feedback in #9.

The last submitted patch, 11: switch_shortcut_set-2409885-11-test-only.patch, failed testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

joelpittet’s picture

Issue summary: View changes

Added beta evaluation.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c83565f and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed c83565f on 8.0.x
    Issue #2409885 by nlisgo, tstoeckler, crasx, dmsmidt: Switch shortcut...
mgifford’s picture

Issue tags: -Accessibilty +Accessibility

Excellent.. This got fixed before I even saw it. Thanks everyone!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.