Problem/Motivation
Inline Form Errors replaces the error messages in the summary block at the top of the page with links to the invalid fields.
For errors inside details and vertical tabs, clicking the error link opens the details / vertical tab and focuses the invalid field.
For errors in Default Admin's closed sidebar, clicking the error link does nothing. The sidebar remains closed, and the invalid field remains hidden.
I think this causes failure of WCAG SC 3.3.3 Error Suggestion (Level AA).
Inline Form Errors moves the descriptive error messages with suggestions to the invalid fields. If users can't find the invalid fields, they also won't find the detailed error messages.
Related:
The closed sidebar provides no indication that it contains invalid fields. Working on that in #3619387: Indicate on forms sidebar toggle button that sidebar has child element errors.
Steps to reproduce
- Set Default Admin as the admin theme.
- Install the article test recipe (
drush recipe core/tests/fixtures/recipes/article_content_type),
or create an Article content type & install the Path module (akapath_alias). - Enable the Inline Form Errors module.
- Go to
/node/add/article. - Reduce viewport width until the sidebar on the right containing the "advanced" fields collapses.
- Click the toggle button to expand the sidebar.
- Enter a value without a beginning slash in the
URL aliasfield. - Close the sidebar.
- Click the
Savebutton.
This should cause a validation error at the top of the page, something like the following with "URL alias" being a link:
"1 error has been found:
URL alias". - Click the "URL alias" link in the error message.
Expected
- The sidebar is opened.
- Focus moves to the invalid
URL aliasfield.
Actual
Nothing happens when the "URL alias" link is clicked.
Proposed resolution
Listen for formFragmentLinkClickOrHashChange events and open the sidebar, as with details elements.
Remaining tasks
Create merge request.Review.- Make the decision regarding jQuery (frontend framework manager or higher?).
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #29 | after-3-result-of-clicking-url-alias-link.png | 93.22 KB | ironnuts |
| #29 | after-2-url-alias-link-generated.png | 62.64 KB | ironnuts |
| #29 | after-1-add-url-alias-in-sidebar.png | 92.41 KB | ironnuts |
Issue fork drupal-3619127
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:
Comments
Comment #2
kentr commentedComment #3
kentr commentedComment #4
kentr commentedComment #5
kentr commentedComment #6
kentr commentedIt's not just the "mobile" version.
Comment #7
kentr commentedWorking on it.
Comment #8
mgiffordI haven't had a chance to walk through this, but this sounds like an important issue to resolve.
Comment #9
kentr commentedI'm pretty close to an MR. Just working on the test.
Comment #10
kentr commentedComment #12
kentr commentedTagging with 11.5.0 release priority because IFE is 11.5.0 release priority and this was declared as a blocker for IFE in #3576488: [meta] Admin theme: path to beta and stable:
Comment #13
smustgrave commentedcomment on the MR.
Comment #14
kentr commentedThanks @smustgrave. I made the change per your comment.
Comment #15
kentr commentedActually, I didn't create a base class.
Kick it back with details on what you're thinking if you feel strongly about that.
Comment #16
smustgrave commentedThanks for humoring me. Think this is fine for now but do think once we add more (I imagine there will be plenty more as the new admin theme) we should get a AdminTestBase going to avoid duplicate code. But this looks good.
Comment #17
longwaveAdded some feedback to the MR.
Comment #18
kentr commentedWorking on the review comments.
Comment #19
kentr commentedMade some changes per review comments.
Comment #20
kentr commentedMissed one of the review comments.
Comment #21
kentr commentedComment #22
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #23
kentr commentedWorking on it.
Comment #24
kentr commentedRebased and green pipeline.
Comment #25
ironnuts commentedTo start off a review, I ran the test-only manual test https://git.drupalcode.org/issue/drupal-3619127/-/jobs/12002577.
That seems to confirm test coverage to prove the fix is in place. I think a manual test would be useful as steps to reproduce are nice and clear.
Comment #26
ironnuts commentedOn the other hand, all the comments in the MR have been addressed. The code has been kicked into shape by some useful comments. I will try RTBTC. But ready to concede if someone deems e.g. a manual test and screenshots necessary.
Comment #27
ironnuts commentedComment #28
smustgrave commentedIt should be tested.
Comment #29
ironnuts commentedComment #30
ironnuts commentedHave followed the steps to reproduce to manually test the fix. I confirmed that nothing happens when you follow the steps then click the 'url-alias' link without the fix. With the fix the focus shifts to the sidebar url alias field as it should. I have uploaded 3x screenshots showing the behaviour after the fix. When nothing happens it is a bit pointless making a screenshot of that. The second photo shows that that stage anyway.
Comment #31
ironnuts commentedComment #32
larowlan@mherchel's question about jquery isn't resolved - I'll ping him
Comment #33
kentr commented@larowlan I replied to your MR comment from 9/11.
Comment #34
ironnuts commentedAll comments have been responded to. Manual testing has been completed. RTBTC.
Comment #35
ironnuts commentedComment #36
kentr commentedSadly, not yet 😭...
The comments have been responded to, but the concerns aren't resolved.
To be resolved, the responses have to satisfy the concerns (in this case, raised by high-level core maintainers).
Comment #37
ironnuts commentedOk, good point.
Comment #38
kentr commentedBut maybe another frontend framework manager will be available sooner to examine the responses and make the call regarding jQuery.
Comment #39
kentr commentedComment #40
kentr commentedSorry, I misremembered this to be a blocker for #3576488: [meta] Admin theme: path to beta and stable.
I don't want to take FEFMs away from D12 beta blockers. Perhaps it can wait until @mherchel is available.