Add support for 'signal' option to FederatedEventTarget.addEventListener

[jmcclell]

This change brings the addEventListener implementation more in line with the DOM implementation.

Description of change

Adds support for the 'signal' option to the addEventListener method of FederatedEventTarget. This makes this method more congruent with the DOM implementation. Fixes #10043

Pre-Merge Checklist

• Tests and/or benchmarks are included
• Lint process passed (npm run lint)
• Tests passed (npm run test)

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5749f11:

Sandbox	Source
pixi.js-sandbox	Configuration

[bigtimebuddy]

This works for me. Thank you @jmcclell

[bigtimebuddy]

@jmcclell could you resolve conflicts with the once PR?

nvm, I think I got it.

[bigtimebuddy]

Thanks again for these PRs 👍 This was added to v8 too.

[jmcclell]

@bigtimebuddy I just realized this contains a nefarious bug. 🤦🏻 In an effort to guarantee (which I beleive the DOM implementation does) that there is no chance the event will fire after abort() is called, I wrap the original istener to add a check on the signal itself rather than solely relying on the signal's abort event to remove the original listener. Unfortunately, this means that if you want to manually remove the listener, you'd need a reference to that wrapped function as your original listener will no longer match in off.

Here is a test case that demonstrates:

    it('should allow manually removing the original event listener when using an AbortSignal', () =>
    {
        const renderer = createRenderer();
        const [stage, graphics] = createScene();
        const eventSpy = jest.fn();

        renderer.render(stage);
        const controller = new AbortController();

        const listener = (e: Event) => {
            expect(e.type).toEqual('click');
            eventSpy();
        };

        graphics.addEventListener('pointertap', listener, { signal: controller.signal });

        const click = () =>
        {
            const event = new PointerEvent('pointerdown', { clientX: 25, clientY: 25 });

            renderer.events.onPointerDown(event);
            const e = new PointerEvent('pointerup', { clientX: 30, clientY: 20 });
            // so it isn't a pointerupoutside

            Object.defineProperty(e, 'target', {
                writable: false,
                value: renderer.view
            });
            renderer.events.onPointerUp(e);
        };

        click(); // Once
        graphics.removeEventListener('pointertap', listener);
        click(); // Twice
        expect(eventSpy).toHaveBeenCalledOnce();
    });

For the moment, I might suggest we revert the portion of the change that wraps the listener function and keep the listener to the signal's abort function. In the meantime, I'm trying to see what the DOM implementation actually guarantees. I've always relied on it being as I wrote it, but perhaps I've just been lucky.

[jmcclell]

@bigtimebuddy Okay, so given that browsers run JS in a single-threaded context, I believe we are safe to remove the wrapper and that it is entirely superfluous.

If my understanding of the runtime semantics is correct, as soon as the abort() method fires the abort event, that event is added to the event loop's queue. No other subsequent events will be processed before that event. The processing of that event should allow for the removal of the listener so that any subsequent events that would have otherwise triggered the listener will now be processed without the listener present.

Will ship up a patch. Let me know if my thinking seems correct here. JS is not my primary language. :)

[bigtimebuddy]

Yeah, that seems reasonable approach and simplifies the code too.

[jmcclell]

PR #10073 pushed. Sorry for the confusion! Appreciate the prompt responses.

[jmcclell]

Scratch that. Had some confusion on branches on my end. PR #10074 :)