getOverflowAncestors: traverse into iframe parent#2535
getOverflowAncestors: traverse into iframe parent#2535atomiks merged 4 commits intofloating-ui:masterfrom
Conversation
🦋 Changeset detectedLatest commit: 61ac08d The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for vibrant-gates-22c214 canceled.
|
| win.visualViewport || [], | ||
| isOverflowElement(scrollableAncestor) ? scrollableAncestor : [] | ||
| isOverflowElement(scrollableAncestor) ? scrollableAncestor : [], | ||
| win.frameElement ? getOverflowAncestors(win.frameElement) : [], |
There was a problem hiding this comment.
This could use a unit test - there is already an existing test suite for getOverflowAncestors().
We might need to configure loading of subresources because jsdom by default won't load any subresources like iframes, and for an element inside the iframe, node.ownerDocument.defaultView will be equal to the parent window object.
There was a problem hiding this comment.
I added a unit test. Working with iframes in JSDOM turned out to be fine. I don't load any document, just add elements to iframe.contentDocument. And the two Window objects are distinct.
There was a problem hiding this comment.
Nice, great to hear that it wasn't a problem 👍
|
|
||
| document.body.append(scroll); | ||
| scroll.append(iframe); | ||
| iframe.contentDocument!.body.append(test); |
There was a problem hiding this comment.
We may need a no-non-null-assertion ignore here.
There was a problem hiding this comment.
iframe.contentDocument can be null, but by appending the element into the document.body structure, we are "attaching" it and it gets loaded. Then contentDocument and contentWindow get a reliable non-null value. But this certaintly can't be expressed in types.
There was a problem hiding this comment.
This looks good, but to prevent warnings appearing here, can you appease the checker?
| win.visualViewport || [], | ||
| isOverflowElement(scrollableAncestor) ? scrollableAncestor : [] | ||
| isOverflowElement(scrollableAncestor) ? scrollableAncestor : [], | ||
| win.frameElement ? getOverflowAncestors(win.frameElement) : [], |
There was a problem hiding this comment.
Nice, great to hear that it wasn't a problem 👍
| ]); | ||
| }); | ||
|
|
||
| test('returns overflow ancestors in iframe parents', () => { |
|
Thanks @atomiks for merging the patch so quickly 👍 There might be, however, something wrong with the release. The |
|
oh 🤦 I forgot that function was moved out into the |
Fixes #2518.
When
getOverflowAncestorsreaches thewindowobject, it checks whether there iswindow.frameElementand continues traversing up into the iframe's parent.TODO before this is shippable:
getClippingElementAncestors. Do we also want to traverse into iframe's parent there?