karlicoss / promnesia

Another piece of your extended mind
https://beepb00p.xyz/promnesia.html
MIT License
1.76k stars 75 forks source link

Running Promnesia crashes Facebook (and some other sites) #341

Closed Stvad closed 1 year ago

Stvad commented 1 year ago

Recipe is something like:


A similar story occurs on my digital garden website: https://vlad.roam.garden. Algorithm is similar but instead of scrolling timeline - the action is to open internal ink (which should open in a new side-panel).

The common thing between those two, I think is, triggering a React re-render

Running it in Firefox 109.0b6, macOS 13.1

karlicoss commented 1 year ago

Yeah, can confirm it happens on your garden site. I think the issue is caused by "mark as visited" feature (it says 'experimental' because I sort of anticipated it might break stuff). Can you confirm it doesn't crash when "mark as visited" is disabled?

To workaround there are a few options at the moment

I think the right thing to do (considering it's inevitable that page modifications would break something) would be:

Stvad commented 1 year ago

Can confirm that disabling Mark visited prevents crashes both on roam.garden and FB.

Will think on what workaround works best 🤔

Proper solutions wise: I think it's possible to make the feature work more reliably without crashing things.

For example my extension https://github.com/transclude-me/extension does very similar things mechanics-wise (mark up relevant links & show popup when you hover over them) without causing any crashes (afaik)

I believe what makes the difference is that I do mark-up in CSS-only way (add a class to relevant links and style them differently) and don't modify the DOM

karlicoss commented 1 year ago

Interesting, thanks for the reference! (cool extension by the way :) )

IIRC the main reason I was wrapping elements into extra HTML was to add a bit which activates the popup on hover -- not sure if it's possible with CSS?

I guess in your case it works because you're using a hotkey + hover to activate -- I'd personally prefer if I didn't have to. Although maybe it's a good compromise in return for managing with CSS only (or could be configurable depending what the user preferes).

https://github.com/karlicoss/promnesia/blob/32b8e9a68b672faa9f65bc5fd463dc9a36aa4834/extension/src/showvisited.js#L186-L199

Stvad commented 1 year ago

um, I think you can just add a onHover event listener to the link element?

In my case I'm outsourcing it to the Tippy.js library, the hotkey part is not something essential to it working, just a preferred UX for me.

karlicoss commented 1 year ago

Ha, I guess I was a bit too much in the "static html" mindset, didn't even occur to me to do it via javascript. I guess that makes more sense in this case considering the tradeoff of modifying the page, yeah.

Thanks for tippy.js -- would be very cool if I can replace my custom code with something mature :pray:

For my own reference later:

I might give it a try soon -- I've been meaning to update extension towards better Manivest v3 support anyway.

If you think Promnesia could benefit from any other existing libraries -- please throw in in your suggestions :)

Stvad commented 1 year ago

looks like it has sticky plugin https://github.com/atomiks/tippyjs/blob/8f7b2df156ae39c4df23c481b77ab59fe7f08c44/website/src/pages/v5/all-props.mdx (implemented via javascript, but whatever)

fyi you're looking at the version of the docs for the older version of the lib https://atomiks.github.io/tippyjs/v6/all-props/#sticky

Stvad commented 1 year ago

I might give it a try soon -- I've been meaning to update extension towards better Manivest v3 support anyway.

🔥

If you think Promnesia could benefit from any other existing libraries -- please throw in in your suggestions :)

will do!

Stvad commented 1 year ago

something like this to activate on hover: https://github.com/transclude-me/extension/blob/cae110bb97b5b4892b6dca5ca2d90ee83ecd644d/source/content/onboarding-tooltip/index.ts#L25

also that is me specifically adding the "guard button" support bc tippy does not provide that by default. basic usage is a lot simpler: https://atomiks.github.io/tippyjs/#html-content

Stvad commented 1 year ago

https://github.com/GoogleChromeLabs/text-fragments-polyfill plausibly as a one way to do highlights

Stvad commented 1 year ago

I used https://github.com/caroso1222/notyf for notifications, though not sure how annoying is to do them yourself

Stvad commented 1 year ago

very likely not worth it to switch if things are working, but I find Parcel a lot more pleasent to work with vs WebPack

karlicoss commented 1 year ago

I used https://github.com/caroso1222/notyf for notifications, though not sure how annoying is to do them yourself

Thanks -- I actually used https://github.com/apvarun/toastify-js -- not sure which is better. For some reason it's vendorized in promnesia instead of being a proper npm dependency -- perhaps some webpack shenanigans.

I find Parcel a lot more pleasent to work with vs WebPack

thanks, also noticed you were using it and was surprised to find it had almost no configuration 😅 I might try it

karlicoss commented 1 year ago

image some WIP on tippy.js, after a bit of fighting with webpack looks promising :eyes:

karlicoss commented 1 year ago

@Stvad curious, how did you manage to release your extension with Tippy.js without running in linting errors during publishing due to innerHTML assignments? https://github.com/atomiks/tippyjs/issues/1041#issuecomment-1399631809

karlicoss commented 1 year ago

So, managed to release after all with some horrible JS patching, but it works! For now just put a self-signed Firefox version + unpacked chrome version here https://github.com/karlicoss/promnesia/releases/tag/extension-wip-tippyjs , if you wanna test will appreciate it :)

