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

  1. Set Default Admin as the admin theme.
  2. Install the article test recipe (drush recipe core/tests/fixtures/recipes/article_content_type),
    or create an Article content type & install the Path module (aka path_alias).
  3. Enable the Inline Form Errors module.
  4. Go to /node/add/article.
  5. Reduce viewport width until the sidebar on the right containing the "advanced" fields collapses.
  6. Click the toggle button to expand the sidebar.
  7. Enter a value without a beginning slash in the URL alias field.
  8. Close the sidebar.
  9. Click the Save button.
    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".
  10. Click the "URL alias" link in the error message.
Expected
  1. The sidebar is opened.
  2. Focus moves to the invalid URL alias field.
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

  1. Create merge request.
  2. Review.
  3. Make the decision regarding jQuery (frontend framework manager or higher?).

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3619127

Command icon 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

kentr created an issue. See original summary.

kentr’s picture

Issue tags: +Usability
kentr’s picture

Issue summary: View changes
kentr’s picture

Issue summary: View changes
kentr’s picture

Issue summary: View changes
Issue tags: +wcag333
kentr’s picture

Title: Mobile forms sidebar doesn't work with Inline Form Errors module » Forms sidebar doesn't work with Inline Form Errors module

It's not just the "mobile" version.

kentr’s picture

Working on it.

mgifford’s picture

Priority: Normal » Major

I haven't had a chance to walk through this, but this sounds like an important issue to resolve.

kentr’s picture

I'm pretty close to an MR. Just working on the test.

kentr’s picture

Title: Forms sidebar doesn't work with Inline Form Errors module » Forms sidebar doesn't work with error links from Inline Form Errors module

kentr’s picture

Title: Forms sidebar doesn't work with error links from Inline Form Errors module » Forms sidebar doesn't open with error links from Inline Form Errors module
Issue summary: View changes
Status: Active » Needs review
Issue tags: +11.5.0 release priority

Tagging 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:

#3619127: Forms sidebar doesn't open with error links from Inline Form Errors module

  1. There appears to be a dependency on the other issue that KentR is working on. Don’t feel this is a blocker since IFE is not enabled by default
  2. [Mike G] I do see this as being a blocker if we want to have IFE in core and finally moving from an experimental module. Core is changing a lot though, so….
  3. [Mike H] It’s a blocker for IFE in core, but not Admin theme being stable
smustgrave’s picture

Status: Needs review » Needs work

comment on the MR.

kentr’s picture

Status: Needs work » Needs review

Thanks @smustgrave. I made the change per your comment.

kentr’s picture

Actually, I didn't create a base class.

Kick it back with details on what you're thinking if you feel strongly about that.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks 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.

longwave’s picture

Status: Reviewed & tested by the community » Needs work

Added some feedback to the MR.

kentr’s picture

Working on the review comments.

kentr’s picture

Status: Needs work » Needs review

Made some changes per review comments.

kentr’s picture

Status: Needs review » Needs work

Missed one of the review comments.

kentr’s picture

Status: Needs work » Needs review
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new98 bytes

The 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.

kentr’s picture

Issue tags: +Needs rebase

Working on it.

kentr’s picture

Status: Needs work » Needs review
Issue tags: -Needs rebase

Rebased and green pipeline.

ironnuts’s picture

To 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.

ironnuts’s picture

On 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.

ironnuts’s picture

Status: Needs review » Reviewed & tested by the community
smustgrave’s picture

Status: Reviewed & tested by the community » Needs review

It should be tested.

ironnuts’s picture

ironnuts’s picture

Have 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.

ironnuts’s picture

Status: Needs review » Reviewed & tested by the community
larowlan’s picture

Status: Reviewed & tested by the community » Needs review

@mherchel's question about jquery isn't resolved - I'll ping him

kentr’s picture

@larowlan I replied to your MR comment from 9/11.

ironnuts’s picture

All comments have been responded to. Manual testing has been completed. RTBTC.

ironnuts’s picture

Status: Needs review » Reviewed & tested by the community
kentr’s picture

Status: Reviewed & tested by the community » Needs review

Sadly, 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).

ironnuts’s picture

Ok, good point.

kentr’s picture

But maybe another frontend framework manager will be available sooner to examine the responses and make the call regarding jQuery.

kentr’s picture

Issue summary: View changes
kentr’s picture

Sorry, 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.