Conversation
🦋 Changeset detectedLatest commit: 2451310 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| // bounds is reported as being in PiP. | ||
| const pipWindow = window.documentPictureInPicture?.window; | ||
| if (pipWindow) { | ||
| return isDocumentInPiPWindow(el.ownerDocument, pipWindow) && isElementInViewport(el, pipWindow); |
There was a problem hiding this comment.
🟡 Offscreen PiP frames keep video active
For a video inside a PiP iframe, isElementInPiP compares frame-local offsets with the top-level PiP viewport. The iframe's position and clipping are ignored, so videos inside offscreen frames return true and keep adaptive streaming active.
Learn more
Elements inside an iframe use coordinates relative to that iframe's document. The PiP window uses coordinates relative to its top-level document. Passing the PiP window to isElementInViewport compares those incompatible coordinate spaces and never tests whether the iframe itself is visible. The resulting pictureInPicture value bypasses background pausing in updateVisibility.
Example: Place an iframe at top: 2000px in an 820-pixel-high PiP document, then place a video at top: 10px inside the iframe. The ancestry check succeeds, and 10px intersects the PiP viewport, although the entire iframe is offscreen. The track remains subscribed instead of becoming hidden.
Recommended fix: Measure the element in its owner window, then project and intersect its rectangle through each ancestor iframe up to pipWindow. Include each frame's position, scrolling, and clipping before returning true.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Accurate as a description of current behavior, but it predates this PR rather than being introduced by it.
Before this change isElementInPiP called isElementInViewport(el, pipWindow) with no ancestry check at all. In your example the video's frame-local top: 10px was compared against the PiP window's 820px height exactly as it is now, so it already returned true. This PR adds the ancestry gate and leaves measurement untouched.
The PR description calls this out under "Known limitation, unchanged by this PR". Measuring in the element's own window (el.ownerDocument.defaultView ?? pipWindow) would be correct within a frame and identical for the direct case, but it still would not account for the frame's own position or clipping. Doing that properly needs per-frame rect projection and intersection up to pipWindow, which is a larger change than this fix and is better kept separate.
Happy to open a follow-up issue for the frame geometry if that would be useful.
isElementInPiP decided Document PiP membership from geometry alone, so an element still in the opener document was reported as being in PiP whenever its opener-document coordinates fell inside the PiP window's bounds. Since HTMLElementInfo.visible is isPiP || isIntersecting, such an element also counted as visible while scrolled out of view and escaped pauseVideoInBackground. The element now has to live in the PiP window's document, or in a frame nested inside it -- the spec permits frames in a PiP window, so an element in one is genuinely in PiP. Ancestry is established by top-window identity, which stays readable across origins, with a same-origin frameElement walk behind it. That matters for PiP document (origin A) -> frame (origin B) -> frame (origin A): the innermost frameElement reads null, but top is still the PiP window. An opener element tops out at the opener, so it is rejected either way. Measurement is unchanged. isElementInViewport still walks offsetParent, which stops at a frame's own document root, so a nested frame is measured in frame-relative coordinates against the PiP window's viewport. That was true before this change too; projecting rectangles through ancestor frames is a separate piece of work.
5c50bea to
2451310
Compare
2 participants
Fixes #2105.
isElementInPiPdecided Document PiP membership from geometry alone, so an element still in the opener document was reported as being in PiP whenever its opener-document coordinates fell inside the PiP window's bounds. BecauseHTMLElementInfo.visibleisisPiP || isIntersecting, such an element also counted as visible while scrolled out of view and escapedpauseVideoInBackground.The check now requires the element to live in the PiP window's document, or in a frame nested inside it. Frames are included because the spec permits them in a PiP window, so an element in one is genuinely in PiP. The ancestry walk terminates at the opener document, whose
frameElementis null, so opener elements and opener-owned frames are still rejected. A cross-originframeElementaccess throws; that is caught and treated as "ancestry cannot be established", not as in-PiP.Tests
Seven cases, all driven through
RemoteVideoTrack.attach()and assertingTrackEvent.VisibilityChanged, with the opener backgrounded so the defaultpauseVideoInBackgroundreports hidden unless the element is genuinely in PiP. The movement case creates and attaches the element in the opener, appends it into the PiP document after the enter event, and assertsownerDocumentactually changed.Each test was checked against the revision it guards:
ownerDocument === pipWindow.documentonEnterPiPdeferral removedThe third row covers #1868's deferral, which the previous tests could not have caught. That code is untouched.
Known limitation, unchanged by this PR
isElementInViewportwalksoffsetParent, which stops at the frame's own document root, then compares frame-relative offsets against the PiP window's dimensions. For a nested frame that is not meaningful geometry; it reads true because frame offsets are small and PiP windows are large. This PR only gates on ancestry and leaves measurement exactly as it was. Measuring against the window that lays the element out (el.ownerDocument.defaultView ?? pipWindow) would be correct within a frame and identical for the direct case, but it changes measurement semantics, so I left it out. Clipping through ancestor frames would need per-frame rect intersection.