I get the following JS error when hitting the 'Save and continue' button on the Database configuration page during an install of Drupal 8. It prevents the form from submitting, leaving the user stuck there.

An invalid form control with name='pgsql[username]' is not focusable.

Steps to reproduce:

  1. Attempt a fresh install using Chrome
  2. Leave MySQL database option selected and enter details
  3. Press 'Save and continue'

I can't replicate the same behaviour in Safari or Firefox. One noticeable difference is that in Safari and Firefox, the client-side inline validation warnings don't appear when I leave a field blank and try to submit, whereas they do in Chrome.

See also the Chrome bug report: https://code.google.com/p/chromium/issues/detail?id=166549

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Title: Javascript error prevents install using Chrome on the Mac » Javascript error prevents install using Chrome on the Mac or adding new languages
Priority: Normal » Major
Issue tags: +JavaScript

This also happens for the 'Add language' button on the add language page (admin/config/regional/language/add)

nod_’s picture

Can't replicate with chrome on linux :(

floydm’s picture

Assigned: hemantsharma » Unassigned
FileSize
352.74 KB

I also ran into this on Chrome 30.0.1599.66 on Mac and could not complete the install.

In Safari, no such error.

alexweber’s picture

+ 1 Bug confirmed on OSX 10.8.5 & Chrome 30
+ 1 confirm that the bug doesn't occur in Safari

nod_’s picture

Could you tell me what happens when JS is disabled?

@Floydm: can you remove the image from inside the text please? it's needlessly slowing the page load, thanks :)

hemantsharma’s picture

Assigned: Unassigned » hemantsharma

I am going to work on it.

floydm’s picture

Assigned: Unassigned » hemantsharma
FileSize
140.23 KB

@nod_ Attached is a snap of what I get when I click submit with javascript disabled on Chrome.

swentel’s picture

And these are console errors on the add language page

Screen Shot 2013-10-06 at 00.05.19.png

nod_’s picture

hemantsharma’s picture

I am using mysql but the element displayed is "pgsql" one. Does anybody know how page decides which input field is made visible and hidden?

hemantsharma’s picture

Looks like database name and user name fields are required. Based on radio button selection, only corresponding database name and username field are made visible, other 2 are hidden (But still required). When we submit, validation logic still try to run required validation on hidden fields and thus get this error as field is not visible.

My question is why we are generating database name and user name field for each type of database? We can just have single field to capture database name and user name.

Anonymous’s picture

It seems as though Chrome is doing its own validation and sees the "required" attribute on the username and password fields for all of the various database types. However, most of those fields are hidden. Chrome is trying to "focus" on those fields to alert us that they're required, but it can't do so because they're hidden.

Also, it appears as though Chrome 29.x doesn't have this problem, but Chrome 30.x does.

nod_’s picture

Title: Javascript error prevents install using Chrome on the Mac or adding new languages » Browser validation error with chrome on hidden required fields
Component: install system » forms system

Not a JS issue. I guess related is #1797438: HTML5 validation is preventing form submit and not fully accessible. Leaving JS tag because if we do something to fix it, we'll need to use some JS.

evilehk’s picture

By adding a #states property to the $form['database'] and $form['usernames'] that makes those fields required when ':input[name=driver'] is the name of the appropriate pdoDriver (mysql, pgsql, etc.), and removing the #required field, the problem will go away in Chrome 30. However, that only makes the fields 'look' required. The states.js does not actually make the input field required, just changes the label. A check that the database name and username fields have values can be put in a validation function.

evilehk’s picture

If the 'required' #states route is a viable solution, I re-rolled this patch that will make the input field required as opposed to just changing the field label.
https://drupal.org/node/1372374#comment-7938551

nod_’s picture

YesCT’s picture

I have seen several new people trying to work on d8 bugs run into this.

evilehk’s picture

Status: Active » Needs review
FileSize
2.58 KB

I used Chrome's "Report an issue" feature that, in short, asks if Chrome should respect the html5 required attribute in a hidden form field. If so, then the behavior of trying to make the hidden field focusable seems like a bug in Chrome

In any event, this patch solves the issue in the following ways:
1) has the drupal8.javascript.1372374-7.patch applied to states.js (see comment #15)
2) removes the #required form property from the database driver specific config form, and replaces it with an appropriate #states property

I'm not sure who to assign the ticket too. I should really attend one of YesCT's core sprints!

alanburke’s picture

Patch works - in that it allows Drupal to be installed on Chrome on osx.
Not sure whether this is the right approach - but it looks good to me.

hass’s picture

I do not think we should add workarounds for a broken chrome v30 to core. Better push Google to fix in v30.xx or v31 and the issue will be gone in 4 weeks.

nod_’s picture

Status: Needs review » Needs work

