Problem/Motivation

In #3313946: Update CKEditor 5 to 35.2.1, a work-around had to be introduced because \Behat\Mink\Driver\Selenium2Driver::setValue() does:

        // Remove the focus from the element if the field still has focus in
        // order to trigger the change event. By doing this instead of simply
        // triggering the change event for the given xpath we ensure that the
        // change event will not be triggered twice for the same element if it
        // has lost focus in the meanwhile. If the element has lost focus
        // already then there is nothing to do as this will already have caused
        // the triggering of the change event for that element.
        $script = <<<JS
var node = {{ELEMENT}};
if (document.activeElement === node) {
  document.activeElement.blur();
}
JS;

        // Cover case, when an element was removed from DOM after its value was
        // changed (e.g. by a JavaScript of a SPA) and therefore can't be focused.
        try {
            $this->executeJsOnElement($element, $script);
        } catch (StaleElementReference $e) {
            // Do nothing because an element was already removed and therefore
            // blurring is not needed.
        }

… but blurring is not always a desired event. Really, Selenium should've been triggering the input event instead.

Related, in #3316274: Stabilize FunctionalJavascript testing AJAX: add ::assertExpectedAjaxRequest() it was discovered that calling ::setValue() may not trigger the necessary AJAX system updates, or at least not in time, because core/misc/form.js does:

        // Initialize form behaviors, use $.makeArray to be able to use native
        // forEach array method and have the callback parameters in the right
        // order.
        $.makeArray($forms).forEach((form) => {
          const events = 'change.formUpdated input.formUpdated ';
          const eventHandler = debounce((event) => {
            triggerFormUpdated(event.target);
          }, 300);
          formFields = fieldsList(form).join(',');

          form.setAttribute('data-drupal-form-fields', formFields);
          $(form).on(events, eventHandler);
        });

In other words: it listens to the input event, which never gets triggered, and it debounces it to allow updates at most every 300 milliseconds. But in tests there is no need for debouncing, since there is no continuous typing at all. In other words: it'd be safe to not debounce; which we can simulate by triggering formUpdated immediately, which would in turn slightly speed up functional JS tests. This part is optional though.

Steps to reproduce

Proposed resolution

Override ::setValue()'s behavior.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Active » Needs review
FileSize
4.25 KB
+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/ImageTestBase.php
@@ -86,14 +86,8 @@ protected function addImage() {
-    // Do not use setValue method as it triggers a blur event by default that
-    // closes the CKEditor 5 panel, making it impossible to click on the Insert
-    // button.
-    $this->getSession()->executeScript('
-      const input = document.querySelector(".ck-dropdown__panel.ck-image-insert__panel input[type=text]");
-      input.value = "' . $src . '";
-      input.dispatchEvent(new Event("input", {bubbles:true}));
-    ');
+    $src_input = $panel->find('css', 'input[type=text]');
+    $src_input->setValue($src);

+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/ImageUrlTest.php
@@ -104,14 +104,8 @@ public function testImageUrlWidget(): void {
-    // Do not use setValue method as it triggers a blur event by default that
-    // closes the CKEditor 5 panel, making it impossible to click on the Insert
-    // button.
-    $this->getSession()->executeScript('
-      const input = document.querySelector(".ck-dropdown__panel.ck-image-insert__panel input[type=text]");
-      input.value = "' . $src . '";
-      input.dispatchEvent(new Event("input", {bubbles:true}));
-    ');
+    $src_input = $panel->find('css', 'input[type=text]');
+    $src_input->setValue($src);

These revert the changes introduced in #3313946: Update CKEditor 5 to 35.2.1.

Wim Leers’s picture

Fix.

Wim Leers’s picture

… and the optional formUpdated improvement that overrides the unnecessary debouncing.

The last submitted patch, 2: 3316816-2-tests-only.patch, failed testing. View results

The last submitted patch, 3: 3316816-3.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 4: 3316816-4.patch, failed testing. View results

Wim Leers’s picture

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
629 bytes
5.29 KB

Now let's run this against all of core's functional JS tests.

nod_’s picture

That looks good to me

nod_’s picture

Just need to remove the changes to drupalci.yml and I'd call that RTBC

nod_’s picture

  1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
    @@ -142,6 +142,24 @@ public function setValue($xpath, $value) {
    +  // Do not wait for the debounce, which only triggers the 'formUpdated` event
    +  // up to once every 0.3 seconds. In tests, no humans are typing, hence there
    +  // is no need to debounce.
    +  // @see Drupal.behaviors.formUpdated
    +  node.dispatchEvent(new Event("formUpdated", {bubbles:true}));
    

    So there will be two triggers of formUpdated event, the one called here, and the one debounced from listening to input/change events.

    Does it cause any problem to have the 300ms delay? I'd rather avoid triggering the event twice if we can help it.

  2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
    @@ -142,6 +142,24 @@ public function setValue($xpath, $value) {
    +  node.blur = original;
    

    So if I follow correctly, after calling blur one time, we restore the previous blur behavior.

    That means that calling blur does not actually call blur the first time it triggers.

    That should be fine

Wim Leers’s picture

#13.1: it's a race condition that this removes: rather than waiting up to 300 ms (and that's not really guaranteed either, could be longer if DrupalCI is under heavy load), it's instantaneous. IOW: reduces likelihood of random failures.

nod_’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.78 KB
1.42 KB

  • catch committed b98c7e5 on 10.0.x
    Issue #3316816 by Wim Leers, nod_: Stabilize FunctionalJavascript...
  • catch committed 6a1bac3 on 10.1.x
    Issue #3316816 by Wim Leers, nod_: Stabilize FunctionalJavascript...
  • catch committed 954b937 on 9.4.x
    Issue #3316816 by Wim Leers, nod_: Stabilize FunctionalJavascript...
  • catch committed 9931579 on 9.5.x
    Issue #3316816 by Wim Leers, nod_: Stabilize FunctionalJavascript...
catch’s picture

This seems OK. Feel like there is a wider question of how much benefit we're getting from the chromedriver tests in general, but not for here.

Committed/pushed to 10.1.x and cherry-picked back through to 9.4.x, thanks!

Wim Leers’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

danflanagan8’s picture

I had some FunctionalJavascript tests in contrib that started failing on October 26, 2022. (#3326427: Crossword Functional JS tests failing)

I managed to stumble upon this change and determined that this commit results in the failures. I've failed to find any way to get things passing again.