If you have an empty required field within a collapsed details element, when you submit the form it appears as nothing at all happens.
If you open up chrome Devtools, you will see the following error.
An invalid form control with name='name_of_field'
Open the details element for invalid fields being targeted via JS
File: core/misc/details.js
$('details input[required="required"]').on('invalid', (e) => {
let $element = $(e.target).parent();
handleFragmentLinkClickOrHashChange(e, $element);
});
Respond to #19
Resolve comments in the MR
N/A
N/A
N/A
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 11x-screenshot-error.png | 299 KB | phthlaap |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
mdolnik commentedHere is a proposed addition to
collapse.jswhich adds a validation on submission to open up the collapsed field group with the invalid field, resulting in no JS error and the problem field being highlighted for the user.This is based off of 2787179-highlight-html5-validation-74.patch from comment 74 in Field Group's Ensure visibility of invalid fields (JS) issue but is applied to all collapsible details elements instead of just ones built by Field Groups.
Comment #3
cilefen commentedDrupal 8 is end of life. Is the bug reproducible on a supported release?
Comment #4
alieffring commentedI can confirm that this issue is still happening in Drupal 9, and that the general idea of the patch (registering a listener for validation issues and opening parent details tags when triggered) fixes it.
Comment #5
galileo25 commentedThis patch works for Drupal 9.5.x version
Comment #6
dmitry.korhovComment #7
galileo25 commentedComment #8
galileo25 commentedComment #9
dmitry.korhovre-roll on drupal 9.5.x branch
Comment #10
dmitry.korhovre-compile .js from es6.js
Comment #11
smustgrave commentedAs a bug will need a test case.
Will also need to verify this is a bug in 11.x branch and patch made for that as that's the current development branch.
Comment #12
tregonia commentedNoticed that this patch is using jQuery.once and not Drupal once. Should this issue include using Drupal once?
https://www.drupal.org/node/3158256
Comment #13
tregonia commentedLooks like I spoke too soon before diving in. The code in this issue is against 9.5.x and affects a file that was removed in `10.0.x` and `10.1.x`.
See https://www.drupal.org/project/drupal/issues/3269082 for more details.
Next steps are to reroll this patch against D10, or at least D11
Comment #14
phthlaap commentedComment #16
phthlaap commentedComment #17
phthlaap commentedComment #18
phthlaap commentedFixed the issue on 11x branch and create merge request. Added a tests.
Comment #19
smustgrave commentedRemoving tests tag as they have been added
This covers details but wonder if a future ticket could make it more dynamic in determining if it's hidden.
Comment #21
quietone commentedThanks for working on this improvement!
I'm triaging RTBC issues. I read the IS and the comments. There is a question in #19 that has not been answered.
I then tested the MR, with and without the fix and it works as expected. I then reviewed the MR and left comments there some of which require work. I have used the suggestion feature but they are just suggestions. Whoever works on this may have better ideas.
I am changing the status to NW.
Comment #22
phthlaap commentedComment #23
smustgrave commentedLeft some small comments.
Comment #24
phthlaap commentedComment #25
smustgrave commentedFeedback has been addressed.
Comment #26
nod_Couple of frequent usage case not supported (detail with required field added with ajax, changing the required state with the #states API)
Comment #27
nod_Posted a solution that should work all the time.
I used a native event so I can listen in the capture phase that way opening the details element is the very first thing that runs, before the native browser callbacks are called, otherwise it causes an error with the details element being open too late.
So use my suggestion and add some more comments please.
Comment #28
phthlaap commentedResolved and added more tests to cover form with ajax.
Comment #29
smustgrave commentedApplied 1 nitpicky suggestion and cleaned up the issue summary.
Changes look good though
Comment #30
longwaveThis is a minor behaviour change but I think it is eligible for backport as fixing the bug is worse than leaving it in place.
Committed and pushed a31d15a87f to 11.x and 5663d5dd9c to 10.3.x and 4fbd912d57 to 10.2.x. Thanks!