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

Status: RESOLVED FIXED

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

Description

[Kestrel]

Attachment: PiP window showing titlebar on Ubuntu 19.04

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.

Comment 1

[Tom S [:evilpie]]

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.

Comment 2

[Pascal Chevrel:pascalc (PTO until April 26)]

(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 :)

Comment 3

[Martin Stránský [:stransky] (ni? me)]

I'll look at it.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

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.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

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

Comment 6

[Martin Stránský [:stransky] (ni? me)]

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

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1580733 - Don't use dialog for PIP window on Linux, r=JSON_voorhees

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

Comment 8

[Martin Stránský [:stransky] (ni? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19d1301e4574d4a2ddecbe3ca0ecc9cfc1f8825e

Comment 9

[Martin Stránský [:stransky] (ni? me)]

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.

Comment 10

[Kestrel]

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.

Comment 11

[Tom S [:evilpie]]

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

Comment 12

[Martin Stránský [:stransky] (ni? me)]

(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.

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

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.

Comment 14

[Danny Colin [:sdk]]

(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.

Comment 15

[Martin Stránský [:stransky] (ni? me)]

(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.

Comment 16

[Danny Colin [:sdk]]

(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.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63f9bee4712f
Don't use dialog for PIP window on Linux, r=JSON_voorhees

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4bf8ad9c7de4
Fixed Es lint failure on PictureInPicture.jsm CLOSED TREE

Comment 19

[Tom S [:evilpie]]

+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"
);

Comment 20

[Martin Stránský [:stransky] (ni? me)]

Okay, I'll fix that. Thanks.

Comment 21

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/63f9bee4712f
https://hg.mozilla.org/mozilla-central/rev/4bf8ad9c7de4

Comment 22

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1580733 - Fix AppConstants module getter, r=evilpie

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7dd1ac96a354
Fix AppConstants module getter, r=evilpie,mconley

Comment 24

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/7dd1ac96a354