Closed Bug 1562740 Opened 5 years ago Closed 5 years ago

Input.dispatchKeyEvent should wait for events to be fired in the content process

Categories

(Remote Protocol :: Agent, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(4 files, 3 obsolete files)

In the initial implementation of Input.dispatchKeyEvent in Bug 1543142, we did the implementation completely in the parent process, because events created in the parent process are closer to real/trusted user events.

However, this means we are waiting for the appropriate dom event in the parent process as well. And this can lead to race conditions in the tests.

Typically if the puppeteer test is doing:

    await page.type(cssSelector, someText);
    const actualValue = await page.evaluate(() => getValueFromContentPage());
    // assert actualValue === expected value

If the content page is listening to an input's change or input event in order to update its internal state and if the handler takes too much time to process the event, then await page.type will resolve before the content page has fully processed the event.

Waiting for the event in the content process instead of the parent process fixes the issue, at least according to a quick test that moved the implementation of dispatchKeyInput to the content Input domain.

The API is not clear about when we should actually resolve dispatchKeyEvent, but that seems to match Chrome's behavior here.

Now for the actual fix, we either can move the whole implementation to the content process, but we know this will limit the kind of keys we can simulate. Or we reintroduce a mix of parent and content process. We can initiate the call in the parent process and only listen to the event in the content process. Would probably be easier if our domain design allowed for an easy communication from parent to content.

Could we use docShell.chromeEventHandler for this?

It might still be a good idea to spin the event loop after waiting
for the precise content DOM events.

(In reply to Andreas Tolfsen 「:ato」 from comment #1)

Could we use docShell.chromeEventHandler for this?

Do you mean from the content process? If so, then yes that's what we would use (or at least that's what I tried, and it worked :) ).

It might still be a good idea to spin the event loop after waiting
for the precise content DOM events.

I agree, it's probably best to do both.

Priority: P3 → P2
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P2 → P1
Depends on: 1545724
Attachment #9075581 - Attachment description: Bug 1562740 - Wait for key event in the content and wait for next tick → Bug 1562740 - Wait for content key event and next tick in Input dispatchKeyEvent

Depends on D36626

In theory the mochitest no longer needs to wait for so many events

Alternative approach with less impact. More similar to what was originally suggested by ato.

Attachment #9075608 - Attachment is obsolete: true
Attachment #9075607 - Attachment is obsolete: true
Attachment #9075581 - Attachment is obsolete: true
Attachment #9076387 - Attachment is obsolete: true
Attachment #9076387 - Attachment is obsolete: false
Attachment #9076387 - Attachment description: Bug 1562740 - Allow TabSession Domains to call methods in content domain → Bug 1562740 - Allow Domains managed by a TabSession to call executeInChild
Attachment #9076389 - Attachment description: Bug 1562740 - Wait for key event in the content and wait for next tick → Bug 1562740 - Input:dispatchKeyEvent should wait for events in content process
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/be3c1b453e2c Allow Domains managed by a TabSession to call executeInChild r=remote-protocol-reviewers,ochameau https://hg.mozilla.org/integration/autoland/rev/3220f8938d1c Input:dispatchKeyEvent should wait for events in content process r=remote-protocol-reviewers,ochameau https://hg.mozilla.org/integration/autoland/rev/394b9ac5eed9 Simplify dispatchKeyEvent test and stop waiting for content events in the test r=remote-protocol-reviewers,ochameau https://hg.mozilla.org/integration/autoland/rev/0e47b22aefcc Add test for race condition when using dispatchKeyEvent r=remote-protocol-reviewers,ato,ochameau
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: