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

Opening the file page using Octotree doesn't trigger the extension #416

Closed
lucasbento opened this issue Dec 19, 2017 · 13 comments
Closed

Opening the file page using Octotree doesn't trigger the extension #416

lucasbento opened this issue Dec 19, 2017 · 13 comments

Comments

@lucasbento
Copy link

Browser name: Firefox/Chrome

Browser version: 57.0/63.0.3239.84

OctoLinker version: 4.15.1

URL and line number where issue occurs:

Expected behavior: Trigger the extension when opening the file page in screen.

Actual behavior: It doesn't trigger the extension when opening the page using Octotree, it only works after reloading the page or entering by using github.com itself.

Demo

@josephfrazier
Copy link
Member

I believe github-injection is used to determine when the page changes, so we probably need to fix it there.

@stefanbuck
Copy link
Member

Will have a look at it soon (next week)

@stefanbuck
Copy link
Member

I found the reason why this is not working. Whenever you navigate between files, PRs or issues on GitHub, they fetch the HTML from the server via ajax and replaces the content of container with the loaded html. An event from type pjax:end is dispatched on document when this occurs.

Octotree, is using basically the same technique to load the pages with a small difference. Other than GitHub.com they use a pjax jQuery plugin to achieve this. Since this is a jQuery plugin they dispatch all events on a wrapped document which leads to jQuery events rather than native events. By the nature of chrome extensions, Octotree and OctoLinker have different jQuery instances which makes in impossible to listen for events from the other extension.

Currently there are only two feasible options to solve this issue. Either creating an Octotree PR to dispatch also a native event on document or we revert OctoLinker/injection@0dcf827 which was moving away from listening for dom changes in favor of pjax:end. The latter feels wrong to me, since other extensions could still have the same issue if they also rely on the pjax:end event

@josephfrazier do you have any preference?

@josephfrazier
Copy link
Member

Wow, thanks for digging so deeply into this, @stefanbuck! I agree with you, Octotree should dispatch a native pjax:end event for compatibility with other extensions. I'll open an issue there and link back to this one, to see what they think.

@buunguyen
Copy link

Any idea how GitHub does pjax? I mean if it uses jquery-pjax as Octotree, then it should trigger the event on $(document). Pretty surprised that it triggers a native event. If I know what lib GitHub uses, I would use the same for Octotree.

@stefanbuck
Copy link
Member

I deobfuscated some javascript files from github.com. It looks like they have their own pjax implementation.

screen shot 2018-02-08 at 19 49 44

I found a pjax module which does not depend on jQuery https://github.com/MoOx/pjax. From a brief look I would say this should be compatible with the github one.

@josephfrazier
Copy link
Member

josephfrazier commented Feb 8, 2018

Yeah, it looks like https://github.com/MoOx/pjax might be the way to go, as jquery-pjax does not trigger a native event (see defunkt/jquery-pjax#349).

EDIT: Hmm, https://github.com/MoOx/pjax may not actually emit pjax:end events like jquery-pjax does (I think Github's implementation is (based on) jquery-pjax), so this may not "just work" after all.

EDIT 2: Perhaps https://github.com/OctoLinker/injection should switch back to using a MutationObserver instead of listening for pjax:end events. See OctoLinker/injection#2 (comment) and OctoLinker/injection#12 for context.

@buunguyen
Copy link

One thing is that Octotree listens on $(document).on('pjax:end') and it could receive events from GitHub navigation itself (you can see the Octotree spinner when clicking on files in GitHub tree, not Octotree tree). I think custom events are attached to the wrapped object, document in this case, which is shared among the host and extensions. I suppose if you change from document.addActionListener to $(document).on() it would work with any implementation of pjax. Is it an okay compromise?

@stefanbuck
Copy link
Member

That was one of the first things I tired out and it took me a while to understand why this is not working. I startet a little exploration with two new blank extensions. I added the same jQuery version to each of them. In extension A) I triggered a custom event via jQuery on the document node. The other extension was listening to this event, but it never received it. Doing the exact same thing without jQuery worked.

jQuery has his own event implementation which holds a list of all event listeners attached to an event. To me it looks like that those two extension does not share the same jQuery instance (which makes sense) and therefore the event list is also not shared.

@josephfrazier
Copy link
Member

jQuery has his own event implementation which holds a list of all event listeners attached to an event. To me it looks like that those two extension does not share the same jQuery instance (which makes sense) and therefore the event list is also not shared.

This sounds right, see jquery/jquery#2476

@yuu2lee4
Copy link

watch this

@fregante
Copy link
Collaborator

It was fixed: ovity/octotree#490

@stefanbuck
Copy link
Member

Awesome 🎉

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

No branches or pull requests

6 participants