When in a form where required fields appear in a Vertical Tab, if a field that is required isn't filled in, or skipped by accident, and another tab is accessed, the tab with the validation error is not open, which can sometimes lead to confusion.
I've created a patch that will check for the first element it finds that is required and is invalid, and selects the correct tab, focusing it. I've tested this out and it seems to work well for the scenario.
Reproduce
To reproduce, you need to have a field inside a vertical tab that can fail validation. Usually, required and similarly validated fields are not inside tabsets in core
The easiest way to reproduce is apply 2911932-30-test-only.patch from #30, enable form_test
(must have
$settings['extension_discovery_scan_tests'] = TRUE;
).
Then go to form-test/group-vertical-tabs and enter "bad" into the input in "Second group element", go to an different tab, then submit to trigger an error.
It not working === it does not switch to second tab, where the error is
Problem Happening
https://youtu.be/kYnLIIWCQes
Problem Fixed
https://youtu.be/Ydr06gybiFI
Note that this is specific to the Vertical Tabs render element provided by Drupal core. Any issues occurring in contrib elements, such as those provided by Field Group, will need to be filed as issues with the contirb module providing them.
Comment | File | Size | Author |
---|---|---|---|
#49 | 2911932-49.patch | 6 KB | Gauravvvv |
#30 | 2911932-30.patch | 4.3 KB | bnjmnm |
| |||
#30 | 2911932-30-test-only.patch | 2.58 KB | bnjmnm |
#22 | 2911932-22.patch | 6.08 KB | bnjmnm |
#21 | 2911932-20-test-only.patch | 4.02 KB | bnjmnm |
Issue fork drupal-2911932
Show commands
Start 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:
- 2911932-tab-error compare
- 2911932-correct-vertical-tab changes, plain diff MR !2266
Comments
Comment #2
mlncn CreditAttribution: mlncn at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedComment #3
mlncn CreditAttribution: mlncn at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedComment #4
CKIDOW+1
Comment #5
nod_Would have bet we had something for it already.
The patch is a little too specific. I would use something like
With the appropriate tests all around this to make sure the field is in a vertical tab.
Comment #7
EthanTOriginal patch didn't grab the tab (was failing on the .length check). Modified a bit and is now working as expected.
Comment #8
EthanTComment #9
EthanTComment #10
EthanTComment #14
hooroomooComment #16
klonosI've tried to test this in latest D8.9.20 and D9.3.5, but did not see any change. Any vertical tab that is not the default/focused that has required fields does not get any focus whatsoever. Things are made worse by the fact that the default/native browser errors do not trigger any validation error summary at the top of the page, so it appears as though as clicking the submit button does nothing (when in fact the native validation error is triggered "in the background", but not shown because the vtab that contains the field is not the one that's focused).
Comment #18
bnjmnmComment #20
bnjmnmComment #21
bnjmnmSyntax error in drupalci.yml
Comment #22
bnjmnmFix + the test from #21 which should hopefully pass now.
Comment #24
bnjmnmAdding some tags to make this more popular
Comment #26
arnaud-brugnon CreditAttribution: arnaud-brugnon commented#22 can't be apply on Drupal 10
Comment #27
arnaud-brugnon CreditAttribution: arnaud-brugnon commentedComment #28
arnaud-brugnon CreditAttribution: arnaud-brugnon commentedEven if i improve #22, it does not work for Drupal 10 (I am not sure to even understand what does #22 suppose to do).
If someone want to unlock validation, i improve #7 for Drupal 10 and add support for textarea.
But i don't think it's the right direction to take.
It seems to specific (and it won't work if form element is not input, select or textarea).
Anyway, enjoy the improvement
Comment #29
arnaud-brugnon CreditAttribution: arnaud-brugnon commentedBy the way, here's the improvements for #22 if it can help.
But it's weird.
Most of the code was already in the core and like es6 support is gone, the patch is just about moving a behavior (which does not work at all)
Comment #30
bnjmnm@arnaud-brugnon #22 provides intentionally failing tests that prove the problem exists and #24 gets those tests to pass, which proves the solution is effective. That should at least suggest the current solution is effective enough to not require a complete rewrite. Your patches completely remove the test coverage, which is not at all productive.
There's no need to target specific element types. There are selectors added via Drupal's Form API that can be targeted.
Patch #7 is a solution based on the assumption that Drupal uses HTML5 form validation. It would not work at all for server side validation as the logic is triggered on submit before any of that server side validation is run.
No.es6 files are no longer needed in Drupal 10 as the supported browsers all support modern syntax. Yes, this patch is about moving a behavior that successfully addressed an issue with CKEditor 5 - it's being moved so all Vertical tabs can benefit from the fix, not just those used in CKEditor 5 config, hence the
@todo Remove when https://www.drupal.org/project/drupal/issues/2911932 lands.
comment in the CKEditor5 JS.The test coverage provides objective proof that there was a problem and that patch #22 fixes it. Perhaps there are use cases that have not yet been accounted for, and details regarding how to reproduce those conditions would be helpful.
Comment #32
arnaud-brugnon CreditAttribution: arnaud-brugnon commentedOkay.
Thanks for all the explanation.
Comment #33
arnaud-brugnon CreditAttribution: arnaud-brugnon commented#30 works like a charm
Comment #34
mgiffordWCAG tagging.
Comment #35
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
Let me know if I'm testing this wrong
Installed Drupal 10.1.x with a standard profile
Installed field_group module
In the article content type setup a tabs group with 2 tabs A and B.
Moved a required field into the B tab and a non required field into the A tab
Created an article not filling in the required field in tab B
When I click save tab B is displayed with the required field in red showing as required.
Comment #36
vija CreditAttribution: vija commentedI've deleted my comment as I figured out that my problem was problem with field_group tabs
Comment #37
crutch CreditAttribution: crutch commentedoriginalThank you for explanation and information #38. I didn't realize field_group was the problem but it seems to be. This issue fixed it for us. Even though it results in resolving "Please fill out this field" errors first then core errors afterward, it should work fine.
Comment #38
bnjmnmre #37 testing should never require installing a contrib module like field_group. Any issue fixed in core should be reproduceable with just core, otherwise its an issue that should be filed with the contrib module. In this case it is a confirmed problem.
To reproduce, you need to have a field inside a vertical tab that can fail validation. Usually, required and similarly validated fields are not inside tabsets in core
The easiest way to reproduce IMO is apply 2911932-30-test-only.patch from #30, which already reproduces the problem for the purposes of testing. Enable the
form_test
module (must have).
Then go to form-test/group-vertical-tabs and enter "bad" into the input in "Second group element", go to an different tab, then submit to trigger an error.
It not working === it does not switch to second tab, where the error is
Problem Happening
https://youtu.be/kYnLIIWCQes
Problem Fixed
https://youtu.be/Ydr06gybiFI
Re #37
That makes sense as patches are built for the most recent dev branch, which is currently 10.1.x. This can probably be made into a 9.x patch pretty easily - if you have success with that perhaps upload it here for users who may also need it.
Comment #39
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks for the clarity @bnjmnm and the youtube links!
Did as you suggested and used the form_test module for testing.
Patch works great!
Comment #45
lauriiiCommitted 8f9e0bd and pushed to 10.1.x. Thanks!
Triggered 10.0.x tests for backport.
Comment #48
lauriiiCherry-picked to 10.0.x. Still need a separate patch for 9.5.x backport.
Comment #49
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedI have attached the re-rolled patch for 9.5.x
Comment #50
smustgrave CreditAttribution: smustgrave at Mobomo commentedReroll seems fine.
Comment #52
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeems random
Comment #54
lauriiiCommitted 5b7e07a and pushed to 9.5.x. Thanks!
Comment #56
sbrenner02 CreditAttribution: sbrenner02 commentedI'm using a module that makes the log message field in the 'information' tab of drupal core required ( https://www.drupal.org/project/require_revision_log_message ) and ran into this issue. Was hoping that this patch would address the issue but it does not seem to. If anyone has the capacity to assist that would be appreciated