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

fix: should now inject interaction buttons properly on post detail page #87

Merged
merged 7 commits into from
Dec 13, 2022

Conversation

hueyy
Copy link
Contributor

@hueyy hueyy commented Nov 24, 2022

@rugk
Copy link
Owner

rugk commented Nov 25, 2022

Okay, tested it practically (ref):

  1. seems mostly fixed, however only when I open the link directly/reload the page. If I click the link here on GitHub, it again does not detect the injection… (reproducible) 😢 (I wonder whether that is, because of some PWA stuff or because of preloading/caching and/or a timing bug/race condition or whatever…)
  2. Does still not work for me. STR:
  3. I open https://mastodon.social/@drewtoothpaste/109393612157338955 in a new tab (paste in URL)
  4. (optionally) click an interactuion button to confirm it initially works
  5. now navigate somewhere else (the back button does not work as we have not naviagted from somewhere), e.g. by clicking on the link to the profile of the instance admin:
    grafik
  6. Okay, IMHO it's good you cannot reproduce this. I still can. But I guess it's related to my target instance (chaos.social) then.
  7. If I am in (1) state, i.e. a broken state, where it does not work, it resproducibly fails, i.e. even the follow button does nto work, no matter how much I navigate around. (Actually my understanding is each URL change should cause the injection, should not it? Or no, actually only when the tab is loaded/a new site is loaded? If so, then we have a problem with the injection of course.

If the injection should be a problem and makes it less (sporadically) buggy, I BTW, would be fine for introducing the "Acess all sites" permissions (note just to explain the permission in the permission.md document then).

Mastodon v4.0.2
Firefox 106.0.4
add-on code: 2110424

Copy link
Owner

@rugk rugk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I am unsure what the problem here is… but I have some tips anyway.

src/content_script/mastodonInject.js Outdated Show resolved Hide resolved
src/content_script/mastodonInject.js Outdated Show resolved Hide resolved
src/content_script/mastodonInject.js Outdated Show resolved Hide resolved
Copy link
Owner

@rugk rugk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed a comment

src/content_script/mastodonInject.js Outdated Show resolved Hide resolved
@hueyy
Copy link
Contributor Author

hueyy commented Nov 26, 2022

Thanks for the detailed writeup! Yes, now I can reproduce it. The problem is with the injection. It seems that when you navigate to a different page on a PWA browser.tabs.executescript() is called but somehow the content script is not re-run. Let me experiment a bit, there should be a way around this.

@hueyy
Copy link
Contributor Author

hueyy commented Nov 26, 2022

Figured it out. So it appears that Firefox will wait for the Return Value of the content_script to resolve before re-running the content_script on the same tab, unless the tab reloads. The return value of the mastodonInject.js content_script was not returning quickly enough for 2 reasons:

  1. the init() function was awaiting a waitForElement call with a timeout of 20s; and
  2. the declaration of the constant TIMEOUT_DURATION at the top of the file caused an error to be thrown on subsequent content_script executions because it appears the same context is used for subsequent executions

@rugk, trouble you to test it again and confirm the issue has been resolved.

@hueyy hueyy requested a review from rugk November 26, 2022 07:36
@rugk
Copy link
Owner

rugk commented Nov 30, 2022

Please also note we have a manual testing guide, so if you could test it manually based on that guide, that would be helpful, too: https://github.com/rugk/mastodon-simplified-federation/wiki/Testing

Please note the guide is partially outdated, feel free to correct that… see #45.

@rugk
Copy link
Owner

rugk commented Nov 30, 2022

forget this part

I managed to find a problem: If I use the link from the guide here and quickly click on any interaction button/star, while the progress bar at the top is still loading, I get the same issue again, it's still the old dialog.

I assume the content script is only injected/we trigger only when the page has fully loaded? Or actually the MutationObserver can trigger earlier, can't it?

Addendum: Managed to reproduce this twice and I guess it was because I was fast hmm... maybe one also needs to load a toot that is not yet cached in your browser and poztentially large (image or so).

Okay, tried to reproduce it again via network throtteling, I cannot: https://firefox-source-docs.mozilla.org/devtools-user/network_monitor/throttling/index.html

As soon as the star is there I click on it and here it goes… though hmm, maybe of course network throtteling does only throttle the network not JS… 🙃

Ahhh, I did not update my testing code I am sorrry…

Back testing 8490dbe

So with throtteling in 2G good/3G scenarious (may not be relevant) I noticed when I click on a toot at the top right (the date) to open it and then click any of the interaction buttons, it does not work either.
Going back to the account overview (not the detailed toot view) …and there it works again though.

Also scrolling down and loading new stuff via AJAX works.
Opening the link (click on date again), in a new tab (middle click via mouse e.g.) works, too.

It reproducibly fails to inject the stuff when I navigate from account overview to a single toot. Navigation via browser back/forwards buttons yields in the same result.

@rugk
Copy link
Owner

rugk commented Nov 30, 2022

the declaration of the constant TIMEOUT_DURATION at the top of the file caused an error to be thrown on subsequent content_script executions because it appears the same context is used for subsequent executions

Ahh, yeah… that is a good thing you noticed! However, your solution of removing the const variable is not a proper solution IMHO.

Actually, do we really want to inject the content script multiple times? I think, actually, wo don't! It has all things to stay there and keep reacting on things that happen, AFAIK.

So, instead, could not we maybe check for a "if isDeclared(SOME_CONSTANT_THAT_SHIOWS_WE_INJECTED_THIS_SCRIPT)" and then break processing, so nothing gets reinjected? (This is pseudo code of course.)

Related problems (not looked into details): https://stackoverflow.com/questions/74624848/inject-content-script-only-once-in-a-webpage and https://discourse.mozilla.org/t/why-is-it-soo-hard-to-inject-a-content-script-only-once-at-site-load/38903 (ouch, I just saw that last one was from past self here, actually 🙈 )

Copy link
Owner

@rugk rugk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+ see my comment above, reinjection complicates stuff and may cause weird side-effects indeed, so let's prevent that from happening.

src/content_script/mastodonInject.js Outdated Show resolved Hide resolved
@rugk rugk linked an issue Nov 30, 2022 that may be closed by this pull request
@hueyy
Copy link
Contributor Author

hueyy commented Dec 1, 2022

It seems to be difficult to re-run the injection functions when the Mastodon uses the History API to navigate (e.g. from toot detail to profile page) because Firefox does not emit a popstate, hashchange, or pageshowevent. This is apparently to prevent malicious websites from hijacking exit events, but it does make our job more difficult.

Instead, I have used a new MutationObserver to check for new UI elements. I have removed the previous MutationObserver and have carefully tailored the CSS selector for the new one, so I think the overhead of monitoring UI changes should be reasonable. I have tried various more specific CSS selectors, but besides the risk of breaking changes if Mastodon changes its UI, the element selected may also be replaced by some actions (e.g. the main column .column[role="region"] is occasionally replaced), hence any MutationObserver observing that becomes ineffective.

@hueyy hueyy requested a review from rugk December 1, 2022 02:45
Copy link
Owner

@rugk rugk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise LGTM, let me test.

src/content_script/mastodonInject.js Outdated Show resolved Hide resolved
@rugk
Copy link
Owner

rugk commented Dec 3, 2022

BTW while testing I had #88, but now seems to work or I am not sure really.

@rugk
Copy link
Owner

rugk commented Dec 8, 2022

Another unfortunate problem

I have to say that, unfortunately, now, the add-on also triggers on the "usual" Mastodon page i.e. my own instance and tries to redirect stuff back to my own instance quite often.
I.e. some interaction buttons (mostly noticed the "reply" button), just trigger it, which is obviously quite bad.

Obviously, we should avoid that… 🙃

@hueyy
Copy link
Contributor Author

hueyy commented Dec 10, 2022

@rugk Oops, I realised this happens when you open a remote instance page in a background tab, because the executeScript function injects into the active tab by default. Fixed that by specifying the tabId!

@hueyy hueyy requested a review from rugk December 10, 2022 02:47
Copy link
Owner

@rugk rugk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise LGTM.

@rugk rugk merged commit 2c82e7b into rugk:master Dec 13, 2022
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.

Support for Mastodon v4
3 participants