If the label field is not required for example, the machine_name will stay hidden with an inline style of "display: none;".
In Chrome this leads to the situation that if you submit the form you don't get an HTML5 form validation error, but the also the form will not submit, because the browser will throw an error saying "An invalid form control with name='id' is not focusable." There is no information for the user why the form is not submittable.

To fix this:

  • The machine name filed should only be "visually hidden"
  • if the field is invalid (empty), the field including the validation error should be shown.
Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions
CommentFileSizeAuthor
#11 interdiff_2488884-1-11.txt995 bytespixelmord
#11 drupal-core-machine-name-frontend-validation-2488884-11.patch1.27 KBpixelmord
#5 machine-name.gif3.31 MBtim.plunkett
#1 drupal-core-machine-name-frontend-validation-2488884-1.patch1.54 KBpixelmord

Comments

pixelmord’s picture

pixelmord commented
StatusFileSize
new drupal-core-machine-name-frontend-validation-2488884-1.patch1.54 KB

Here is a patch that sets the visually-hidden class instead of using jQuery.hide() and adds an event listener to the machine name input to show its container in case it is invalid (empty in this case).

pixelmord’s picture

pixelmord commented
Status: Active » Needs review

Ready to test and review

pixelmord’s picture

pixelmord commented
Issue tags: +frontend, +Novice
jmarkel’s picture

jmarkel commented
Issue summary: View changes
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +JavaScript
StatusFileSize
new machine-name.gif3.31 MB

I manually tested it and it worked great!
We can't really test this, so RTBC

nod_’s picture

nod_
Primary language French
Location Lille
commented

Agreed with the patch but the UX of that admin page is pretty bad, if the machine name is required the label should be too really. Or the form rearranged at least.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

I did fix that immediately after :)
http://cgit.drupalcode.org/page_manager/commit/?id=e2cda93

But I still think the patch is reasonable to go in.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

RTBC +1!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: drupal-core-machine-name-frontend-validation-2488884-1.patch, failed testing.

droplet’s picture

droplet commented

I agreed it still reasonable to go in. But instead of a new `invalidHandler`, it should call `clickEditHandler` to end up auto-generated machine name.

pixelmord’s picture

pixelmord commented
StatusFileSize
new drupal-core-machine-name-frontend-validation-2488884-11.patch1.27 KB
new interdiff_2488884-1-11.txt995 bytes

@droplet: Good idea, will save some lines of code.

I had to remove the e.preventDefault() in clickEditHandler() handler though, because it will prevent the machine name field to get focus. However this is unproblematic, because the we do not need to prevent the default anyways because the other listener is bound to the span.admin-link.

pixelmord’s picture

pixelmord commented
Status: Needs work » Needs review

Please review the modified patch

droplet’s picture

droplet commented
Status: Needs review » Reviewed & tested by the community

Great! Much better.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

That #5 gif is AWESOME! :D Much better.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 1a62e3c on 8.0.x 
    Issue #2488884 by pixelmord, tim.plunkett, droplet: Machine name HTML5...

Status: Fixed » Closed (fixed)

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