In bug 1535354, I added a toggle to trigger Picture-in-Picture that sits on top of <video> elements when the mouse hovers them. The technical details of how this more or less works can be found in bug 1535354 comment 1.

Having had a few days to test the toggle on a few different sites on a few different computers, I've found some deficiencies with the approach. The major ones are:

1. It's possible for the toggle to appear to "drift" away from the <video> element. For example, view any Twitch video, and when the toggle is visible, scroll the section of the page with the video on it. Because the region is scrollable, but not the top-level document, the toggle appears to separate from the <video> since the toggle is in top-level AnonymousContent that's absolutely positioned.

2. The toggle doesn't always appear when it should. In order to avoid checking mousemove and click events unnecessarily, I use an IntersectionObserver to notice when a <video> is within the viewport of the page, and only then do I observe mousemove and click events. However, the IntersectionObserver code I'm using is configured to be pretty strict, and only allows the toggle to appear if the whole video is viewable when the IntersectionObserver fires. This, unfortunately, isn't always the case, so there are often times when the toggle just doesn't appear for some reason.

If you look in bug 1535354 comment 1, I originally wanted to put the toggles in the video controls UAWidget bindings, but bailed out of that idea once I realized that mouse events might not reach the binding. I think I want to revisit this idea, but combine it with the technique that the current toggle uses to get the best of both worlds.

My idea is:

1. For the <video controls> case, add the Picture-in-Picture toggle (and flyout toggle) as another control on the binding that obeys normal mouse events. Assume that most sites that put their own controls on top of a <video> and capture clicks do not use <video controls>, and don't do any special handling.

2. For the <video> case without controls, have a special binding for desktop that only displays the toggle / flyout toggle. However, the PictureInPictureToggle class inside of PictureInPictureChild.jsm will be responsible for showing and hiding the toggle, by using the same IntersectionObserver and mousemove-checking technique that the current toggle uses. Effectively:

3. If a <video> with no controls intersects the viewport, begin tracking mousemove and click events

4. If a mousemove occurs over top of a video without controls, fire a chrome-only event on that <video> that the binding can pick up to know to display the toggle (and have a similar technique for hiding the toggle).

5. On a click event, have PictureInPictureChild look at the mouse position, and determine if the click occurred on the currently hovered video. If it did, get the rect for the currently displayed toggle, and determine if the click happened within that rect. If it did, fire the event that kicks of PictureInPicture.

The only major drawback (besides the fact that I've got to refactor a bunch of stuff I just landed) that I can see here is that there might be cases where a site visually overlaps the toggle, and if the user clicks on them, we'll enter Picture-in-Picture despite the user not seeing the toggle.

What do you think of this approach, jaws?

(In reply to Mike Conley (:mconley) (:⚙️) from comment #0)

My idea is:

1. For the <video controls> case, add the Picture-in-Picture toggle (and flyout toggle) as another control on the binding that obeys normal mouse events. Assume that most sites that put their own controls on top of a <video> and capture clicks do not use <video controls>, and don't do any special handling.

Do we need the flyout toggle in for <video controls/>? I think we can get by with only an extra button in the video controls toolbar for popping out the video and not do the flyout toggle here.

2. For the <video> case without controls, have a special binding for desktop that only displays the toggle / flyout toggle. However, the PictureInPictureToggle class inside of PictureInPictureChild.jsm will be responsible for showing and hiding the toggle, by using the same IntersectionObserver and mousemove-checking technique that the current toggle uses. Effectively:

3. If a <video> with no controls intersects the viewport, begin tracking mousemove and click events

We should probably drop the intersection threshold from 1.0 to 0.5.

2. If a mousemove occurs over top of a video without controls, fire a chrome-only event on that <video> that the binding can pick up to know to display the toggle (and have a similar technique for hiding the toggle).

3. On a click event, have PictureInPictureChild look at the mouse position, and determine if the click occurred on the currently hovered video. If it did, get the rect for the currently displayed toggle, and determine if the click happened within that rect. If it did, fire the event that kicks of PictureInPicture.

This sounds like a lot of re-implementing of our events. If the Picture-in-Picture toggle/flyout is implemented in nsVideoFrame this may be a lot simpler. You could look at our captions are implemented as an example.

(In reply to (behind on needinfos) Jared Wein [:jaws] (please needinfo? me) from comment #2)

Do we need the flyout toggle in for <video controls/>? I think we can get by with only an extra button in the video controls toolbar for popping out the video and not do the flyout toggle here.

For now, let's assume that for consistency's sake, we need both the normal and flyout toggles as they are for both <video controls/> and <video/>. I'll double-check with Emanuela from UX, but for now, let's just assume this is the case.

We should probably drop the intersection threshold from 1.0 to 0.5.

Sure, that's a fine idea.

This sounds like a lot of re-implementing of our events. If the Picture-in-Picture toggle/flyout is implemented in nsVideoFrame this may be a lot simpler. You could look at our captions are implemented as an example.

I see that nsVideoFrame inserts anonymous content into itself. Is that what you mean? I'm unsure how this makes things simpler - can you elaborate a bit on that?

(In reply to Mike Conley (:mconley) (:⚙️) from comment #0)

The only major drawback (besides the fact that I've got to refactor a bunch of stuff I just landed) that I can see here is that there might be cases where a site visually overlaps the toggle, and if the user clicks on them, we'll enter Picture-in-Picture despite the user not seeing the toggle.

Speaking with emilio and mstange about this, I think I can work around this by adding FrameForPointOption::OnlyVisible support to nsIDOMWindowUtils.nodesFromRect. Check to see if the <video> is in the returned set. If so, then nothing is obscuring the region of the video that the user just clicked on, and we should then do the work to determine if the user clicked on the toggle.

I spoke to jaws over IRC, and we weighed the pros and cons of inserting the toggle using nsIAnonymousContentCreator API on nsVideoFrame, and it seemed like there were a few difficulties:

1. The flyout toggle uses a string from Fluent, so we'd either need to move that string back to a .dtd or .properties file to access it synchronously, or do some heroics to support async string availability for this thing

2. Doing the hit testing with the specially prepared nodesFromRect won't work for this kind of anonymous content, I don't think.

So we're going to try putting these toggles on the videocontrols bindings.

Going to break this up into a few separate bugs.

https://hg.mozilla.org/mozilla-central/rev/7900dcbab822

https://hg.mozilla.org/mozilla-central/rev/7900dcbab822