No error messages are shown for applied validation on a view exposed filter with on "AJAX"

Proof of concept, this works, but not sure about the IEF part. Always the question where to put code that is integrating two modules, but right now, without that, you get duplicated messages and they are also quite challenging to theme inside the typical exposed form, not a lot of room. And forms tend to be short and typically on top of the page.

Also, the IFE thing also happens on non-ajax views, so that could be fixed separately from this. Didn't find an existing issue for views + IEF though.

Screenshots. No screenshot when ife is disabled, but then you simply get nothing until the refresh.

HEAD, with IFE enabled:

HEAD, after page refresh (with and without IFE)

With patch, without the IFE exclude, also HEAD without ajax

With patch including IFE exclude.

While printing the messages makes sense to me, I'm just curious how it looks when there are existing messages on the page and you get a new error message. Can we somehow use the Javascript Messaging API #77245: Provide a common API for displaying JavaScript messages ?

The IFE fix feel a bit like a band-aid, it seems that IFE + AJAX is not 100% (just getting this from the comments by @dmsmidt in the above issue, didn't do any research to verify this). But as a band-aid it makes sense...

I don't feel like this is the issue to start using that API? I just replicated the existing default ajax behavior, when you only return a return array then messages are automatically prefixed, exactly like this. See \Drupal\Core\Render\MainContent\AjaxRenderer::renderResponse(). It's only when you return an ajax response that you need to take care of that yourself.

Yes, the IFE exclude is certainly a band-aid, but ajax is not the only problem. The problem is that views exposed forms have this really weird behavior where they store label information in a separate structure and only set them in pre_render on the form elements, then IFE sees no label and bails out in \Drupal\inline_form_errors\FormErrorHandler::displayErrorMessages(). If the label would be set, you would instead see the usual "2 errors found on: LabelA, LabelB" message on top, not the duplicated validation error.

And IMHO even if that would work, it wouldn't make that much sense in this context, so I think opting out makes sense. Then again, I proposed the same for the login form which is IMHO weird with IFE, but wasn't able to convince others :)

For what it's worth, Berdir's patch in #5 is working well for me on 8.7.11.

Re-rolled for D8.9.x

I've re-rolled patch for 9.1

Works really well, just needs tests! I'll look to add some tomorrow.

Working on tests

Here's #12 with a test and test-only patch (also the interdiff)

