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:
- Attempt a fresh install using Chrome
- Leave MySQL database option selected and enter details
- 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
Comment | File | Size | Author |
---|---|---|---|
#46 | JS_head.png | 13.82 KB | alexpott |
#46 | Screenshot_09_11_2013_18_22-2.png | 97.86 KB | alexpott |
#46 | dbsettings_nojavascript-2.png | 49.52 KB | alexpott |
#41 | core-js-states-installer-2101427-41.patch | 1.98 KB | nod_ |
#35 | drupal8.forms-system.2101427-35.patch | 3.73 KB | evilehk |
Comments
Comment #1
swentel CreditAttribution: swentel commentedThis also happens for the 'Add language' button on the add language page (admin/config/regional/language/add)
Comment #2
nod_Can't replicate with chrome on linux :(
Comment #3
floydm CreditAttribution: floydm commentedI also ran into this on Chrome 30.0.1599.66 on Mac and could not complete the install.
In Safari, no such error.
Comment #4
alexweber CreditAttribution: alexweber commented+ 1 Bug confirmed on OSX 10.8.5 & Chrome 30
+ 1 confirm that the bug doesn't occur in Safari
Comment #5
nod_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 :)
Comment #6
hemantsharma CreditAttribution: hemantsharma commentedI am going to work on it.
Comment #7
floydm CreditAttribution: floydm commented@nod_ Attached is a snap of what I get when I click submit with javascript disabled on Chrome.
Comment #8
swentel CreditAttribution: swentel commentedAnd these are console errors on the add language page
Comment #9
nod_Seems like it's http://stackoverflow.com/a/12486761
Comment #10
hemantsharma CreditAttribution: hemantsharma commentedI am using mysql but the element displayed is "pgsql" one. Does anybody know how page decides which input field is made visible and hidden?
Comment #11
hemantsharma CreditAttribution: hemantsharma commentedLooks 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.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedIt 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.
Comment #13
nod_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.
Comment #14
evilehk CreditAttribution: evilehk commentedBy 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.
Comment #15
evilehk CreditAttribution: evilehk commentedIf 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
Comment #16
nod_See http://code.google.com/p/chromium/issues/detail?id=166549
Comment #17
YesCT CreditAttribution: YesCT commentedI have seen several new people trying to work on d8 bugs run into this.
Comment #18
evilehk CreditAttribution: evilehk commentedI 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!
Comment #19
alanburke CreditAttribution: alanburke commentedPatch 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.
Comment #20
hass CreditAttribution: hass commentedI 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.
Comment #21
nod_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.Comment #22
YesCT CreditAttribution: YesCT commentedI 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?
Comment #23
evilehk CreditAttribution: evilehk commented@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.
Comment #24
evilehk CreditAttribution: evilehk commentedComment #25
evilehk CreditAttribution: evilehk commentedAdded spacing between {} to adhere to Drupal coding standards. Third time's a charm!
Comment #26
evilehk CreditAttribution: evilehk commentedremoves variables only useful in debugging. Thanks again for the help in irc, nod_.
Comment #27
nod_Patch is not the usual format, there might be a problem for applying. Other than that, It looks good to me.
Comment #28
nod_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!
Comment #29
evilehk CreditAttribution: evilehk commentedComment #30
evilehk CreditAttribution: evilehk commentedApplies fixes to language add form. Latest patch and interdiff attached.
Comment #31
evilehk CreditAttribution: evilehk commentedComment #32
nod_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.
Comment #33
nod_title is too restrictive.
Comment #35
evilehk CreditAttribution: evilehk commentedGood 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.
Comment #36
nod_Perfect, thank you :)
Tested, does remove the attribute and puts it back as expected.
Comment #37
ndeet CreditAttribution: ndeet commentedCan confirm that the patch works on OSX 10.8.5 with Chrome Version 30.0.1599.101
Comment #38
alexpottTested the installer without Javascript. This patch actually fixes a bug due to the fact that both postgres and mysql credentials are required.
Comment #39
tim.plunkett:)
Comment #40
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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).
Comment #41
nod_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.
Comment #42
nod_Comment #42.0
nod_added chrome bug report link
Comment #43
nod_41: core-js-states-installer-2101427-41.patch queued for re-testing.
Comment #44
David_Rothstein CreditAttribution: David_Rothstein commented#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.
Comment #45
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #46
alexpottWell 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
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 :)
Comment #47
evilehk CreditAttribution: evilehk commentedIn 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.
Comment #48
evilehk CreditAttribution: evilehk commentedAdding #1372374: Adapt states.js to new required html5 attribute as a related issue
Comment #49
David_Rothstein CreditAttribution: David_Rothstein commentedYup.
In summary:
I assume you tested that version of Chrome with JavaScript turned on too? If so, want to mark this RTBC?
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.
Comment #50
evilehk CreditAttribution: evilehk commentedTested #41 in Chrome 30+ with javascript disabled as well as enabled. Marking as RTBC.
Comment #51
webchickAwesome work, folks!
Committed and pushed to 8.x. Thanks!
Comment #53
ianthomas_ukMy 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.
Comment #54
catchMoving to 'before release' for now, this wouldn't involve an API change.
Comment #55
xjmComment #56
catchOpened #2565171: Remove installer cruft for old version of chrome.