Error on pressing Enter in component props form

Went down the road of pursuing a backend solution. Setting the action of the form to an empty string

ComponentPropsForm::buildForm

public function buildForm(array $form, FormStateInterface $form_state, ?FieldableEntityInterface $entity = NULL): array {
...
# Remove the action attributes value to prevent the form from being
    # submitted when user presses Enter.
    $form['#action'] = '';
    return $form;
}

This did what was intended and now pressing enter doesn't directly lead to an error page. But hitting enter still "submits" the form which leads to a moment where the page reloads. After the page reloads, all my components I've placed are gone, and I have to start over.

I don't think this solution is enough. Perhaps it does ultimatley make sense to remove the giant value in the action attribute, but it still is a bug to users.

I think we should pursue a solution where we teach the component props form to respond differently to pressing Enter. Perhaps that's not sufficient to fix the bug, because the main thing we want to avoid is doing a full form submission.

The main thing to figure how is where to make change. These two Form objects seem like good candidates:
* ui/src/components/form/Form.tsx
* ui/src/components/form/FormElement.tsx

While I was convinced before that this should be handled where the form is generated — the backend —, after more thinking, and mostly based on your findings, I have to realize that it may not be enough.

I think on the frontend we should consider leveraging the HTMLFormElement: submit event, and Event: preventDefault() method to prevent the browser doing the default action upon submitting the form. It feels more semantic to me than handling a specific keypress, but I'm curious what others think.

I did consider taking over the form's submit event. For now, I'm testing with only taking over the form's KeyDown event. In my local testing that has proven to be sufficient. I'm in the process of getting the tests to run to see if this is ready for review.

After re-running the pipeline, things are green now! 🟢

Here is what I suggest we do next:

1. Change the implementation to use the HTMLFormElement: submit event. As I mentioned before in #3, I think it's more semantic to intervene directly in the event that causes what we would like to prevent, instead of intercepting the event that leads to said event. We would like to ensure that if and when the form gets submitted, none of the default actions happens. How the form gets submitted, e.g.. as a result of the Enter keypress, is an implementation detail.
2. Even though we already fixed the reported bug, I think we should remove the action property from the generated form markup returned in \Drupal\experience_builder\Form\ComponentPropsForm::buildForm, so the form markup doesn't express something that is not expected.
3. Tests would be nice to validate this.

Assigning to Wim Leers to get some feedback on the above.

I've included the recommended changes from #7.

First: thanks for the great bug report, @cosmicdreams! Can't believe we didn't spot this 😅

🤔 Would it be more in line with the HTML spec to set method="dialog" per https://developer.mozilla.org/en-US/docs/Web/HTML/Element/form#method?

https://caniuse.com/dialog indicates we could use <dialog>, and the current right sidebar — labeled xb-contextual-panel — could be wrapped in a <dialog> instead of a <div>?

That seems appropriate because:

The HTML element represents a modal or non-modal dialog box or other interactive component, such as a dismissible alert, inspector, or subwindow.

— https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog, emphasis mine.

• #3: see above wrt <form method="dialog">.
• #7.1: I independently observed the same, that's still present in the MR — perhaps @cosmicdreams forgot to push something in #8?
• #7.2: Nope, the Media Library Widget continues to work fine! 👍

I really like the solution of using method="dialog". Sounds like it was custom made for this situation. I'll check what I pushed recently and make sure I work this in.

I'm not actually sure this still needs test coverage then, because that'd be testing browser-level functionality.

Let's get expert feedback 🤓

<form method="dialog"> is beautiful! 👏🏻

I was just thinking that a simple end-to-end test would be reassuring to make sure we don't accidentally remove it when generating the form, then face this awkward bug again. 😊

Fair — simply triggering the Enter key should be sufficient then :)

I'll take a shot at writing the test, but this will be my first one for Experience Builder. Are we talking about a cypress test?

Also, I can't seem to be able to run any tests in my local development environment. I'm working through that.

Left a few comments and also tests are still failing.Marking it NW again.

Thanks for the review. I can see now that even though I can't get these tests to run locally (i'm on a Mac and xQuartz isn't working properly) I do see the test results on Gitlab. I see my use of data attributes is what is at fault here. I'm attempting to provide a more Cypress-centric implementation now.

Test passes, ready for review.

@deepakkm Did you accidentally include work from another issue into mine?

@cosmicdreams its a rebase, to actually verify one failure related to cypress test which i see is now fixed. Also it will be helpful for someone to RTBC this issue. I hope this helps.

Good solution and tests, left a bit of feedback on the MR for commenty stuff.

Automatically closed - issue fixed for 2 weeks with no activity.