Skip to content

Use window.top and window.top.document#294

Open
eoghanmurray wants to merge 6 commits intorrweb-io:masterfrom
statcounter:window-top
Open

Use window.top and window.top.document#294
eoghanmurray wants to merge 6 commits intorrweb-io:masterfrom
statcounter:window-top

Conversation

@eoghanmurray
Copy link
Contributor

@eoghanmurray eoghanmurray commented Aug 20, 2020

This pull request is to break out of simple same-domain <iframe>s, i.e. when rrweb.js is loaded from within the iframe.

This is the case for GoDaddy.

I'm not sure whether this is generally a good idea for all of rrweb's users, so maybe it needs further consideration before being accepted?

@Juice10
Copy link
Member

Juice10 commented Aug 20, 2020

Great work eoghanmurray!
I’m wondering if this could potentially work cross-domain, for example in electron if rrweb is injected in all the iframes.

@eoghanmurray eoghanmurray force-pushed the window-top branch 3 times, most recently from 136f8df to c42cfb5 Compare August 21, 2020 13:12
@eoghanmurray
Copy link
Contributor Author

eoghanmurray commented Aug 21, 2020

@Juice10 I hadn't envisioned that usecase and haven't tested that.

I just came across this since reading your comment:
rrweb-io/rrweb-snapshot#10
Maybe that is unrelated?

Do you have a chance to test out what would happen with multiple iframes in the same document?

@Juice10
Copy link
Member

Juice10 commented Aug 25, 2020

I just came across this since reading your comment:
rrweb-io/rrweb-snapshot#10
Maybe that is unrelated?

@eoghanmurray I'm not sure to be honest.

Do you have a chance to test out what would happen with multiple iframes in the same document?

Yes I'd be happy to, anything specific you'd like me to do? Just do a recording with this branch? Any specific pages you'd like me to try out?

One thing that might make electron based recording a little trickier is that every iframe/browser-window has its own rrweb.record({ emit(event) { /* send to central location */ } }); so electron receives events from multiple pages at the same time without knowing which one is which.

@eoghanmurray
Copy link
Contributor Author

eoghanmurray commented Aug 25, 2020

The electron (https://www.electronjs.org/ ?) application of rrweb is definitely not on my radar, I was only thinking of the web; so I guess a run through it's paces would be great to make sure this change doesn't break things there.

@Juice10 Juice10 mentioned this pull request Sep 4, 2020
Closed
@eoghanmurray
Copy link
Contributor Author

eoghanmurray commented Sep 24, 2020

Should this be a configuration option for it to be accepted?

@eoghanmurray eoghanmurray mentioned this pull request Nov 23, 2020
Closed
@eoghanmurray
Copy link
Contributor Author

eoghanmurray commented Jun 15, 2021

Does #481 change it so that it starts with the top level document, or is it just in whatever document that rrweb is initiated?

rrweb/src/record/index.ts

Lines 439 to 442 in 1dd91f0

const init = () => {
takeFullSnapshot();
handlers.push(observe(document));
};

I think the latter is the case so maybe this pull request is still relevant?

@eoghanmurray
Copy link
Contributor Author

eoghanmurray commented Sep 29, 2021 •
edited
Loading

I've done a full rework as this PR was originally authored before we had nested iframe support (#481)

This is also based on #716, although that is still in progress, as they are related.

Now with a working test!

eoghanmurray referenced this pull request in fouad1998/rrweb Sep 29, 2021
@fouad1998
working on different browsers
41b243f
@eoghanmurray
Remove click event as it produces variability in test output in diffe…
9829fca 
…rent environments (I can't see how the `positions` attribute is normalised away in record.test.ts if indeed it is)
@eoghanmurray
Pass in IWindow type; necessary after 53492c1 although I don't fully …
8a3af67 
…understand it
@vlazdimir vlazdimir mentioned this pull request Jul 28, 2022
Merged
@samber
Copy link
Contributor

samber commented Oct 10, 2022 •
edited
Loading

Any update regarding this PR?

How can we move things forward?

I wonder if it would be better to add window as a property of recordOptions? 🤔 It would let the developer pick the right window: window, window.parent, window.top or child iframe.

@samber samber mentioned this pull request Oct 15, 2022
Open
@eoghanmurray
Copy link
Contributor Author

eoghanmurray commented Jan 19, 2023

I wonder if it would be better to add window as a property of recordOptions? thinking It would let the developer pick the right window: window, window.parent, window.top or child iframe.

I think both will be good, but I think it should default to the highest window it can find

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@eoghanmurray @Juice10 @samber