1822772 - Missing navigation events when document gets replaced via "document.[open/write/close]"

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I was informed by the Puppeteer team today that the following code which is used to set a page's content doesn't trigger any navigation events via WebDriver BiDi. It's used a lot in Puppeteer and actually works when running with Chrome.

    await this.evaluate(html => {
      document.open();
      document.write(html);
      document.close();
    }, html);

Even through this API is not pretty and removes all the registered event listeners it is also used in web pages and as such we need to support it.

Given that we have our listeners attached from within a script running in chrome context, it should basically continue to work. If not we may have to register them via the windowRoot.

I'll create some test cases including documentElement.remove() and using the DOMParser to add new content to see how those scenarios work.

We may have to add this to our M7 milestone. Lets discuss on Monday.

Comment 1

[Julian Descottes [:jdescottes]]

FWIW, I quickly tested two things:

• if I subscribe to the event after document.open() is used, I get a load event when document.close() is called, so platform fires an event
• using this.#window = win.windowRoot.ownerGlobal; for the LoadListener window does not help with this issue

Looking at CDP, it seems that we use ChromeEventHandler to listen to load events, but I didn't manage to make it work for BiDi. I'm assuming our current CDP implementation works for the pattern from Puppeteer, so it might still be worth investigating.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Julian! Looks like that we might indeed can pick what we have in our CDP implementation. Beside the load event we would also need the navigationStarted and domContentLoaded. I hope that this would be easy enough as well - or maybe you only tested for load?

Here an example WebDriver BiDi test:

async def test_document_write(bidi_session, subscribe_events, top_context, wait_for_event):
    await subscribe_events(events=[DOM_CONTENT_LOADED_EVENT])

    on_entry = wait_for_event(DOM_CONTENT_LOADED_EVENT)

    await bidi_session.script.evaluate(
        expression="""document.open(); document.write("<h1>Replaced</h1>"); document.close();""",
        target=ContextTarget(top_context["context"]),
        await_promise=False
    )

    event = await on_entry

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Another case could be the following:

    await bidi_session.script.call_function(
        function_declaration="""(html) => {
                const parser = new DOMParser();
                const doc = parser.parseFromString(html, "text/html");
                const errorNode = doc.querySelector("parsererror");
                if (errorNode) {
                    throw new Error("Failed");
                } else {
                    document.documentElement.replaceWith(doc.documentElement);
                }
            }
            """,
        arguments=[{"type": "string", "value": "<h1>Replaced</h1>"}],
        target=ContextTarget(top_context["context"]),
        await_promise=False
    )

Here we also don't fire events, but the question is if we should do or not. I haven't checked if platform is actually doing that.

Comment 4

[Julian Descottes [:jdescottes]]

Managed to make this work with BiDi as well. We can grab the chromeEventHandler via window.docShell.chromeEventHandler, but then it's important to use capture: true for those events.

For info, I tried the second test case with documentElement.replaceWith and in this case we don't fire a load event, even with the changes which allow the document.open/close approach to work.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Julian Descottes [:jdescottes] from comment #4)

For info, I tried the second test case with documentElement.replaceWith and in this case we don't fire a load event, even with the changes which allow the document.open/close approach to work.

Yes, that works differently and as told by Olli it's expected that no load event is fired. As such we should keep the document.open() approach on our focus for now given that this is a highly important feature for Puppeteer.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1822772 - [bidi] Use windowRoot to monitor load events

Event listeners are removed when using document.open, so we should use chromeEventHandler instead

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1822772 - [wdspec] Add tests for document open with bidi load events

Depends on D173020

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1822772 - [messagehandler] Destroy MessageHandlers when pages move to BFCache

Comment 9

[Phabricator Automation]

Comment on attachment 9324157 [details]
Bug 1822772 - [messagehandler] Destroy MessageHandlers when pages move to BFCache

Revision D173132 was moved to bug 1823670. Setting attachment 9324157 [details] to obsolete.

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1822772 - [wdspec] Add cleanup logic to the wait_for_event bidi fixture

Depends on D173020

The new tests were failing on Android because the previous (expected fail) test was leaving the test harness in a bad state.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56721ff2d9f3
[bidi] Use windowRoot to monitor load events r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/eaa0d06fff8d
[wdspec] Add cleanup logic to the wait_for_event bidi fixture r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/379e1d5e3b79
[wdspec] Add tests for document open with bidi load events r=webdriver-reviewers,whimboo

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Failed to create upstream wpt PR due to merge conflicts. This requires fixup from a wpt sync admin.

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/56721ff2d9f3
https://hg.mozilla.org/mozilla-central/rev/eaa0d06fff8d
https://hg.mozilla.org/mozilla-central/rev/379e1d5e3b79

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/39182 for changes under testing/web-platform/tests

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by jgraham