1299751 - [Ubuntu 16.04] Missing mouse pointer from Firefox window share

Status: NEW

(Core :: WebRTC, defect, P3)

Description

[Bogdan Maris, Desktop QA]

Attachment: Screencast showing the issue

[Affected versions]:
- Firefox 49 beta 8
- Latest Developer Edition 50.0a2
- Latest Nightly 51.0a1

[Affected platforms]:
- Ubuntu 16.04 x32 and x64

[Steps to reproduce]:
1. Open Firefox
2. Open https://mozilla.github.io/webrtc-landing/gum_test.html in a new tab
3. Click Window
4. Select the window to share
5. Move mouse in the shared window

[Expected result]:
- Mouse pointer is displayed

[Actual result]:
- Mouse pointer is invisible

[Regression range]:
- This is a recent regression. Here is the mozregression output:

Last good revision: c23ff1725f56734958eb2978040ab36f278a6063
First bad revision: 317aecd284b8a55b335a016a1a5bed3278893c9f
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c23ff1725f56734958eb2978040ab36f278a6063&tochange=317aecd284b8a55b335a016a1a5bed3278893c9f

I think that this is the bug that caused this:
51c543f66795	Randell Jesup — Bug 1042631: Fix Linux mouse position when sharing a window in WebRTC screensharing r=ng

Comment 1

[Andreas Pehrson [:pehrsons]]

I can reproduce this, and I also get interesting positions of the gUM permission prompt.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Ok, with 16.04 I see it as well.  GDB tells me that child_window from XQueryPointer is never coming back as non-0 (aka None), and thus we're being told the pointer isn't in the window.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Unmarking regression since (for Ubuntu 16) this simply replaced one bug (wrong position of the cursor) with a different bug (no cursor).

WONTFIX for 49 since it's too late and the fix isn't simple.  Also this highlights that the fix for Bug 1042631 is incomplete, in that windows that are composited (like an Emacs window with the inner frame aand outer menu-subtitle and borders) doesn't show the cursor in the outer area.

Better would be to change how the compositing is done.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Use topmost Window when selecting a window for sharing

Using the topmost window associated with the picked window works much better - it's consistent (emacs windows for example would share the menubar and contents, while calculator and other 'interior-less' windows would share the decorations).  It fixes the cursor positioning better than the earlier fix, since the cursor compositing now works over the menu bars.  It does expose *all* the WM decorations unfortunately, including dropshadows, but consistently removing the ones we don't want is Not Easy given the wide range of WMs, and we do get the window titles (which are also a decoration).  A very simple fix; almost all the lines of this are moving the GetTopLevelWindow() function to a shared file.   NOTE: this patch assumes we backout bug 1042631 and apply this instead.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8787671 [details] [diff] [review]
Use topmost Window when selecting a window for sharing

As alluded in comment 2, the problem is the interpretation of child_return
from XQueryPointer.

Trying to draw the decorations is difficult because what works with one window
manager will not work with another.

-// WindowCapturer returns window IDs of X11 windows with WM_STATE attribute.
-// These windows may not be immediate children of the root window, because
-// window managers may re-parent them to add decorations. However,
-// XQueryPointer() expects to be passed children of the root. This function
-// searches up the list of the windows to find the root child that corresponds
-// to |window|.

XQueryPointer() doesn't expect to be passed children of the root window, but
isn't quite the API that is required for all the desired information in a
single call.

MouseCursorMonitorX11::Capture() expects the window passed to XQueryPointer to
be a parent of only the captured window.

This patch may help with window managers that provide a single parent window
for the decorations, but some window managers won't do this [1].  Even for
window managers that do provide a consistent parent, whether the cursor shows
over decorations depends on the window manager having a child window covering
these.

Whether the WM is reparenting or not, if there is a virtual root present [2],
then GetTopLevelWindow()/SelectWindow() would end up selecting the entire
virtual root, not just the decorations.

Instead, calling XQueryPointer twice, once with the selected window and once
with the root window would provide enough information to know the cursor
position relative to the selected window, whether the pointer is over a
descendant of the selected window, and whether the pointer is over the
selected window.

MouseCursorMonitor::CreateForWindow() and WindowCapturerLinux::SelectWindow()
can then each operate on the window indicated in their parameter.
The more complicated task of displaying the window decorations, if desired, is
really separate from this bug.  If displaying the title is necessary, then
perhaps better UI could be provided than drawing decorations.

I wonder how necessary it is to hide the cursor when another window is
occluding the selected window.  If not critical, then maybe
CursorState::INSIDE can be passed when unknown, and only one XQueryPointer
call would be required.

EnterWindowMask/LeaveWindowMask events may provide a more efficient accurate
solution to minimize XQueryPointer round trips.  I guess the change in
behavior wrt pointer grabs would be fine, but I'm not certain.  Xlibint.h or
libxcb are other possible ways to perform two operations in one round trip.
However, such optimizations need not be in the first correct implementation.

[1] https://en.wikipedia.org/wiki/Re-parenting_window_manager
[2] https://specifications.freedesktop.org/wm-spec/wm-spec-1.5.html#idm140200477402304

Comment 6

[David Jameson]

This problem also occurs on Macos Sierra with latest Nightly.

Comment 7

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 8

[Bulk Bug Changes for mreavy's org]

Mass change P1->P2 to align with new Mozilla triage process

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information