fix: calculate positions for elements inside iframes correctly

[istateside]

Fixes #28

When cy.realClick() or cy.realHover() are called on elements that are inside of an iframe within the Cypress app's own iframe, the position calculated for the element doesn't account for the possible offset position of its containing iframe.

[dmtrKovalenko]

Also it is required to calculate the real size of the iframe (if it was scaled). Just like we are doing already for the upper iframe

[dmtrKovalenko]

I cannot merge PRs without tests. If you want this to be merged please add some tests or wait – I will add some tests myself

[istateside]

I have an update with tests I'll push up here soon, just gotta polish it up a bit.

I don't think my changes cover elements that are being transformed themselves (transform applied to the target element or one of its containers, as opposed to a transform on the <iframe> itself), but I don't think the library covers that case before this PR anyway. I'd prefer to leave that out of this PR and report it as a separate issue

[istateside]

Just pushed up a commit fixing the failing test and rebasing on develop.

Sorry about that, I recently changed some git defaults to not index untracked files by default. I typically work in a very large repo and that change speeds up all my git commands. Still haven't acclimated to it yet, so I keep forgetting to manually add untracked files to my commits.

Let me know what you think of the approach I went with (or take the PR over if you'd prefer), i added some HTML files to test the iframe behavior against.

[istateside]

I'm still getting a failure on the swipe test, for the toTop test. It only fails when run in CI mode, it passes fine with cypress open. @dmtrKovalenko would you be able to look into that for me?

[Andarist]

Just a note regarding .frameElement - it should return null for a cross-origin frame which could be a problem when running with --disable-web-security but I've checked that and it seems that this flag makes .frameElement return the correct element from the parent.

[Andarist]

I'm wondering - couldn't this algorithm be simplified to process the whole thing in a single pass when traversing elements up? This potentially would also allow dropping this Cypress-specific~ line.

[istateside]

Hey, just checking in here. I haven't had time to come back to this, but I'm hoping to put some more time in this upcoming weekend to see if I can figure out why the build is failing (and to respond to the outstanding comments)

[dmtrKovalenko]

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[Andarist]

out of curiosity - is there any situation in which this fallback would actually kick in? note that if it does it might not return the correct iframe as there might be multiple iframes on the page and this doesn't account for that

[dmtrKovalenko]

Yep when running with disable-web-security it follows the original .frameElement.

I didn’t get the last part. This prop returns the frame where window is embedded. It means that it returns the upper frame which is the only that contains this window.

If think this way it may be really possible to remove this branching, but what exact problem do you see here?

[Andarist]

Yep when running with disable-web-security it follows the original .frameElement.

I was the one who has checked how this behaves with disable-web-security and to my slight surprise, it just has returned the "correct" .frameElement despite .frameElement having to return null for cross-origin iframes. So it is as web-security is also responsible for guarding access to .frameElement - so it returns null normally and if we disable it starts to return the correct element.

So using disable-web-security (according to my tests) doesn't influence the logic anyhow - and from what I understand we shouldn't be even able to select cross-origin element in Cypress (when not using disable-web-security) as it's running in the real browser's context so it would just not allow us to do that.

I didn’t get the last part. This prop returns the frame where window is embedded. It means that it returns the upper frame which is the only that contains this window.

I was referring to the fallback logic (currentWindow.parent.document.querySelector("iframe")) - it can just queries the first iframe but the currentWindow.parent.document might contain more than one and thus we can select incorrect one using this.

If think this way it may be really possible to remove this branching, but what exact problem do you see here?

I just usually try to avoid dead branches of the code and was really curious if this is dead or not (the ?? part). I might very well just miss something here. And, as mentioned, even if it's not dead I believe that it might lead to incorrect results at times.

[dmtrKovalenko]

Makes sense. So it needs to be processed by looking for iframe which content window is equal to current frame, yeah it looks correct.

Feel free to open PR, I am on vacation right now so won’t code — but I can review PR and merge if everything is fine

[Andarist]

The question is - how to write a test for this? Under what circumstances are we able to "fail" reading the currentWindow.frameElement?

[dmtrKovalenko]

In any case except cypress is running over chrome with disabled web security I think

[Andarist]

In a regular scenario (without disabled web security) - we can't even get access to cross-origin frames. I've just re-tested this using this repo:

Uncaught DOMException: Blocked a frame with origin "https://example.cypress.io" from accessing a cross-origin frame.

So I think (but not entirely sure) that it's not possible to get access to an element from some inner cross-origin so this entire logic just can't be executed on such an element (no cypress command can).

However, I see that a regular relation between embedding context and an embedded iframe is somehow hacked for the AUT in Cypress. How do you even achieve that? 🤔 I have never actually thought about it as our tests are running against localhost.

And note - a cross-origin AUT has .frameElement === null. So this logic is actually not dead 👍 As it handles the last iframe on the stack (the AUT) and it just happens that the AUT is the first one (at least for now 😉 ).

If I have made any mistake in my whole reasoning - please let me know.

[dmtrKovalenko]

Cypress proxies response of your website for testing so the origin will always be the same. The whole cypress is build over making iframe same origin.

[Andarist]

Cool, that's good to know! Now when you have said that I think I've seen your injection code when debugging - just didn't connect the dots.

Either way - would you agree then that this fallback logic is currently only executed when looking for the frameElement of the AUT? Or are there any other cases as well that come to your mind?

[dmtrKovalenko]

Yes it looks like it exists for aut or the edge case, but you are right it’s mainly for the cypress aut

[istateside]

Just checked in on this and saw it was merged - thanks for taking this over, gonna upgrade in our app today 😃