Problem/Motivation
Drupal 8 leverages the required
attribute for html inputs. There is a known issue with the Chosen library where it breaks this (https://github.com/harvesthq/chosen/issues/2075), which hasn't been fixed, but includes workarounds.
To reproduce, add a required multi-select field to a content type. Fill out all other required fields on the node add form. The form will not submit, but no direction is given to the user.
In Chrome, the user gets no feedback, and this error appears in the console log:
An invalid form control with name='field_test[]' is not focusable.
In Firefox, the message appears at the top of the browser window, rather than by the element:
Proposed resolution
We should wrap some JS around required fields to handle this via one of the workarounds.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#12 | interdiff_7-10.txt | 633 bytes | gambry |
#10 | chosen-required-patch_10.png | 26.02 KB | gambry |
#10 | 2705891-10.patch | 1.55 KB | gambry |
|
Comments
Comment #2
jhedstromComment #3
jhedstromIf https://github.com/harvesthq/chosen/pull/2594 or something similar goes in, we won't have to address this here.
Comment #4
danquah CreditAttribution: danquah at Reload commentedWhile we wait for a proper fix like https://github.com/harvesthq/chosen/pull/2594 I've attached a patch that rather crudely implements the fix @jhedstrom suggested in the github issue directly in the modules .js
Comment #5
danquah CreditAttribution: danquah at Reload commentedThe previous patch did not work in all cases, so here is a more simple patch that in case of a required multipe-selector removes the required attribute. This means you loose the client-side validation, but we will at least be able to submit the form.
This element will still be visually marked as required (red star), and will be highlited if it fails validation.
Again, this is a temporary workaround.
Comment #6
acbramley CreditAttribution: acbramley at PreviousNext for Transport for NSW commentedRerolled against latest dev.
Comment #7
gnugetI just create a patch which use the workaround suggested on: https://github.com/harvesthq/chosen/issues/515#issuecomment-104602031
This make it work without need to remove validation.
Comment #8
mpp CreditAttribution: mpp as a volunteer and at AmeXio commented#7 applies and rather 'brutally fixes' the issue :)
Comment #9
mpp CreditAttribution: mpp as a volunteer and at AmeXio commentedNote that the message appears a bit too low though. Edit: Probably because we have two consecutive elements that use the chosen widget.
Comment #10
gambry#9 No, I have 2 chosen fields in a row as well. I moved one and message still appears lower then it should.
That's due the select multiple height (or width in some cases) being spanning over its chosen widget.
Attached patch set select height and width to 0, and widget looks now correct to me.
I'm also setting this to NR as it has been postponed for too long, and all this time there hasn't been any light at the end of bug's tunnel.
And I think we are going to wait still a good while before issue is fixed on the library itself.
Attached also a screenshot proving the html5 validator works, without the need to skip it.
Comment #11
gnugetHi Gambry.
Thanks for your review, can you please provide an interdiff?
Thanks!
Comment #12
gambryYep, sorry. Got distracted and forgot.
Attached interdiff 7-10.
Comment #13
dpagini CreditAttribution: dpagini as a volunteer commentedWe've been using this patch for 4 months now and it's fixed this issue for us, setting to RTBC status.
Comment #14
gnugethi @dpagini.
Which pacth did you use? it seems that the workaround on #7 has a few bugs when used on multiple elements, and seems that #10 fixed this problem (I haven't tested it myself)
Also, not sure if we can just commit a workaround as a fix.
Maybe we can do it if we create a follow-up remind us to undo this change when the library is fixed?
Comment #15
dpagini CreditAttribution: dpagini as a volunteer commentedWas using #7 and updated to #10. I missed the few bugs with the "multiple elements" (didn't quite understand the comment completely). I have multiple chosen elements on the frontend, and the error message is showing up in an "acceptable" location for me (see screenshot). It's all the way to the left of the chosen select, but right in the middle. I have not testing on the backend. If I have a chance I'll enable it there and see how it works for me... I'll set back to Needs Review until I have the chance to do that or someone else reviews.
As for the committing a fix... I think it's silly to hold out for those fixes. Unless someone has the ear of the maintainer, there are so many PRs open for that library, it seems like it just doesn't get the active attention it needs. Just my $0.02.
Comment #16
Anas_maw CreditAttribution: Anas_maw as a volunteer commentedThis workaround not working for me.
Comment #17
Anas_maw CreditAttribution: Anas_maw as a volunteer commentedAfter long search i found that it's an issue with Drupal core, https://www.drupal.org/project/drupal/issues/1797438
The last patch works fine for me after applying core patch
Comment #18
Anas_maw CreditAttribution: Anas_maw as a volunteer commentedThis should be committed, waiting for chosen to fix this will not be the right choice.
Comment #19
DanieleN CreditAttribution: DanieleN commented+1 #18 (and Patch #10 with Core 8.5 works fine)
Problem with IE11Steps for reproduce:This is an important feature for using this module.Update:
Works in IE11 with Core Patch against HTML Validation:
https://www.drupal.org/project/drupal/issues/1797438 (current #128)
Comment #20
nagy.balint CreditAttribution: nagy.balint commentedIn #17 there is a core patch which seem to not be committed yet in 8.5
Is that not needed anymore?
Comment #21
sonfdPatch #10 is resolving issues for me.
Comment #22
mpp CreditAttribution: mpp as a volunteer and at AmeXio commentedAnyone willing to commit #10?
Comment #23
nagy.balint CreditAttribution: nagy.balint commentedThere is no response yet to #20.
If it needs a core patch to work then its a different story.
Comment #24
mpp CreditAttribution: mpp as a volunteer and at AmeXio commented@nagy.balint #1797438 describes an issue in Drupal core with HTML5 validation. The current direction is to totally remove HTML5 validation, which eventually makes this issue irrelevant. But another JavaScript based approach that keeps HTML5 validation is also discussed.
As a maintainer you should decide if you want to support HTML5 validation or not. If you do, I'd merge this patch, if you don't I'd refer to the core issue and suggest people to disable HTML5 validation as a whole.
Comment #25
nagy.balint CreditAttribution: nagy.balint commentedComment #17 says that the workaround did not work without the core patch.
Which is what i asked in #20.
Comment #26
mpp CreditAttribution: mpp as a volunteer and at AmeXio commentedYour answer to #20 is no, the core patch is not needed.
If one does apply the core patch, HTML5 validation is disabled. This would make this issue (to support HTML5) irrelevant.
Comment #28
nagy.balint CreditAttribution: nagy.balint commentedI guess we can add this temporarily.