Inline Form Errors not compatible with Quick Edit

Is there a design or UX guideline for how we want this to be? I rather like how in one of your screenshots, the error message ends up inside the quick edit dialog. I think it's a nice way to keep the underlying page as wysiwyg as can be, and be less dependent on front end theming.

Running this on 8.3.x : this is what I can see :

Which seems better that what was found originally when this issue was opened.

Jumping on what @marcvangend said at #3, here is a patch that moves the error message inside the quick edit box.

See patch 2828092-error-message-inside-quickedit-box.patch

I am sorry for my lack of feedback on this issue. I'll make sure to double-check my account's settings in order to receive email notifications (which I obviously didn't receive).

@dmsmidt Do you need me to upload the patch as-is to #2831546 or do you wish me to make some modifications first ?

Reviewed 2828092-10-ife_quick_edit.patch and behaviour looks like the expected one.

Of course, rises #2831546 that needs to be fixed.

According to feedback from the product and UX team on #2504847: [meta] Roadmap for stabilizing Inline Form Errors module (IFE), disabling IFE for Quick Edit is an acceptable workaround. However, we should also consider a postponed followup issue to better integrate Quick Edit and IFE. That followup issue would not be a blocker for IFE, just a future task for better user experience.

Thanks everyone!

This should update the IS with before/after screenshots. Because the screenshots currently show a solution that doesn't match the proposed solution.

1. +++ b/core/modules/inline_form_errors/src/FormErrorHandler.php
   @@ -53,6 +53,13 @@ public function __construct(TranslationInterface $string_translation, LinkGenera
   +    if ($form['#form_id'] == 'quickedit_field_form') {
   

   ===

2. +++ b/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php
   @@ -127,4 +129,34 @@ public function testSetElementErrorsFromFormState() {
   +  /**
   +   * Test that Quick Edit forms show non-inline errors.
   +   *
   +   * @covers ::handleFormErrors
   +   * @covers ::displayErrorMessages
   +   */
   +  public function testDisplayErrorMessagesNotInlineQuickEdit() {
   

   This is the sort of thing that really needs a functional test, not a unit test. A unit test doesn't give enough guarantees here, because it's only the integration of many things (form system, Quick Edit, user interactions) that can prove this works as intended.

+++ b/core/modules/inline_form_errors/src/FormErrorHandler.php
@@ -53,6 +53,13 @@ public function __construct(TranslationInterface $string_translation, LinkGenera
+    if ($form['#form_id'] == 'quickedit_field_form') {

I was wondering whether it wouldn't be nicer to have a sort of $form['#disable_inline_form_errors'] = TRUE flag or something like that. That way Quickedit could set that on its form, but it would also allow contrib modules that run into similar issues to do the same. And it wouldn't introduce any knowledge about Quickedit in inline_form_errors.

No, that's absolutely legitimate. Nice that there's already a follow-up for that.

Already has the "Needs tests" tag

For whoever does write the test, please use BrowserTestBase or JavascriptTestBase, and not WebTestBase.

@dmsmidt - Can you upload your test anyway? I'm travelling at DrupalCamp Iceland just now, but I can probably find time to try running your test,

Test failure for patch #38:

There was 1 error:

1) Drupal\Tests\inline_form_errors\Unit\FormErrorHandlerTest::testDisplayErrorMessagesNotInlineQuickEdit
Undefined index: #array_parents

/var/www/html/core/lib/Drupal/Core/Form/FormErrorHandler.php:132
/var/www/html/core/lib/Drupal/Core/Form/FormErrorHandler.php:23
/var/www/html/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php:170

I think we need to declare #array_parents in FormErrorHandlerTest::testDisplayErrorMessagesNotInlineQuickEdit(), like we do in the other tests in the same class?

Ooops, we're both online at the same time. Let's se if htat failure persists with #41

Patch #41 has:

+  public static $modules = [
+    'contextual',
+    'quickedit',
+    'node',
+    'inline_form_errors',
+  ];

Does contextual need to be here? I is a dependency of quickedit so would be enabled anyway.

This patch:

• removes contextual module from setUp in FormErrorHandlerQuickEditTest, because it's a dependency of quickedit
• spilts users permisssions onto separate lines, there are 5 of them
• adds #array_parents => [] to the test form in FormErrorHandlerQuickEditTest.php::testDisplayErrorMessagesNotInlineQuickEdit()

Scheduling test against 8.3.x for good measure too.

I think we should have before/after shots for this.

Does this still need an issue summary update?

i've added an email field to basic article type
after activation 'Quick Edit' contextual link and then going down and clicking on Email filed. the phrase 'email ->gfdghfd' ( gfdghfd is the node title.) remains inplace, then the field will not validate.

i've rerun this without IFE enabled, and it produces the same error. so, there seems to be some other break with Edit. still, I'm noting this manual test.

yes. the patch does make Edit and IFE work together. The IFE message appeared without an inner hash link, as expected.

Awesome, so it seems there is nothing left here, right? Patch still looks good to me.

+++ b/core/modules/inline_form_errors/src/FormErrorHandler.php
@@ -53,6 +53,13 @@ public function __construct(TranslationInterface $string_translation, LinkGenera
+    // the errors are already near the form element.
+    if ($form['#form_id'] == 'quickedit_field_form') {
+      parent::displayErrorMessages($form, $form_state);
+      return;
+    }

I think it's OK for the patch to go in like this, but inline_form_errors should eventually be not-a-module whereas quickedit is going to stay one. So I think we need a follow-up to remove the hard-coded dependency.

Was going to suggest a .yml file with a list of form IDs, then we just check in_array()/isset() rather than hard-coding knowledge of other module's forms here. Or a custom $form['#property'] to check would be more lightweight.

#2848319: Find a way to make it easier to hide Inline Form Error messages on child elements was suggested as the issue for that, but not sure that's about this issue at all? Looks like it was talking about other bugs which are already fixed, might just need re-scoping though.

Might just need an issue summary update but it's not clear to me what the next steps are here.

Thanks everyone! Great to see this RTBC. +1 for the followup; I agree with the goal of hotfixing this for 8.3.x and then coming up with a resilient and complete fix in 8.4.x. I've added the new followup to the overall roadmap as well.

I think this issue needs both usability and product manager signoff, given that it's the primary product concern for IFE and affects product integrity. Leaving at RTBC, because the product team join weekly usability meetings where they could review this as a group. I will let them know about the issue.

Oh, since IFE is still alpha (though close to beta I think), we can also make this change during 8.3.x's RC.

Also, for what it's worth, the solution in this patch looks like an acceptable hotfix to me so long as we address the followup later (probably before we mark IFE beta, although I put it under RC on the roadmap). So if the product team agrees, I think we are on track for 8.3.0 with this fix!

It says tests failed but they did not; setting back to RTBC and retesting.

I reviewed and compared situations with and without the patch. Ran into confusing behaviour of Quick Edit as captured in #2671202: [regression] Text field's label is duplicated in the value when edited & saved using Quick Edit but that's QE's problem. Updated the issue summary a bit. IFE's part in showing sane error messages in Quick Edit scenarios seems to be covered here. Thanks @dmsmidt.

Thanks @yoroy!

Since @yoroy was also involved in the usability and product review that prioritized this hotfix originally, I think that's sufficient signoff here and I overdid it with both tags. :)

@catch's review in #58 has also been addressed with #2856950: Add a possibility to disable inline form errors for a complete form.

It looks like #25.1 was never addressed though (using ===). Let's fix that and then get this in!

#72 is good. I made an interdiff 45-72 locally to confirm it.

Using === was the only outstanding issue in #68, so back to RTBC.

Tested the patch and Drupal 8.3.0 RC1 without the patch. Looks like a clear improvement and also in line with what we discussed to be done at the UX meeting. Quick Edit is not changed when enabling Inline Form Errors because Quick Edit already displays errors near where the error is. Quick Edit itself needs to be made more accessible but IFE cannot help with that since the premise of displaying the error on the form element is not possible given Quick Edit removes the form element when you submit the form by definition.

Looks good from a product manager standpoint.

Updated screenshots.

Ps. this is my first Product Manager review. In reference to https://groups.drupal.org/node/516400

Committed and pushed ade9d78 to 8.4.x and f9925e7 to 8.3.x. Thanks!

All the feedback has been addressed and followups filed. Nice work everyone.

I think we should mention this in the release notes, for two reasons:

1. It's worth knowing that it's for now a design behavior for IFE not to happen with Quick Edit and that this opt-out will be offered for other forms later.
2. It's also the last big requirement to keep IFE in core for this minor! Two other issues are still outstanding, but since this was the biggest remaining concern identified from the product perspective, I think we've accomplished what we need to keep the module in core for this minor. Thank you all! I will also follow up on the roadmap and tag that issue as well, so we can highlight the improvements to IFE in the 8.3.0 release notes.

So exciting to see this green :)

Congratulations and gratitude to all of you! Great effort.

I am still not getting errors , printed alongside quick edit.

@rajeevku - we closed this issue before 8.3.0 was released, and the 8.4.0 release is expected soon. If you can reproduce you problem with 8.4.0-beta1, can you provide more detail in a new issue please?

Restoring previous tags - I'll still use these to find old accessibility issues after closing them.