Well to be fair it makes sense to add it there. I'm for it.

Because the field isn't really required when you don't choose something else than mysql. Firefox will show a crappy button at the top of the browser (on linux at least) and still fail the validation (saw that the other day when trying it out).

The only problem is that it should be using a theme function in JS to add the <abbr> tag.

YesCT’s picture

Title: Browser validation error with chrome on hidden required fields » Browser validation error with chrome on hidden required fields leads people to not be able to install

I read the chrome bug report. The chrome fix may only help in so much as to give an error/hint that a field in the collapse advanced is required. which will help a bit.

But wont that just make us open up that fieldset? And it is collapsed because the default is fine. So we might still want to do something ourselves here. (Also, having alphas out there that people cannot install is bad d8 press.)

@hemantsharma did you want to keep this assigned to you and try out what @nod_ recommends in #21?

evilehk’s picture

@hemantsharma, please feel free to ping me on the IRC channels if you would like to work together on this. Because of the severity of the issue and the potential for bad D8 press as mentioned in the previous comment, I added the abbr tag to a JS Drupal.theme function. Patch is attached.

evilehk’s picture

Status: Needs work » Needs review
evilehk’s picture

Added spacing between {} to adhere to Drupal coding standards. Third time's a charm!

evilehk’s picture

removes variables only useful in debugging. Thanks again for the help in irc, nod_.

nod_’s picture

Assigned: hemantsharma » Unassigned

Patch is not the usual format, there might be a problem for applying. Other than that, It looks good to me.

nod_’s picture

Status: Needs review » Needs work

Ok so that fixes the install but not the language form, let's get all the ones we know directly.

Can you add the changes to the form on this page /admin/config/regional/language/add so that chrome users can add a custom language?

Can you make a simple git diff for your next patch as well? it threw me and a few others off ;)

Thanks!

evilehk’s picture

Assigned: Unassigned » evilehk
evilehk’s picture

Applies fixes to language add form. Latest patch and interdiff attached.

evilehk’s picture

Status: Needs work » Needs review
nod_’s picture

Looks good to me :)

One gotcha here is that it means we don't have validation for non-js users anymore. We might need to keep the required attribute by default and remove it with JS if there is a #states required condition that applies on it.

nod_’s picture

Title: Browser validation error with chrome on hidden required fields leads people to not be able to install » Browser validation error with chrome on hidden required fields

title is too restrictive.

Status: Needs review » Needs work

The last submitted patch, drupal8.forms-system.2101427-30.patch, failed testing.

evilehk’s picture

Status: Needs work » Needs review
FileSize
842 bytes
1.4 KB
3.73 KB

Good point, nod_. It looks like SimpleTest agrees with you, and checks that those fields are required. This latest patch keeps the required attribute, and states is smart enough to remove the attribute when it's not needed. Locally, all language tests pass.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Perfect, thank you :)

Tested, does remove the attribute and puts it back as expected.

ndeet’s picture

Can confirm that the patch works on OSX 10.8.5 with Chrome Version 30.0.1599.101

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Tested the installer without Javascript. This patch actually fixes a bug due to the fact that both postgres and mysql credentials are required.

tim.plunkett’s picture

Committed 5ab1bfc and pushed to 8.x. Thanks!

:)

David_Rothstein’s picture

Title: Browser validation error with chrome on hidden required fields » (Rollback) Browser validation error with Chrome on hidden required fields, even on forms that don't allow client-side validation
Priority: Major » Critical
Status: Fixed » Postponed
Issue tags: +revisit before beta

The installer part of this patch is incorrect. If a field is required, we shouldn't be removing server-side validation for it. This is fundamentally wrong, but also has some impact on the user interface: after this patch the user no longer gets the normal visual feedback (message and red border) if they submit the form without the required field filled out.

#38 is incorrect also; since the form uses #limit_validation_errors it would never have previously forced you to type in both sets of credentials, even with JavaScript off.

I think the actual bug here is that Chrome is not respecting the "formnovalidate" property on the submit button? It shouldn't be doing any kind of client-side validation on these forms at all, but apparently it is: https://code.google.com/p/chromium/issues/detail?id=303707

If that's true, there's nothing Drupal can do to fix it, and it will have repercussions well beyond these couple of forms. Luckily, it looks like they already have a fix ready for Chrome.

So maybe there's some value in leaving this for now, if it helps people install Drupal, but once the browser fix is out this should be rolled back (or at least the installer part should).

nod_’s picture

Assigned: evilehk » Unassigned
Status: Postponed » Needs review
FileSize
1.98 KB

Here is a patch that fixes the issue, I think the #required in the install was simply forgotten, we talked about it in #32 and #35 for the language form.