1. +++ b/core/modules/views/src/Controller/ViewAjaxController.php
   @@ -200,6 +201,14 @@ public function ajaxView(Request $request) {
   +          $response->addCommand(new PrependCommand(".js-view-dom-id-$dom_id", $output));
   

   Micro-optimisation:

   Was wondering if sprintf() can be used to add $dom_id

Test-only failed as expected. Back to NR

Looks great, just a few questions re: unrelated changes.

1. +++ b/core/modules/views/src/Controller/ViewAjaxController.php
   @@ -136,17 +137,17 @@ public function ajaxView(Request $request) {
          foreach ([
   -          'view_name',
   -          'view_display_id',
   -          'view_args',
   -          'view_path',
   -          'view_dom_id',
   -          'pager_element',
   -          'view_base_path',
   -          AjaxResponseSubscriber::AJAX_REQUEST_PARAMETER,
   -          FormBuilderInterface::AJAX_FORM_REQUEST,
   -          MainContentViewSubscriber::WRAPPER_FORMAT,
   -        ] as $key) {
   +        'view_name',
   +        'view_display_id',
   +        'view_args',
   +        'view_path',
   +        'view_dom_id',
   +        'pager_element',
   +        'view_base_path',
   +        AjaxResponseSubscriber::AJAX_REQUEST_PARAMETER,
   +        FormBuilderInterface::AJAX_FORM_REQUEST,
   +        MainContentViewSubscriber::WRAPPER_FORMAT,
   +      ] as $key) {
   

   Formatting changes?

2. +++ b/core/modules/views/src/Form/ViewsExposedForm.php
   @@ -34,7 +34,7 @@ class ViewsExposedForm extends FormBase {
   -   * Constructs a new ViewsExposedForm
   +   * Constructs a new ViewsExposedForm.
   
   @@ -86,7 +86,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -    // Check if the form was already created
   +    // Check if the form was already created.
   

   Maybe also out of scope?

Yeah let's remove those changes.

+++ b/core/modules/views/src/Form/ViewsExposedForm.php
@@ -145,6 +145,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    // Labels are built too late for inline form errors to work, resulting
+    // in duplicated messages.
+    $form['#disable_inline_form_errors'] = TRUE;

Looks good to me. I've read @Berdir's comment on opting out here. Not sure how it was managed elsewhere, but it seems fine that if someone needed this feature a follow-up could be opened to address it.

Here's the 8.9 patch.

1. +++ b/core/modules/views/src/Form/ViewsExposedForm.php
   @@ -149,6 +149,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    // Labels are built too late for inline form errors to work, resulting
   +    // in duplicated messages.
   +    $form['#disable_inline_form_errors'] = TRUE;
   

   Do we need test coverage for this?

2. +++ b/core/modules/views/tests/src/FunctionalJavascript/ExposedFilterAJAXTest.php
   @@ -151,4 +165,17 @@ public function testExposedFiltersInModal() {
   +    $this->assertSession()->assertWaitOnAjaxRequest();
   

   Can we use something other then assertWaitOnAjaxRequest()? Seems like we can use waitForElement here which is less brittle.

How can we test #24.1?
I agree #24.2 can be changed.

+++ b/core/modules/views/tests/src/FunctionalJavascript/ExposedFilterAJAXTest.php
@@ -151,4 +165,17 @@ public function testExposedFiltersInModal() {
+    $this->assertSession()->assertWaitOnAjaxRequest();
+    $this->assertSession()->elementExists('css', 'div[aria-label="Error message"]');

These two can be combined and waitForElement('div[aria-label="Error message"]') can be used.

Hi @jibran
Updated patch please review.

Thanks!

+++ b/core/modules/views/tests/src/FunctionalJavascript/ExposedFilterAJAXTest.php
@@ -123,7 +137,7 @@ public function testExposedFiltersInModal() {
-    $this->assertSession()->assertWaitOnAjaxRequest();
+    $this->assertSession()->waitForElement();

Please revert this change.

On second thought, I don't think we need tests for #24.1.

Reverted changes from patch #26 as mentioned in #27, please review.

Fixing test case failure issue in #30, please review.

Great!

I read the Issue Summary and skimmed the issue and the patch. I think the proposed resolution in the IS should match the solution in the patch, and it doesn't seem like it does. The IS contains a before screenshot, does it need an after screenshot? The IS says there is an issue for Drupal 7, is there any recommendation here for helping to finish that issue? Adding tag for updating the IS based on those questions.

And during my skim of the patch I notice 2 nits.

+++ b/core/modules/views/src/Controller/ViewAjaxController.php
@@ -22,6 +22,7 @@
+use Drupal\Core\Ajax\PrependCommand;

Can we add this in alphabetical order? There are other entries in the use statements using \Drupal\Core\Ajax\... and this would fit nicely there.

+++ b/core/modules/views/tests/src/FunctionalJavascript/ExposedFilterAJAXTest.php
@@ -19,13 +20,20 @@ class ExposedFilterAJAXTest extends WebDriverTestBase {
+  protected static $modules = ['node', 'views', 'views_test_modal', 'user_test_views'];

greater than 80 characters

Fixing 2 nits , use statements in alphabetical order and greater than 80 characters.

Random failure back to need review.

According to the issue summary I reproduced this issue on local. Error message was not shown for exposed filter in view with AJAX. Patch #35 applied cleanly. Now error message is shown for exposed fields.

Here I have fixed a coding standard message of patch #35.

Looks like an unrelated test failure, so making it RTBC as per comment #38.

On second thought, I don't think we need tests for #24.1.

Can you elaborate on what the reasoning was here - cheers

Added coverage for inline form errors, this fails without the disable_inline_form_errors line. Also fixed the expected message, added :void

It seems like all failures similar to #49 in core are already ignored so this is my best guess at something that will work for us.

Fixed the error in #53 patch.
Please review.

Tried to fix CCF on #53

The new tests got lost in #53, please add them back in

Add the Tests As Per #55 Suggestion. try To Fix failed test.

The correct fix is to not let ViewAjaxControllerTest return NULL for renderRoot. There was already a mock for render which isn't even called in that test, so we just replace that mock with renderRoot

Confirmed the issue following the steps in the issue summary.
Using patch #57 I now get an error.
There are no users matching "asfsd".

Changes look good.

+++ b/core/modules/views/src/Controller/ViewAjaxController.php
@@ -200,6 +201,14 @@ public function ajaxView(Request $request) {
+        $output = $this->renderer->renderRoot($status_messages);

Should we be using ::executeInRenderContext like we do on 193-201 just in case something leaks metadata?

+++ b/core/modules/views/src/Controller/ViewAjaxController.php
@@ -200,6 +201,17 @@ public function ajaxView(Request $request) {
+        $output = $this->renderer->executeInRenderContext(new RenderContext(), function () use ($renderer, $status_messages) {

I think we need to pop the context here and apply the cacheable metadata, but then what would we apply that to?

However, looking at other usages of status messages in AJAXy type scenarios in core we never use executeInRenderContext.

See AjaxRenderer, ManagedFile, ViewsFormBase

So I think we're good with #57.

Think I agree with @acbramley

If there's room for refactoring the standard of AJAX status messages maybe that should be a follow up?

ViewsFormBase is wrapped by \Drupal\views_ui\Form\Ajax\ViewsFormBase::ajaxFormWrapper as far as I can see, and that does the executeInRenderContext dance.

Status messages is placeholdered so in Big pipe scenarios could require JS, additionally anyone could in theory alter #cache/#attached in a hook_preprocess_status_messages.

However looking \Drupal\Core\Ajax\CommandWithAttachedAssetsTrait::getRenderedContent it looks like if you pass in an array it will take care of getting the attached libraries and rendering, so can we just pass ['#type' => 'status_messages'] as the content to the prepend command? Would simplify things a lot I think so it's worth a shot I think.

@larowlan that does work! So much cleaner.

Need to mock getInfo on the element manager now since way down the stack that is called with the new method of passing in an array.

Retested following steps in issue summary.
Patch #70 still addresses the issue.

Can we get an answer for #41 or alternatively @Lendude can you confirm they're not needed?

Adding issue credits

I think we already have it when pageTextContains was changed to pageTextContainsOnce, basically what I was looking for was something that would fail if somebody removed $form['#disable_inline_form_errors'] = TRUE;, and if the comment above that line is correct, that should do the trick.

So this looks good to me, nice work all!

Committed to 10.1.x and backported to 10.0.x

Queued a test run for 9.5.x, will backport there if it passes.

9.5 run came back green, backported to 9.5.x - marking as fixed 💪

Is there a patch available for drupal 8.9? I can't seem to find one. The one for version 9 isn't working for me.