Basically it shouldn't do any DOM structure modification for "mark visited", doesn't crash facebook for me :sunglasses:

Doesn't break https://vlad.roam.garden anymore, but seems like there is a style clash between tippies used on your site and the ones in Promnesia -- odd since I specifically used an extra CSS class to prevent this, but will investigate more before merging

Stvad commented 1 year ago

@Stvad curious, how did you manage to release your extension with Tippy.js without running in linting errors during publishing due to innerHTML assignments? atomiks/tippyjs#1041 (comment)

hrm, I haven't really used the web-ext lint before 😅. I suppose these particular warnings are not cause for review rejection 🤷‍♂️

Stvad commented 1 year ago

So, managed to release after all with some horrible JS patching, but it works! For now just put a self-signed Firefox version + unpacked chrome version here https://github.com/karlicoss/promnesia/releases/tag/extension-wip-tippyjs , if you wanna test will appreciate it :)

Basically it shouldn't do any DOM structure modification for "mark visited", doesn't crash facebook for me 😎

Doesn't break https://vlad.roam.garden anymore, but seems like there is a style clash between tippies used on your site and the ones in Promnesia -- odd since I specifically used an extra CSS class to prevent this, but will investigate more before merging

Yay! Will give it a try!

Stvad commented 1 year ago

First feedback is that it doesn't seem readable in Dark mode

image

(not sure if that's new or I never tried it in dark mode before 😅)

also seems to not quite be on top of all things here 🤔

karlicoss commented 1 year ago

Hmm, so the issue with tippy on https://vlad.roam.garden/ is that when promnesia injects tippy CSS into the page, it seems to override some attributes (even if I don't do anything else with the page)

Tippy has a namespace configuration, but it's a build-time thing, so not sure how to change it when it's already bundled up... https://github.com/atomiks/tippyjs/blob/master/.config/rollup.config.js#L12

Regarding dark mode -- tried dark mode on your site, but it looks okay, can you link the exact page where you seeing this? :)

karlicoss commented 1 year ago

Actually reproduced the dark mode issue -- and also fixed other glitches on your garden. The trick was to use headless tippy.js since it's not as intrusive (also solves the problem with innerHTML) New pre-release: https://github.com/karlicoss/promnesia/releases/download/extension-wip-tippyjs/promnesia-1.2.0.6.xpi

Stvad commented 1 year ago

Nice! FWIW the dark mode issue was on FB

Stvad commented 1 year ago

Happens only when I've just installed the plugin, but thought I'd save it for posterity 😛 : image

karlicoss commented 1 year ago

Ah, yes I've seen it haha -- generally things are glitchy on extension update -- perhaps need to support some sort of callback. But been low on my priority list since releases are not that often

karlicoss commented 1 year ago

Also noticed tippy isn't working on google search page

To be fair, old (current) version of tooltips isn't working well either -- even not showing the outline properly

image

have to use 'double click' logic to bring it on top -- so perhaps it's more of an issue with

karlicoss commented 1 year ago

Right, so after investigation it seems that it's because one of the parent elements for the link has contain: layout CSS property, which prevents stuff 'inside' of that parent elements to poke out of it. So the tooltip ends up clipped.

It seems that it might have been fixed in popper (which is renamed to floating-ui now?), but tippyjs is using a version which is too old. There is an issue for migration, but not sure if it happens anytime soon https://github.com/atomiks/tippyjs/issues/1071

It's also possible to create tooltips in floating-ui directly, but I'm worried I'll have to reimplement quite a few things

Ended up doing a hack, that finds the closest parent element without contain: layout and attaching the popper to that. Although worried it might break something else...

karlicoss commented 1 year ago

damn.. turned out to be a bit easier, needed to use appendTo: document.body: https://github.com/atomiks/tippyjs/blob/master/MIGRATION_GUIDE.md#props-1 Seems that it might break accessibility, but I guess sadly promnesia isn't super accessible anyway, so we can solve it later, or make configurable via options https://atomiks.github.io/tippyjs/v5/accessibility/#interactivity

karlicoss commented 1 year ago

updated version https://github.com/karlicoss/promnesia/releases/download/extension-wip-tippyjs/promnesia-1.2.0.9.xpi

Stvad commented 1 year ago

a bit late for your various woes (though may still be useful as a general approach): in my extension I attach the tippy to a node in a shadow dom, which allows me to isolate it's CSS/etc. Generally putting your page elements into a shadow dom is very helpful to minimize interferance

karlicoss commented 1 year ago

Oh interesting! First time I've heard of shadow dom, will check it out. Do you know if there is a meaningful difference with iframe? E.g. would it make sense to put the sidebar in shadow dom instead?

Stvad commented 1 year ago

E.g. would it make sense to put the sidebar in shadow dom instead?

If you're happy with it - I probably won't bother. I think shadow dom allows for easier integration of the component into the main page, while preserving the isolation (e.g. matching layout, etc). But that's not really relevant for the sidebar (or a popup, I suppose) as it stands alone.

And in my model it's easier to programmatically interact with things across the shadow boundary.

karlicoss commented 1 year ago

I've been running the tippyjs version and think it's been very smooth -- so will merge soon and start motions to release in chrome/mozilla stores :)

karlicoss commented 1 year ago

It's released everywhere!

Stvad commented 1 year ago

image this keeps happening consistently, actually 🙃