Remove unneeded awaits for events from input actions

[lutien]

This logic is problematic to run with WebDriver BiDi, because if we don't await on the script evaluation, we have no guarantee that the script was fully run before the code goes to the next step. In this case, for example, the event listener is set up after the input actions are triggered, so the event is never caught and promise is never resolved.
It looks like it's not really needed here, neither for WebDriver BiDi nor for CDP.

[whimboo]

/botio test

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Received

Command cmd_test from @whimboo received. Current queue size: 0

Live output at: http://54.241.84.105:8877/52f0be53907afd9/output.txt

[moz-tools-bot]

From: Bot.io (Windows)

---

Received

Command cmd_test from @whimboo received. Current queue size: 0

Live output at: http://54.193.163.58:8877/e80614651e4316e/output.txt

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Failed

Full output at http://54.241.84.105:8877/52f0be53907afd9/output.txt

Total script time: 23.89 mins

• Unit tests: Passed
• Integration Tests: Passed
• Regression tests: FAILED

  different ref/snapshot: 13
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/52f0be53907afd9/reftest-analyzer.html#web=eq.log

[moz-tools-bot]

From: Bot.io (Windows)

---

Failed

Full output at http://54.193.163.58:8877/e80614651e4316e/output.txt

Total script time: 34.68 mins

• Unit tests: Passed
• Integration Tests: Passed
• Regression tests: FAILED

  different ref/snapshot: 19

Image differences available at: http://54.193.163.58:8877/e80614651e4316e/reftest-analyzer.html#web=eq.log

[calixteman]

We use a similar pattern at different places in the tests for example:

pdf.js/test/integration/ink_editor_spec.mjs

Line 28 in 319e854

const waitForPointerUp = page =>

pdf.js/test/integration/ink_editor_spec.mjs

Lines 81 to 86 in 319e854

	const promise = waitForPointerUp(page);
	await page.mouse.move(x, y);
	await page.mouse.down();
	await page.mouse.move(x + 50, y + 50);
	await page.mouse.up();
	await promise;

[whimboo]

We use a similar pattern at different places in the tests for example:

pdf.js/test/integration/ink_editor_spec.mjs

Line 28 in 319e854

const waitForPointerUp = page =>

pdf.js/test/integration/ink_editor_spec.mjs

Lines 81 to 86 in 319e854

	const promise = waitForPointerUp(page);
	await page.mouse.move(x, y);
	await page.mouse.down();
	await page.mouse.move(x + 50, y + 50);
	await page.mouse.up();
	await promise;

Yes, we wanted to get these tests running in CI first for this particular case with CDP to see if that makes a difference. While this is fine other tests are failing. Not sure if that is a known issue with reference images or not?

Also please note that when I checked the puppeteer unit tests none of the input tests actually checked for events, so whenever such a related command has been awaited for we probably can assume that the event has been dispatched on the page. CC @OrKoN for proper feedback from the Puppeteer team.

[calixteman]

Yes, we wanted to get these tests running in CI first for this particular case with CDP to see if that makes a difference. While this is fine other tests are failing. Not sure if that is a known issue with reference images or not?

Yes, those failures in ref tests are known (due to small invisible pixel differences because of antialiasing most of the time).

[OrKoN]

This logic is problematic to run with WebDriver BiDi, because if we don't await on the script evaluation, we have no guarantee that the script was fully run before the code goes to the next step. In this case, for example, the event listener is set up after the input actions are triggered, so the event is never caught and promise is never resolved.
It looks like it's not really needed here, neither for WebDriver BiDi nor for CDP.

I wonder if we should address this in the BiDi spec. The pattern is relatively common in CDP/Puppeteer and I know that it used often in tests. In Chrome, the script is guaranteed to run first because both commands run on the page's main thread eventually. Is it not the case for Firefox?

[OrKoN]

I am not sure that those awaits are completely unnecessary since they allow verifying that the even has been dispatched on an expected element which allows failing the test earlier.

[OrKoN]

So to correct myself above: the dispatch of the evaluate was happening synchronously at some point but it has regressed. I'd recommend using the following pattern which would be more reliable:

          using promise = await page.evaluateHandle(
            () =>
              [new Promise(resolve => {
                document.addEventListener("selectionchange", resolve, {
                  once: true,
                });
              })]
          );
          await page.click("#pageNumber");
          await promise.evaluate(promise => promise[0]);

[calixteman]

@whimboo, @lutien, I made this PR:
#17387
so I guess this one is useless now.