Looked into the install system, There is one change to make to the required condition (which tells me not many people uses since it was already broken). It's that the marker is added twice on initialization, fixed that.

nod_’s picture

Title: (Rollback) Browser validation error with Chrome on hidden required fields, even on forms that don't allow client-side validation » Follow-up: Browser validation error with Chrome on hidden required fields, even on forms that don't allow client-side validation
nod_’s picture

Issue summary: View changes

added chrome bug report link

nod_’s picture

David_Rothstein’s picture

#41 looks good to me. Someone who actually has the affected version of Chrome should probably RTBC, though, to make sure it doesn't cause the problem to reoccur...

We should still have a followup to remove the extraneous #states code entirely (once the Chrome bug is fixed), but this looks great in terms of making the fields fully required again.

The states.js fix looks good also. Probably people don't run into that often because (with the exception of working around this browser bug) it is hard to imagine good reasons to use #states to toggle the required status when the server-side code is going to force it to be required always... But I think I've run into the double asterisk once before myself (while doing some complex, custom form API stuff with #states; don't remember exactly what), and this would probably have fixed it.

David_Rothstein’s picture

Priority: Critical » Major
Issue summary: View changes

I don't actually remember why I bumped this to critical. It's bad to have a required field not actually required on the server-side, but if someone submits this particular form without filling those fields out I don't see how it will cause a critical problem or anything.

alexpott’s picture

Well I tested this with no javascript before committed and decided that the actually removing the required is an improvement!

With the patch in #41 and HEAD before this was committed the form without JS looks like this

Only local images are allowed.

Which means you have to enter db user and password for both postgress and mysql in order to proceed!!!

HEAD atm looks like this

Which I think is great because if you select the db type and enter invalid credentials then you get the expect db connection error.

And HEAD with JS looks like this

Which means yes it will fail quicker if you haven't entered the required fields - but I believe this is the best form degraded functionality for no js users... having people fill out unnecessary fields is very bad UX imo.

Although obviously you could argue this was beyond the scope of the original issue :)

evilehk’s picture

In my experience, using Chrome 30.0.1599.101 m on Windows with javascript disabled, the patch from #41 did add the required label to both mysql and postgres credentials. However, if I chose mysql and only put in the credentials for mysql, there was no complaint that the postgres fields were missing. I believe this is due to the #limit_validation_errors setting that David_Rothstein mentioned in #40.

I am not sure what the better user experience is; to have the asterisks that makes all database name and credential fields look required, or to not have the required labels. In any event, the states.js changes look good.

I closed issue #1372374: Adapt states.js to new required html5 attribute, because the states.js changes committed in this issue solve that problem. If this patch is to be rolled back, issue #1372374 might be a good place for the states.js changes.

evilehk’s picture

David_Rothstein’s picture

In my experience, using Chrome 30.0.1599.101 m on Windows with javascript disabled, the patch from #41 did add the required label to both mysql and postgres credentials. However, if I chose mysql and only put in the credentials for mysql, there was no complaint that the postgres fields were missing. I believe this is due to the #limit_validation_errors setting that David_Rothstein mentioned in #40.

Yup.

In summary:

  • Before #41, everyone has a bad user experience (required fields were not properly validated as required) and the server-side validation was incorrect.
  • After #41, only people with JavaScript turned off have a bad user experience (they see extra red asterisks on fields that aren't technically required). This is the same experience they'll have on various other forms too, not just the installer.

I assume you tested that version of Chrome with JavaScript turned on too? If so, want to mark this RTBC?

I closed issue #1372374: Adapt states.js to new required html5 attribute , because the states.js changes committed in this issue solve that problem. If this patch is to be rolled back, issue #1372374 might be a good place for the states.js changes.

I don't think there's any reason to roll back the states.js parts, so all good there.

The only parts that should be rolled back (eventually) are the parts that added extra #states handling to the two specific forms.

evilehk’s picture

Status: Needs review » Reviewed & tested by the community

Tested #41 in Chrome 30+ with javascript disabled as well as enabled. Marking as RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome work, folks!

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

ianthomas_uk’s picture

My reading of the chrome bug report linked in #40, https://code.google.com/p/chromium/issues/detail?id=303707, is that they fixed this in Chrome 32. That's 2-3% of surfers, based on http://caniuse.com/usage_table.php (presumably less with our audience).

I'd say we're OK to roll back the parts of this that David_Rothstein wants rolled back, but I don't really understand the issue they are causing enough to open a followup myself, nor if there is any particular urgency to removing them.

catch’s picture

Issue tags: -revisit before beta +revisit before release

Moving to 'before release' for now, this wouldn't involve an API change.

xjm’s picture

Issue tags: -revisit before release +revisit before release candidate
catch’s picture

Issue tags: -revisit before release candidate