feat: use delegation to listen to trigger clicks

[mxmason]

Summary

As it says on the tin!

[KittyGiraudel]

I also think we’ll have to update the Cypress tests?

[mxmason]

Wondering whether it will have an impact on performance, but it’s probably marginal at best.

Probably marginal, yes, but I’ll take it! By not storing references to DOM nodes, we also reduce the (equally marginal) risk of a memory leak happening if the author removes from the DOM a node to which we have a reference.

As for the tests: they still pass! Is there something I should change?

[KittyGiraudel]

As for the tests: they still pass! Is there something I should change?

Considering the signature of our event handlers has changed (we no longer pass the dialog element as a first argument), I was under the impression we might have to tweak or tests. It’s not a great sign that they don‘t break in that instance. 😹

[mxmason]

Considering the signature of our event handlers has changed (we no longer pass the dialog element as a first argument)

I’m not sure I follow. The delegate function explicitly passes the event object to the show and hide functions, just as direct binding used to, and there’s no change to user-facing APIs in this PR.

[KittyGiraudel]

I’m not sure I follow. The delegate function explicitly passes the event object to the show and hide functions, just as direct binding used to, and there’s no change to user-facing APIs in this PR.

Ah not in this PR no, I meant in general. Our signature did change when we moved the events to be DOM-based. Previously the function would receive this.$el as a first argument, and no longer. That‘s not related to that PR per se though. Sorry for the confusion! :)

[KittyGiraudel]

Created some fancy new tests in https://github.com/KittyGiraudel/a11y-dialog/pull/390.