Entity browser modal dialog opens by pressing enter in any text field

It turns out this is not Entity Browser's fault, but just how core currently works with the HTML spec.

For the people interested in literature, there is some reference and background in:

• https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#...
• #2483505: Not possible to submit forms with pressing ENTER, if form has a file field
• #212423: Standardize form submission behavior when "enter" key is pressed
• #927176: "Enter" key removes file instead of saving node
• #2030659: Disable 'enter' submission on extend page
• #472698: admin/settings/performance: Keyboard <enter> key clears cache instead of applying settings
• #2103017: Pressing the enter key in a variety of fields triggers form submit
• #2785283: Enter key press in autocomplete results lead to ajax form submission
• #1264794: Views exposed autocomplete + ajax view = form submit happens too early

TL; DR:
Currently all text input elements (and probably other elements as well such as number, telephone, etc) have the "enter" key pressing event triggering a click on the default button (spec), which is the first button below that element in the DOM.

Once the majority of core issues I could find seem to be wont-fixes or specific workarounds to prevent the effects rather than modifying this behavior, I believe the only option we have here is to do the same (i.e. just a workaround).

IMHO, the safest approach is to just straight remove the implicit submission for all textfield elements if the form contains a modal entity browser. We could extend that to telephone, number, date, etc as well, but maybe it's not necessary... not sure.

The patch attached does that, feedback will be very welcome.

+++ b/js/entity_browser.modal.js
@@ -40,6 +40,17 @@
+        $('input[type="text"]').keypress(function (e) {

It would need to be limited to the current form context. A page could contain multiple forms ans we should not be too agressive with the sledgehammer...

I'm super confused that the algorithm doesn't take into account some sequence overrides like tabbing...
https://www.w3.org/WAI/GL/wiki/Creating_Logical_Tab_Order_with_the_Tabin...

Talking about sequence, the next problem is, we kill the enter also for form items below the entity browser submit button. There must be some intention behind the implicit submission and we kill it...

It's not yet clear to me if there are some alternatives, like using a button HTML element instead of a submit?
https://www.w3.org/TR/2011/WD-html5-20110525/the-button-element.html

This patch restricts the sledgehammer to only textfields within the same form where the Entity Browser is :)

I have also tested that the button HTML tag is also subject to implicit submission, so this wouldn't be a viable alternative apparently.

Good, better. :-)

It's still applied to all forms and all text fields, disabling the implicit submission completely.

My latest idea was if we can check in the button submit callback for the event origin or form element in focus.
If it is the button itself, it was an explicit submission.
If not, it was an implicit submission and we terminate the handler.
That would be the most minimum invasive approach, no risk to destroy other functionality.

Can we identify the implicit submission and even terminate the handler through such an approach?

That's a good idea...

However, I'm having a hard time implementing it.. If I try something like:

$(context).find('input[formnovalidate="formnovalidate"]').on('click', function (e) {
  if (e.originalEvent.x === 0 && e.originalEvent.y === 0 && e.originalEvent.pageX === 0 && e.originalEvent.pageY === 0) {
    e.preventDefault();
  }
});

