Export onclick handler

[ockham]

Rationale: We're using page.js for client-side routing in a large React-based project. While removing one instance of a React element that for legacy reasons had been accidentally rendered twice, we've noticed that after removing it, React's synthetic event handling collides with page.js's click binding: On a given <a /> element with both href and onclick attributes, page.js would attach its click handler to it, loading the route specified at href before the app-provided onclick handler would be invoked. Our app-provided onclick handler contains a (conditional) event.preventDefault(), so the expected behavior would be that (if the relevant condition evaluates to true) to only run the code in the onclick handler, and not to direct to the href-specified route. (I've also tried event.stopPropagation().)

We've found that we can work around this issue by setting click: false on page.start(), exporting page.js's onclick handler, and attaching it to our top-level React element, see https://github.com/Automattic/wp-calypso/pull/26944.

This PR is about the exporting part -- would this be an acceptable addition to page.js? We haven't been able to find a better solution to this problem that doesn't require modifications to page.js, but are obviously grateful for any sort of feedback or advice.

[ockham]

/cc @Automattic/team-calypso

[coveralls]

Coverage decreased (-1.2%) to 92.469% when pulling 35f72c4bb6f12d4f5cbac23047191dd573b745f8 on ockham:export-onclick into 02c026ca82f9d3f5018038d822592e77c040cfb7 on visionmedia:master.

[matthewp]

Awesome, thanks for contributing! I'll think about this more and get back to you, I think I understand the problem. There are 2 event handlers on the element and yours can't stop page.js' from running. Is that correct? If so can you answer a couple of questions?

1. Does your event handler run first or does page.js' ?

Actually that's my only question right now :) I feel like if you call preventDefault() then we should be able to see that event.defaultPrevented is set (that exists) and know not to run our code.

If my assumptions here are correct let me know and I'll try to fix this soon. Glad to see that Automattic is using page.js!

[matthewp]

If page.js' handler runs first, you want to use useCapture to make your handler run first. Does React provide a way to do that?

EDIT: It looks like React supports capture through onClickCapture. I haven't tried this personally though.

[jsnajdr]

Hi @matthewp!

Does your event handler run first or does page.js' ?

It depends on two things:

• at what time is page.start() called? That's when the clickjacking handler gets installed on the document element.
• when does React render a component with onClick handler for the first time? That's when the React synthetic event system installs its click handler on the document element.

Our desired state is that first React render ever happens as a result of handling the initial route, which means that page.js handler gets installed first, and the React one second. That means that event.preventDefault() in React onClick listener will always come too late, because page.js handler already ran.

There's a detailed analysis here: https://github.com/Automattic/wp-calypso/pull/26944#issuecomment-417270984

If page.js' handler runs first, you want to use useCapture to make your handler run first.

We don't want to ban onClick handlers (in bubble phase) in the entire app. We even don't have the power to do that -- third party components will have onClick (bubble) handlers, because that's the perfectly reasonable thing to do. Allowing only onClickCapture is not a viable solution.

The root cause of the whole issue is that when there is a <a onClick /> React element, then the actual DOM a element won't have an onClick element. There's one global listener on document that figures out what's the event.target and will dispatch the event to the right React component.

If there was a real DOM listener on the a element and then the page.js clickjacking listener on document, the event would bubble as expected: it's handled by the a listener first and by the document listener second. And the React event system tries to preserve the illusion that there are real DOM listeners by doing the capture/bubble dispatch correctly. But it breaks down with the page.js listener in place.

[matthewp]

Ok, so I think I understand the problem better now. It sounds like you want more control over the click handler, and really don't want page.js' default behavior at all. But you do want the logic within the click handler, you just want to register it yourself so you can ensure everything is called in the order you need.

In that case I don't see a problem with exporting the handler. I don't think it should be _onclick though, that would make it seem to be privatish. Since we're now making this a public API let's rename that. page.clickHandler maybe?

[matthewp]

I created #513 as a replacement for this.

[jsnajdr]

you just want to register it yourself so you can ensure everything is called in the order you need.

That's right, we intend to install the handler on the top element of our React render tree, so that it's called by React as part of its synthetic capture/bubble process.

[matthewp]

This is in 1.11.0: https://github.com/visionmedia/page.js#pageclickhandler