Closed Bug 1580733 Opened 1 year ago Closed 5 months ago

Picture-in-picture video window has titlebar decorations on Linux

Categories

(Core :: Widget: Gtk, defect, P2)

71 Branch
Unspecified
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox69 --- unaffected
firefox70 --- unaffected
firefox71 --- fixed

People

(Reporter: ke5trel, Assigned: stransky)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files)

Since the picture-in-picture window type was changed to a dialog (Bug 1543027), it now correctly stays on top with Wayland (Bug 1536747) on Ubuntu 19.04 but it also has a titlebar which it did not have before.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b5d436985831449e4f160b10abc362d937a6486c&tochange=969b3828a301038c57540d8bffb51539132d393c

Regressed by Bug 1543027.

Blocks: 1532677
No longer blocks: 1527926

For some reason mBorderStyle (changed by titlebar=no) is only used for eWindowType_popup and not eWindowType_dialog. (source)
Someone with more GTK experience really should take a look at nsWindow::Create and maybe simplify that whole function.

(In reply to Tom Schuster [:evilpie] from comment #1)

For some reason mBorderStyle (changed by titlebar=no) is only used for eWindowType_popup and not eWindowType_dialog. (source)
Someone with more GTK experience really should take a look at nsWindow::Create and maybe simplify that hole function.

Let's ask Martin about it :)

Flags: needinfo?(stransky)

I'll look at it.

Flags: needinfo?(stransky)

The PIP window also does not have any name on the taskbar on Linux - we should set something to enable move/close that window or disable to show it on a taskbar.

I think there are more fixes needed on Linux:

  • update the window decoration
  • keep window ratio which is not honored now
  • set window name or disable to show it on taskbar
  • display system menu on right mouse click on Wayland to allow place it on top
Depends on: 1583737

I think Bug 1543027 should be reverted for Linux as Dialog is not suitable for this.

Priority: -- → P2

Dialog window on Linux/Gtk adds extra border/titlebar to PIP window
so don't use it there.

Assignee: nobody → stransky

We should not use dialog for PIP window. The borders can be disabled but the window also can't be resized then. Better to keep the previous window type and fix the remaining errors.

I'd rather keep the current dialog window which stays on top of browser windows until we are sure it can be done programmatically with non-dialog windows. Everything else is polish compared with always on top functionality.

Always-on-top already works for me with non-dialog windows (bug 1532103). Is this about Wayland or some specific window manager?

(In reply to Kestrel from comment #10)

I'd rather keep the current dialog window which stays on top of browser windows until we are sure it can be done programmatically with non-dialog windows. Everything else is polish compared with always on top functionality.

The behavior you see with dialog window is wrong.

Dialog window is intended for simple Yen/No modal windows and are keep above only for particular toplevel browser window which is the dialog related to, not all other windows. It's semi-on top behavior which is not intended and may confuse users. Also we can't set proper window decorations for dialog windows without functionality loss (missing resizers).

The former behavior (top-level window) will be on top on X11 Bug 1532103. Wayland does not allow application place itself on top by design so I'm going to implement a functionality which shows titlebar menu when right mouse click is performed on a PIP window and then it can be truly set on top by Wayland compositor.

Marking as fix-optional for 71 to remove it from our regression triage queue as we will not ship the feature for Linux in a later release.

Depends on: 1583852
No longer depends on: 1583852
No longer depends on: 1583737

(In reply to Martin Stránský [:stransky] from comment #12)

The former behavior (top-level window) will be on top on X11 Bug 1532103. Wayland does not allow application place itself on top by design so I'm going to implement a functionality which shows titlebar menu when right mouse click is performed on a PIP window and then it can be truly set on top by Wayland compositor.

AFAIK, this solution doesn't work with a tiling window manager because the window never sends the necessary hints to let the WM know how to
position the window.

I have the exact same problem with lots of other windows in FF like the "About Firefox" that open tiled by default instead of on-top.

(In reply to Danny Colin [:sdk] from comment #14)

(In reply to Martin Stránský [:stransky] from comment #12)

The former behavior (top-level window) will be on top on X11 Bug 1532103. Wayland does not allow application place itself on top by design so I'm going to implement a functionality which shows titlebar menu when right mouse click is performed on a PIP window and then it can be truly set on top by Wayland compositor.

AFAIK, this solution doesn't work with a tiling window manager because the window never sends the necessary hints to let the WM know how to
position the window.

I have the exact same problem with lots of other windows in FF like the "About Firefox" that open tiled by default instead of on-top.

Let's fix that properly by setting correct WM class you need to. Let's fix that at Bug 1532096.
Also Bug 397251 may be related.

(In reply to Martin Stránský [:stransky] from comment #15)

Let's fix that properly by setting correct WM class you need to.

Sounds good to me.

Keywords: checkin-needed

Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/63f9bee4712f
Don't use dialog for PIP window on Linux, r=JSON_voorhees

Keywords: checkin-needed
Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4bf8ad9c7de4
Fixed Es lint failure on PictureInPicture.jsm CLOSED TREE
+const { XPCOMUtils } = ChromeUtils.import(
+  "resource://gre/modules/XPCOMUtils.jsm"
+);
+
+XPCOMUtils.defineLazyModuleGetters(this, {
+  AppConstants: "resource://gre/modules/AppConstants.jsm",
+});

should really be.

const { AppConstants } = ChromeUtils.import(
  "resource://gre/modules/AppConstants.jsm"
);

Okay, I'll fix that. Thanks.

Flags: needinfo?(stransky)
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Flags: needinfo?(stransky)
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---

Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7dd1ac96a354
Fix AppConstants module getter, r=evilpie,mconley

Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 5 months ago5 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.