The conditional appears to correctly detect whether it was an implicit submission or not, however the preventDefault() does not prevent the click from happening on the button. Am I missing something?
(I've also tried stopPropagation(), return false;, etc)

Tested your code and it seems that ajax.js triggers first async with:

      return ajax.eventResponse(this, event);

Then later your click handler is fired. You can't stop the other event anymore.
Can we hook in before the ajax is triggered?

More findings:
Also i discovered that some buttons in EB register for mousedown instead of click. That's a really bad idea.
And as stated in discussions, what EB seems to do within entityBrowserModal code looks hacky weird. Why do we need to write callbacks through settings and unwire them to register later... Something that could be easily attached with a regular behavior code.

A little workaround if you don't need keypress feature for your webmaster ( bad for accessibility however) :

modules/contrib/entity_browser/src/Plugin/EntityBrowser/Display/Modal.php | 1 +
1 file changed, 1 insertion(+)

diff --git a/modules/contrib/entity_browser/src/Plugin/EntityBrowser/Display/Modal.php b/modules/contrib/entity_browser/src/Plugin/EntityBrowser/Display/Modal.php
index aeeac5f..b531d33 100644
--- a/modules/contrib/entity_browser/src/Plugin/EntityBrowser/Display/Modal.php
+++ b/modules/contrib/entity_browser/src/Plugin/EntityBrowser/Display/Modal.php
@@ -64,6 +64,7 @@ public function displayEntityBrowser(array $element, FormStateInterface $form_st
         '#ajax' => [
           'callback' => [$this, 'openModal'],
           'event' => 'click',
+          'keypress' => false,
         ],
         '#executes_submit_callback' => FALSE,
         '#attributes' => $data['attributes'],

Tested the workaround and didn't work out for me... :-)

The patch "2897855-4.patch" fixed my problem, but it didn't submit the form anymore pressing the enter key.
In this patch I fixed this.

@johannijdam the patch contains some extra spaces. Could you remove those? It would also be nice to provide an interdiff for easier review of your changes.

Besides that, some thoughts:

• Disabling the 'enter' key is a problem for accessability. We shouldn't do that.
• The change in #10 tries to find the right button, but I'm pretty sure this will not work in 100% of the cases. Getting the right submit button 100% of the time is tricky!
• Do we really need the entity browser buttons to be of <input type="submit">? We should probably not use a submit element for things that do not submit the form. Can we switch this to <button type="button">? According to the spec a button of type button is not a submit element.

@seanB we tested this already, buttons also get implicit submit events. This doesn't change anything. The only thing that would change is a dummy link with a button look, but that would also be ugly accessibility wise.

The real solution would be a fix in core that adds some property to opt out from implicit submissions. Core could easily handle this case with very few extra lines of code. Something as simple as:

  [
    ...
    '#keypress' => FALSE,
  ]

@johannijdam I have tried your patch at #10 and it works well. Related to @seanB's comment, I do not have any solutions to fix this issue.

Your patch works well, but it limited to the text field only. With the following patch, I have extended your patch with more input elements as well such as:

• 'checkbox'
• 'email'
• 'date'
• 'datetime'
• 'datetime-local'
• 'number'
• 'radio'
• 'search'
• 'tel'
• 'text'
• 'time'
• 'url'

#12 Did you test <input type="button"> (or <button type="button">? I updated the Entity browser submit button markup in the browser and pressing Enter correctly submits the form rather than opening the modal. This is in line with the html specification:

<input type="button"> elements have no default behavior (their cousins, <input type="submit"> and <input type="reset"> are used to submit and reset forms, respectively). To make buttons do anything, you have to write JavaScript code to do the work.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/button

#11 and # 14 should fix the issue. The patch to generate input type button instead of input type submit is uploaded.

I don't think #14/#15 is going to work because core will always render a type="button" element as type="submit" (see Button.php).

This are several old issues about this, and maybe it will change in #1671190: Use <button /> form element type instead of <input type="submit" /> but in the meantime I would suggest fixing it in js or maybe at the template layer.

Attached is an improved version of the patch in #13 which replaces the type="submit" by type="button" for all modal buttons and while still submitting the form by hitting enter.

Here is a slightly improved patch which triggers the form submit instead of directly submitting it.

I tested this and seems fine. Ideally I would have two additional requests:

1. If someone with js background could take another look, that would be great
2. Should we try to test for that?

If none of that it possible atm, we can still commit and improve later.

The code likely needs improvement:
As it operates on the form level (and not the EB widget only) there are multiple risks to break things and create incompatibilities with other modules.
* It seems not to be protected against double inits (with once(), .processed checks), could thus result in two submits
* The capture seems to make it impossible to tab to the Entity Browser button and click there ENTER to open the Entity Browser. That creates another regression in keyboard navigation.

The problem with this fix is - before a custom project could fix it with a template easily.
Now if we introduce a regression, it's really hard to get rid' off the JS effects.

As those are workarounds that are trying to fix core bugs / limitations, we absolutely need test coverage.
Maybe we should simply try to raise prio in the core issue and postpone?

Looks like I missed this issue when searching for it, updated the issue's title.

Could someone update the issue summary (and related issues) with the core issue(s) that are related?

Wrt 'needs tests', any thoughts on how to write tests for this?

I hit the issue that a node form gets submitted when hitting enter in a textarea, or anywhere else.
So I removed patch #17 and all is working fine. The form doesn't get submitted and the entity browser modal is not opening when hitting Enter.

When hitting Enter in a single text field, the form doesn't get submitted (nothing happens). That is probably blocked by entity_browser? Not sure, but is acceptable in my case.

Re-rolling patch #17 with small improvement and that is to click the same element on Enter key press rather than clicking the next button of type submit on the form (there could be multiple button of type submit on the form, and it would randomly click the first submit button it will find).

+++ b/js/entity_browser.modal.js
@@ -34,7 +34,13 @@
+            $('input[data-uuid="' + instance.uuid + '"]').closest('form').keypress(function (e) {

So you propose that as soon as an Entity Browser is placed inside a form, the "Enter to submit" (standard Browser behavior) is destroyed?

This would be a severe Accessibilit issue, a bit similarly severe to the original issue here...

Patch from #17 is introducing an issue :
When typing "enter" key inside a textarea, the form is submitted, instead of a new line inserted into the textarea.
Patch from #16 does not have this issue, and switching the input from submit to button does the trick to avoid opening the entity browser modal on "Enter" keypress.
Maybe staying this simple is the best thing to do ?

I think .attr('type', 'button'); should fix the issue, let browser or Drupal handle the Enter key behaviour.
This patch is to achieve the same.

The same issue also occurs for the remove/edit buttons in the EB widgets. I guess using an actual <input type="button"> is the only way to work around this. Drupal doesn't really support <input type="button"> yet though. See #289240: There is no way to make a button element that can use the AJAX functionality.

I guess we should solve that one first, and create a new render element for AJAX buttons that actually renders <input type="button">. After that new element is in core, modules like EB can start using the new render element for AJAX buttons.

As a workaround for a client project, I took the following approach. This is quite tricky, not sure if this will work in 100% of the cases. It might inspire a better solution / workaround.

1. Add a hook_form_node_form_alter() with an after build function.

mymodule_form_node_form_alter(&$form, FormStateInterface $form_state) {
  // Add after build function to replace all AJAX buttons in node forms with
  // type button.
  $form['#after_build'][] = '_mymodule_node_form_ajax_buttons';
}

2. Mark all AJAX buttons in the after build function.

/**
 * Form #after_build callback for node form.
 *
 * @param array $element
 *   Nested array of form elements that comprise the form.
 */
function _mymodule_node_form_ajax_buttons(array $element) {
  // Change all AJAX buttons to input type button so the enter key won't trigger
  // the form submission on an AJAX button.
  $is_ajax_button = isset($element['#type'], $element['#ajax']) && $element['#type'] === 'submit';
  // Also change the Entity Browser remove button, since that has custom
  // JavaScript attached to trigger the removal.
  $is_eb_remove_button = isset($element['#attributes']['data-entity-id'], $element['#attributes']['class']) && in_array('remove-button', $element['#attributes']['class'], TRUE);
  if ($is_ajax_button || $is_eb_remove_button) {
    $element['#is_ajax_button'] = TRUE;
  }
  foreach (Element::children($element) as $key) {
    if (is_array($element[$key])) {
      $element[$key] = _mymodule_node_form_ajax_buttons($element[$key]);
    }
  }
  return $element;
}

3. Preprocess the input element to change the button type from submit to button for AJAX buttons.

/**
 * Implements template_preprocess_HOOK() for input.
 */
function mymodule_preprocess_input(&$variables) {
  // Change the type of AJAX buttons.
  $is_ajax_button = $variables['element']['#is_ajax_button'] ?? FALSE;
  if ($is_ajax_button) {
    $variables['attributes']['type'] = 'button';
  }
}