Closed Bug 1578985 Opened 4 years ago Closed 3 years ago

Picture-in-Picture does not remember location and size of the popout windows

Categories

(Toolkit :: Video/Audio Controls, enhancement)

Product:
Toolkit
Component:
Video/Audio Controls
Version:
70 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
84 Branch
Tracking Flags:
Tracking Status
firefox84 --- verified

People

(Reporter: heindrich.paul, Assigned: baumga91, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

PictureInPicture_export_20200723.patch
15.95 KB, patch
mconley
: review-
Details | Diff | Splinter Review
Bug 1578985 - Saving the last location and size of Picture in Picture window and opening there the next time. r?mconley,Gijs
47 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:70.0) Gecko/20100101 Firefox/70.0

Steps to reproduce:

Open a Picture-in-Picture video and close it. Reopen it or open a new video and the size of the window and its location is reset to the bottom right corner.

Actual results:

The size and location is reset to the default and original values.

Expected results:

It should remember a user's preference to size and location based on an earlier window.

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → Video/Audio Controls
Product: Firefox → Toolkit
Blocks: videopip
No longer blocks: 1527926

I was about to file a bug for the same request, but while looking at the dependencies of the videopip bug, I'm not sure it's actually desirable in every situation. For example, say you have a dual monitor setup and you have two browser windows, one on each monitor. You open PiP from one of them. Would you want a PiP window open from the other to open at the location of the first one?
On the other hand, if you place a PiP window somewhere, close it, and reopen it from the same video in the same tab in the same window, it's more likely that you do want it where you may have moved it last (that's actually why I came here in the first place).

See Also: → 1581832

Hello
I would try to fix this. I implemented a prototype who seems to work for me on my Fedora Linux.
I will attach the file to show you the actual state.

Please can you assignee this bug to me

Attached patch PictureInPicture_20200406.patch (obsolete) — — Splinter Review

This is a Prototype of how it could work.
I hope the code quality is not to bad

Hi Thomas,

Thanks for taking this on. Instead of using the preferences system to store these values, I think it might make more sense to use XULStore, which is meant to hold things like window position and UI state between restarts.

Here's an example of XULStore being used to set a value: https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/browser/components/distribution.js#558-563

and to get a value: https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/browser/components/places/content/treeView.js#375-379

Here's the documentation for the XULStore interface: https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/toolkit/components/xulstore/nsIXULStore.idl

Where you're doing this work, PictureInPicture.jsm, makes sense - so for XULStore, I suggest using PLAYER_URI as the first argument for setting and getting the values.

Throughout this patch, I'll note that certain things were re-ordered - that generally makes patches harder to review. I highly recommend reducing the changes in the patch to encompass only the things that are required, and not doing any re-ordering unless necessary. If you're ttempting to make the code more consistent with those re-orderings, that might make a good follow-up patch in a separate bug.

Hello Mike

Thanks for your help. I try to do what you sad. I go back with all re-ordered code and use the XULStore to save the Position. I stored the position as single values I hope this is OK.

I tested this patch on Windows and there the position work good I do not see a problems

On Linux are two minor bugs
If the window is in top left the window can moves 2 pixel down when a secound video is loaded then it stay stable. I think the problem coming from a round somewhere handling the parameter layout.css.devPixelsPerPx=1.2 if it is -1.0 it is ok

The second problem can occur in the right button if the aspectRation of the new video is changed it can move some pixel to another position. This I could also see in an officiel firefox version 75 when I move the video to top left and a new video is loaded. I could locate the problem in the function moveTo() if there are sent the same coordinates like if the window open it move the window to wrong position. I see some BUGs for this the best example is 1530495. I could reduce the problem be only use this function if needed.

Can you take a second look to this patch.
What should I do next?

Attachment #9138424 - Attachment is obsolete: true
Attachment #9144484 - Flags: review+
Attachment #9144484 - Flags: feedback+
Flags: needinfo?(mconley)

Hello Mike
How can I help to get this a step further?

Hi Thomas,

I'm afraid I'm currently heads down on another project and don't have cycles to give feedback on this for a little while. I hope to have some time in the next few weeks. Apologies

Hello Mike
Last Weekend something broke I had to make a small fix that on linux the window stay stable on top left.
I see that the pip window now has a border and the problem with the shift of two pixel in the left top seems to gone.

How looks with your time can you take a look into this patch?

Thanks

Attachment #9144484 - Attachment is obsolete: true

Hi Thomas,

I apologize for the long delay. I finally have some time to get back to this. I'll have some review feedback for you today.

Comment on attachment 9165899 [details] [diff] [review]
PictureInPicture_export_20200723.patch

Review of attachment 9165899 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for writing this patch, Thomas. A few things:

It'd be best if this was posted to Phabricator so I can review it there, rather than directly to Bugzilla. Here are instructions: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html - let me know if you need guidance.

I worry that this patch is trying to do too much right out of the gate. I highly recommend we try to start with something much simpler and build on top of it.

For now, let's focus on trying to remember the position of the window, and not worry about the size of the window.

In that case, if I'm understanding your approach correctly, I think what we'll do is identify the nearest corner, and save the X-Y distances relative to that corner to XULStore, to account for display resolution changes. What about if there are multiple displays? Bug 890125, bug 440895 and bug 372650 added support for getting "workspace IDs" on Windows, macOS and Linux. I wonder if we should persist that ID to XULStore too for users that use multiple displays. This is what SessionStore does, for example.

Can you simplify the patch to only store the position of the window, relative to the nearest corner, and also persist/restore the workspace ID? Then we can start thinking about how we want to handle persisting / restoring window dimensions.
Attachment #9165899 - Flags: review-
Flags: needinfo?(mconley)
Assignee: nobody → baumga91
See Also: → 1566494
See Also: → 1567618
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15a1bbaf80e4
Saving the last location and size of Picture in Picture window and opening there the next time. r=mconley
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc07ab3cd238
Saving the last location and size of Picture in Picture window and opening there the next time. r=mconley
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6272305d35d0
Saving the last location and size of Picture in Picture window and opening there the next time. r=mconley
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e444f22bea93
Saving the last location and size of Picture in Picture window and opening there the next time. r=mconley

https://hg.mozilla.org/mozilla-central/rev/e444f22bea93

Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
status-firefox84: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
Flags: qe-verify+

Reproduced the issue with Firefox 71.a1(20190904163258) on Windows 10x64.
The issue is verified fixed with Firefox 84.0b3 (20201119195818) on Windows 10x64, macOS 10.12 and Ubuntu 18.04. The PiP player size and place are remembered when reopened.

Status: RESOLVED → VERIFIED
status-firefox84: fixed → verified
Flags: qe-verify+

Bug happened again from Firefox 86.

Regressions: 1794577
Duplicate of this bug: 1716252
You need to log in before you can comment on or make changes to this bug.