adds live page preview on hover :) #52

palashkaria · 2020-05-10T21:00:57Z

Works by adding an iframe to the page on load, & checking for mouseover of page references. If found, changes the iframe src -- this leads to instant preview :)
To see it in action, see the video here:
https://twitter.com/palashkaria/status/1259589041688768512

Draft PR, WIP

currently works for [[]] links in text. Working on #, & headings etc.
Need to test this with other roam features.
Can make the size/styling of the preview customisable? (currently hard-coded)

Stvad · 2020-05-10T22:06:01Z

would be nice if it were possible to move mouse over popup and scroll that Iframe/etc

also should block references get preview as-well? (I'm not sure about it myself)

Stvad · 2020-05-10T22:17:38Z

hmm, this seems to somehow add a small lag for when I'm typing things in the block (not trying to do hover preview or anything)

Stvad · 2020-05-10T22:19:47Z

maybe just because we need to re-render 2 instances of Roam in the same thread?
wonder if this can be done in a background process or something 🤔

Stvad · 2020-05-10T22:32:32Z

if the reference is close to the sight side of the page the popup can go outside of the screen

palashkaria · 2020-05-10T22:50:46Z

@Stvad

cursor into popover for scroll - this can be a little complex -- as we need to figure out when to close etc. The code itself will be complex
popup getting cut off the screen -- I do plan to handle this, but with a minimal impl, but if we want it to be perfect, we might want to look at some small lib, like tippy. This might help us with the first issue too. Will do some research when implementing.
Not sure about the lag -- might be because we have a mouseover handler? I don't think a roam running in an iframe would cause any issues. I also do a dom checks, on mouse over, which might be causing repaints/layout thrashing. Do you have inspect element open when you get the lag? I will try and do a perf profile to see if we can pinpoint the issue.
Preview works by loading an iframe -- for block reference, we load the page?
Might not make much sense as the context of the data is lost.

Also, currently the toggle does not work -- did not get to fixing it yet.

Stvad · 2020-05-10T23:00:51Z

1,2 using a lib sounds good to me
lag - I don' think it's mouseover - I'm not touching the mouse or anything, just typing the text in the block. Why I think it can be related to Roam in iframe, it's an aggregate of the following:
- Roam is pretty slow in general.
- It does re-rendering when you type things.
- Now you have 2 instances of Roam running in your frontend thread, so you need to do 2x amount of live rendering for each typing event
Block references - no, if you construct URL the way you do right now, but putting the block uid vs page uid you'll get a "zoomed in" view of the block

Not quite sure what you mean by toggle?

palashkaria · 2020-05-10T23:10:03Z

@Stvad we are hiding the iframe using opacity/height/width, set to 0; There will be no repaint/relayout happening in that case. We can easily check for this using profiling in chrome's devtools

AH, you mean Block references + it's children? Could be done, but for longer lengths it will get tricky.

palashkaria · 2020-05-10T23:12:46Z

I've been using roam with this enabled for some time now.. Don't see any lag, really. Perf profile also shows 60fps frames (mostly)

palashkaria · 2020-05-10T23:13:53Z

Will keep a lookout for lag -- should see it if it happens; I have this enabled anyway

Stvad · 2020-05-10T23:23:00Z

ok, rendering is the wrong word, I suppose, background processing would be more precise.
When the preview is enabled: I have these moments when the "scripting" consumes 100% CPU (if I read this correctly) and the Roam freezes up.

the following is without preview enabled:

in both cases I'm just typing in the text into the block

Stvad · 2020-05-11T01:38:26Z

another thing to support here is Markdown aliases [alias]([[Page Name]])

palashkaria · 2020-05-11T05:36:34Z

@Stvad could you send a dump of the log with preview enabled? It can be done by using rightclick-> save
it does look like scripting, but the log will help us pinpoint which script, too

Stvad · 2020-05-11T06:45:44Z

do you mean console log or saving the captured profile?

palashkaria · 2020-05-11T06:46:09Z

captured profile @Stvad

Stvad · 2020-05-11T06:54:20Z

put it here: https://file.io/o5rQDqPn

palashkaria · 2020-05-11T07:31:03Z

@Stvad getting a 404 on that URL

palashkaria · 2020-05-11T13:31:27Z

@Stvad I'm, still not getting the profile you are getting.. Maybe it's the size of the page, etc. Can you please check that link? Maybe something got missed while pasting it here

palashkaria · 2020-05-11T13:35:24Z

@Stvad also, I have all other features disabled; only this running. Can you try that too?

Stvad · 2020-05-11T15:51:44Z

@palashkaria hrm, another attempt: https://file.io/xW8m0Gwn

Stvad · 2020-05-11T15:52:19Z

oh, it seems those links are 1-time use

Stvad · 2020-05-11T15:54:43Z

https://send.firefox.com/download/0f95ee924b341ebf/#fk0CxJsBcqEn73JPxpJ9Lw or https://file.io/mtjk1ySO

palashkaria · 2020-05-11T16:03:57Z

Got it, thanks

Also, are you running the other features? or just the live preview?
and what other browser extensions have you got? I'm currently using brave, with minimal extensions & only this feature, I'm not facing this problem as of now.

@Stvad

palashkaria · 2020-05-11T16:22:07Z

Going by the minified source, I see it's triggered from a 'write' function; which seems to be triggered whenever a new block is created; post this, roam seems to do a server sync.

That's why you see this randomly -- it happens when the sync is happening, and the main thread gets busy. This will need further debugging (or atleast a clean repro case). I'm running perf profile in the background to see if I get this

@Stvad

Stvad · 2020-05-11T16:35:54Z

just fyi - tested in incognito mode, with all the other features disabled - same story.

Stvad · 2020-05-11T16:38:03Z

yeah sync makes sense as a culprit - it's run not only when new blocks are created, but when you just add this to existing one, which is what I'm doing

Stvad · 2020-05-11T16:49:38Z

a note - my main DB is rather large (19mb unpacked JSON file), just tried this on the smaller DB - it's far less pronounced, still get small stuttering (with corresponding cpu spikes) here and there, but a lot better then with the main db

Stvad · 2020-05-11T16:51:48Z

another important thing to confirm is that we're doing the same thing while recording the profiles. As mentioned above - the issues occurs while I'm typing the text into the block (not moving mouse, not creating new blocks or links or anything like it, just continuously enter new text) - are you testing the same scenario?

palashkaria · 2020-05-11T16:52:32Z

I just tried in fresh chrome profile, still nothing.. THe worse I get is this; which is still atleast 30fps

Could be due to the db size too, maybe? We should ask other people to test it, if possible

…view

Stvad · 2020-05-16T18:40:29Z

src/ts/contentScripts/live-preview/index.tsx

+}
+checkSettingsAndSetupIframe()
+
+browser.runtime.onMessage.addListener(async message => {


can be moved to dispatcher, as it already does these checks/etc. would need to make it be able to trigger multiple functions though.

@Stvad doing this causes the iframe removal to stop working, because of the same instance problem -- on toggle, it reloads this script, resulting in the instance being null.

If used via onMessage, it works fine.

Changes I made (which caused it to break

in live-preview, remove this browser.runtime.onMessage.addListener handling.

in dispatcher

['settings-updated', () => { updateShortcuts() checkSettingsAndSetupIframeToggle() }

now on toggle, it does not .destroy() as it does not find the instance.

Stvad · 2020-05-16T18:42:24Z

src/ts/contentScripts/live-preview/index.tsx

+let iframeInstance: PreviewIframe | null = null
+
+const setupIframe = (active: boolean) => {
+    if (!iframeInstance) {
+        iframeInstance = new PreviewIframe()
+    }


I think this can potentially be simplified if you are to always create the holding object, but create the actual view/iframe on activate

hmm, that's what you do. I guess alternative consideration is the complexity of managing state if you are to re-use the object. But you seem to be doing a lot of that anyhow..

Stvad · 2020-05-16T18:46:59Z

src/ts/contentScripts/live-preview/index.tsx

+        iframe.style.width = '0'
+        iframe.style.border = '0'
+
+        // styles


nit: comment seems redundant

Stvad · 2020-05-16T18:51:53Z

src/ts/contentScripts/live-preview/index.tsx

+    }
+
+    private removeIframe() {
+        if (this.iframe && document.body.contains(this.iframe)) {


when would those 2 not be true at the same time? (error scenario?) I imagine if that's the case we may still to cleanup the other part? To say it in other words - should this be 2 separate ifs that would cleanup the corresponding thing?

If we try to remove an iframe which doesn't exist in body, there will be an exception. So both of these need to be true at the same time @Stvad the condition is correct

I meant having something like:

if(document.body.contains(this.iframe)) { //false for undefined/null/etc document.body.removeChild(this.iframe) } if (this.iframe) this.iframe = null

@Stvad typescript will throw an error there

@Stvad will need to check for this.iframe first. Updated

hrm, it's now more complicated then before 🙈.
is typescript error because the this.iframe can still potentially be null?

Stvad · 2020-05-16T18:53:18Z

src/ts/contentScripts/live-preview/index.tsx

+    private getIFrameByUrl(url: string): HTMLIFrameElement | null {
+        return document.querySelector(`iframe[src="${url}"]`)
+    }
+    private getIsIFrameVisibleByUrl(url: string): boolean {


isUrlInPreview?

@Stvad this is about being 'visible', not being in preview -- visible is if opacity is set to 1. Maybe isUrlPreviewVisible

Stvad · 2020-05-16T19:45:24Z

src/ts/contentScripts/live-preview/index.tsx

+                this.isHoveredOutFromTarget(target, relatedTarget, iframe) ||
+                this.isHoveredOutFromIframe(target, relatedTarget, iframe) ||
+                !this.isHoveredElementPresentInBody()


make this a function/variable

Stvad · 2020-05-16T19:46:15Z

src/ts/contentScripts/live-preview/index.tsx

+                this.hoveredElement = null
+                this.clearPopupTimeout()
+                this.resetIframeForNextHover(iframe)
+                this.destroyPopper()


make it into hidePreview function

Stvad · 2020-05-16T20:43:30Z

src/ts/contentScripts/live-preview/index.tsx

+    private resetIframeForNextHover(iframe: HTMLIFrameElement) {
+        if (iframe) {
+            if (iframe.contentDocument) {
+                // scroll to top when removed, so the next popup is not scrolled


should this be using scrollToTopHack ?

this is a different scollToTop hack :) extracting to another function

why do they have to be different. would scrolling to the top of html not be enough here too?

@Stvad that is during loading. When a post is loaded, you don't scroll on the html, you scroll on a div inside .roam-center. Need to set the scrolltop of that.

Stvad · 2020-05-16T20:45:30Z

src/ts/roam/navigation.ts

+        if (!uid) {
+            return this.baseUrl().toString()
+        }


what's use case for this?

to get the daily notes page @Stvad

you can just call baseUrl explicitly..

that does not return a string -- don't want to leak .toString() into app code. And we might need the baseUrl as is for some other purpose

not sure what do you mean by leak .toString. you literally use this only to get baseUrl, and there is already function for that 😛
I don't find it intuitive that that this function would silently "succeed" on falsey input =\

I use this to convert eh baseUrl (which is the class URL) to a string. It's not the same thing. @Stvad Should I add a separate function for this? What should it be called?

By leak toString, I mean exactly that. It needs to be in a function here.

added a function getDailyNotesUrl for this @Stvad

hrm. ok 😛

src/ts/utils/settings.ts

Stvad · 2020-05-17T03:54:11Z

src/ts/contentScripts/live-preview/index.tsx

+
+class PreviewIframe {
+    iframeId = 'roam-toolkit-iframe-preview'
+    iframe: HTMLIFrameElement | null = null


how about constructing this (and maybe calling activate in general?) as a part of class constructor, so we can can be sure that it's not null and avoid null-checking it everywhere?

as after all, iframe should exist during the whole lifecycle of this class

Stvad · 2020-05-17T03:56:43Z

src/ts/contentScripts/live-preview/index.tsx

+        if (this.iframe) {
+            this.iframe = null
+        }


actually I don't think we need to explicitly assign this to null. as we discard the whole PreviewIframe instance, this would just be garbage collected

Stvad · 2020-05-17T03:57:15Z

src/ts/contentScripts/live-preview/index.tsx

+
+    private getIFrameByUrl(url: string): HTMLIFrameElement | null {
+        return document.querySelector(`iframe[src="${url}"]`)
+    }


a nit, but can the prettier be configured to require at list 1 space between functions? 😛

@Stvad Haha, I wish 😅 It is not possible in prettier because of the way prettier works (rewriting your code). See this more more info

It turns out that empty lines are very hard to automatically generate. The approach that Prettier takes is to preserve empty lines the way they were in the original source code
https://prettier.io/docs/en/rationale.html#empty-lines

huh, interesting. May I suggest you configure your IDE to do so then ?😛

Stvad · 2020-05-17T03:59:24Z

src/ts/contentScripts/live-preview/index.tsx

+    private getIFrameByUrl(url: string): HTMLIFrameElement | null {
+        return document.querySelector(`iframe[src="${url}"]`)
+    }


so just occurred to me - why do we need to do this if we already have the reference to the instance as a part of the class?

As mentioned, not sure. We get mouse events triggering multiple times, and these are to make sure that we don't add the iframes multiple times. Will not be able to debug and figure out immediately @Stvad

Stvad · 2020-05-17T03:59:36Z

src/ts/contentScripts/live-preview/index.tsx

+
+    private initPreviewIframe() {
+        const url = Navigation.getPageUrl()
+        const existingIframe = this.getIFrameByUrl(url)


when would this happen?

No clear answer. Can happen, have seen mouse events getting triggered multiple times.

Stvad · 2020-05-17T04:01:22Z

src/ts/contentScripts/live-preview/index.tsx

+        const visibleIframe = this.getVisibleIframeByUrl(url)
+        if (visibleIframe) {
+            // if visible, just return the iframe
+            return
+        }


should use class iframe instance

@Stvad will not work -- mouse events get called 3 times

Stvad · 2020-05-17T04:04:33Z

src/ts/roam/navigation.ts

+        if (!uid) {
+            return this.baseUrl().toString()
+        }


not sure what do you mean by leak .toString. you literally use this only to get baseUrl, and there is already function for that 😛
I don't find it intuitive that that this function would silently "succeed" on falsey input =\

Stvad · 2020-05-17T04:08:22Z

src/ts/contentScripts/live-preview/index.tsx

+}
+checkSettingsAndSetupIframe()
+
+browser.runtime.onMessage.addListener(async message => {


use id based check

palashkaria · 2020-05-17T07:17:08Z

@Stvad as discussed, we have the problem of the script getting executed multiple times, and so multiple instances being created. Added a check based on ID for now, and simplified some more things in 9afd28e

palashkaria · 2020-05-17T08:27:02Z

might have found the root cause for this issue; we are loading the same contentscript multiple times -- once directly, once in dispatcher, for eg. that's why when I move the code to dispatcher, it causes it to laod another time. Each content script is supposed to be isolated on it's own. See this for more details:
https://stackoverflow.com/questions/26240463/how-do-i-re-use-code-between-content-scripts-in-a-chrome-extension

Also attaching screenshots of fuzzy_date getting loaded two separate times, this way (see stacktrace):

@Stvad as discussed, we have the problem of the script getting executed multiple times, and so multiple instances being created. Added a check based on ID for now, and simplified some more things in 9afd28e

Stvad · 2020-05-17T20:01:59Z

src/ts/contentScripts/live-preview/index.tsx

-        const url = Navigation.getPageUrl()
-        const existingIframe = this.getIFrameByUrl(url)
+        const url = Navigation.getDailyNotesUrl()
+        const existingIframe = this.getExisitingIframe()
        if (existingIframe) {


I hope we'd be getting rid of this now. but if we were not - I think the assignment would have been still required, to ensure that we operate on the appropriate underlying object

this can be removed after #58 gets merged. I tested by merging with #57 already :)

Stvad · 2020-05-17T20:16:27Z

src/ts/contentScripts/live-preview/index.tsx

+        this.iframe.style.width = '0'
+    }
+
+    private scrollToTopOnMouseOut() {


nit: can you place the scroll to top hacks close to each other and extract commonality between them?

# Conflicts: # src/manifest.json # src/ts/features/features.ts

move/rename scrollToTop hack

Stvad · 2020-05-17T21:48:03Z

src/ts/features/livePreview.tsx

-    } else if (iframeInstance) {
-        iframeInstance.destroy()
-        iframeInstance = null
+    if (active) {


i.e. why change from previous version of this

adds page preview on hover :)

e1e5c57

palashkaria changed the title ~~adds page preview on hover :)~~ adds live page preview on hover :) May 10, 2020

Palash added 2 commits May 17, 2020 00:27

rename function

cece075

Merge commit 'a955330ee4859b3c7dc88b1bf3bb8cbd0c845aa7' into live-pre…

eabd431

…view

palashkaria requested a review from Stvad May 16, 2020 18:59

Stvad reviewed May 16, 2020

View reviewed changes

Palash added 2 commits May 17, 2020 03:22

review changes

bb08eae

fix comment

ea574cd

palashkaria requested a review from Stvad May 16, 2020 21:55

Palash added 5 commits May 17, 2020 03:43

fix: condition

53459da

remove log

b64069e

fix: mouse handling

cc7e463

remove body check

13af95d

review

5a8d720

Stvad reviewed May 17, 2020

View reviewed changes

fix: create iframe in constructor,

9afd28e

use id based check

Palash added 2 commits May 17, 2020 13:28

fix: navigation function

7210520

fix: type

73004cf

palashkaria mentioned this pull request May 17, 2020

Fix contentScripts getting loaded multiple times #57

Closed

Stvad reviewed May 17, 2020

View reviewed changes

Palash added 3 commits May 18, 2020 02:44

Merge remote-tracking branch 'origin/master' into live-preview

f01d18e

# Conflicts: # src/manifest.json # src/ts/features/features.ts

move live-preview file, add to features

caff0e7

remove exisitingIframe check,

aa798b7

move/rename scrollToTop hack

palashkaria requested a review from Stvad May 17, 2020 21:33

Stvad reviewed May 17, 2020

View reviewed changes

Stvad approved these changes May 17, 2020

View reviewed changes

Stvad merged commit 42e407a into roam-unofficial:master May 17, 2020

Stvad added a commit that referenced this pull request May 17, 2020

Fix tslint violation introduced by #52

29dcb99

adds live page preview on hover :) #52

adds live page preview on hover :) #52

Conversation

palashkaria commented May 10, 2020

Stvad commented May 10, 2020

Stvad commented May 10, 2020

Stvad commented May 10, 2020

Stvad commented May 10, 2020

palashkaria commented May 10, 2020

Stvad commented May 10, 2020

palashkaria commented May 10, 2020

palashkaria commented May 10, 2020 • edited Loading

palashkaria commented May 10, 2020

Stvad commented May 10, 2020

Stvad commented May 11, 2020

palashkaria commented May 11, 2020 • edited Loading

Stvad commented May 11, 2020

palashkaria commented May 11, 2020

Stvad commented May 11, 2020

palashkaria commented May 11, 2020

palashkaria commented May 11, 2020 • edited Loading

palashkaria commented May 11, 2020

Stvad commented May 11, 2020

Stvad commented May 11, 2020

Stvad commented May 11, 2020 • edited Loading

palashkaria commented May 11, 2020 • edited Loading

palashkaria commented May 11, 2020 • edited Loading

Stvad commented May 11, 2020

Stvad commented May 11, 2020

Stvad commented May 11, 2020

Stvad commented May 11, 2020

palashkaria commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palashkaria May 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palashkaria May 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palashkaria May 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palashkaria commented May 17, 2020

palashkaria commented May 17, 2020 • edited Loading

Choose a reason for hiding this comment

palashkaria commented May 10, 2020 •

edited

Loading

palashkaria commented May 11, 2020 •

edited

Loading

palashkaria commented May 11, 2020 •

edited

Loading

Stvad commented May 11, 2020 •

edited

Loading

palashkaria commented May 11, 2020 •

edited

Loading

palashkaria commented May 11, 2020 •

edited

Loading

palashkaria May 16, 2020 •

edited

Loading

palashkaria May 16, 2020 •

edited

Loading

palashkaria May 17, 2020 •

edited

Loading

palashkaria commented May 17, 2020 •

edited

Loading