Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dispatch pjax:end event on native document #861

Merged
merged 2 commits into from
Dec 30, 2019

Conversation

jdreesen
Copy link
Contributor

@jdreesen jdreesen commented Dec 29, 2019

Problem

GitHub's own pjax implementation dispatches its events directly in the DOM, while the jQuery pjax library we use here dispatches its events only within its jQuery instance.

Because some GitHub add-ons listen to certain pjax events in the DOM it may be necessary to forward an event from jQuery to the DOM to make sure those add-ons don't break.

Fixes #490

Solution

Dispatch the pjax:start and pjax:end events (for now) in the native DOM whenever they're dispatched within jQuery.

Note that I don't forward the event details/extra parameters or whether they're cancellable at the moment, because the pjax implementations differ in this case and I could not find a case where this info is used by listeners.

Screenshots

Before this PR when navigating through Octotree:
grafik

After the PR or when navigating though GitHub or reloading the page manually:
grafik

As can be seen on the second screenshot, Refined GitHub adds gray dots to show the whitespace and OctoLinker adds pink dots to indicate that you can navigate to the required libraries.


As I have no experience in browser add-on development and there's no documentation how I could build and install my local Octotree development version in my Firefox, I don't know how to verify if the solution is working. But I've done a lot of research and digging through (minified) JavaScript and am therefore pretty confident it does.

So, could you please verify this?

@buunguyen
Copy link
Collaborator

Hi, thanks for this. Question: I'm still puzzled how Octotree breaks other extensions. I read the original issue #490 but wasn't clear why listening on events would break things. Does that stop native events from being fired or what?

@buunguyen
Copy link
Collaborator

Also, to be clear, you didn't test it at all? Or you just didn't test it on Firefox?

@buunguyen
Copy link
Collaborator

I think I get it now. Let me test more carefully. Thanks again for helping out!

@buunguyen buunguyen merged commit 0530bf6 into ovity:master Dec 30, 2019
@buunguyen
Copy link
Collaborator

Tested. Just need to handle re-entrance otherwise there will be call stack error: 44fdb80.

@jdreesen jdreesen deleted the pjax-in-dom branch December 30, 2019 21:00
@jdreesen
Copy link
Contributor Author

Thx for your quick reaction and testing/merging/fixing this! :)

Also, to be clear, you didn't test it at all? Or you just didn't test it on Firefox?

I didn't test it at all, because I didn't know how (I have zero experience in add-on development).

Question: I'm still puzzled how Octotree breaks other extensions. I read the original issue #490 but wasn't clear why listening on events would break things. Does that stop native events from being fired or what?

The problem were not the listeners in Octotree, but that Octotree uses a jQuery pjax plugin, whereas GitHub uses a custom pjax implementation without jQuery that's operating directly on the DOM.

While jQuery's event listeners get called for events dispatched through jQuery or the DOM, everything was fine on your side.

Events dispatched through jQuery, on the other hand, are only visible within jQuery but not the DOM.

This means, that because those other plugins listen to events directly on the DOM, their listeners don't get called when Octotree dispatches its pjax events, which is why we have to re-dispatch them directly on the DOM (which... results in a loop, because jQuery's listeners get called again, so thx for your re-entrance fix!).

Here's a little example:

document.addEventListener('my:test', e => console.log('DOM', e.type))
jQuery(document).on('my:test', e => console.log('jQuery', e.type))

document.dispatchEvent(new Event('my:test'))
// > DOM my:test
// > jQuery my:test

jQuery(document).trigger('my:test')
// > jQuery my:test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pjax:end event not dispatched on native document
2 participants