1041663 - handle dragging the global webrtc sharing indicator

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect)

Description

[Florian Quèze [:florian]]

Bug 1037408 landed without support for dragging the indicator because we wanted to land before the Firefox 33 string freeze and were out of time.

Comment 1

[Maire Reavy [:mreavy]]

Marco -- Will anyone be working on this bug during this current sprint?  This is an important bug for screen sharing in Fx 33.  Thanks.

Comment 2

[Marco Mucci [:MarcoM]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #1)
> Marco -- Will anyone be working on this bug during this current sprint? 
> This is an important bug for screen sharing in Fx 33.  Thanks.

Hi Maire, it hasn't been picked up yet and I'll follow up with Gavin and Chad about it.

Comment 3

[:Gijs (he/him)]

Attachment: should be able to drag the global indicator,

Tested on OS X with the menubar bits backed out so I could see this (using my Windows box for something else atm). This seems to work well. I've added a small threshold to avoid having clicks accidentally break the centering of the icon. I picked 10 rather randomly, perhaps it should be tweaked. There's also no drag mouse cursor feedback because there's no specific drag affordance on the indicator - everything is a button - so I figured that'd be confusing. The code is hopelessly stolen and adapted from WindowDraggingUtils.jsm, but so different now that I really don't think it makes sense to tweak the module to support our usecase (clamping, thresholding, not caring about what nodes are between the mouse event and the window parent, and ignoring the Linux-specific GTK hack in there). We should doublecheck this works on GTK because of the latter, though.

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8461761 [details] [diff] [review]
should be able to drag the global indicator,

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

::: browser/base/content/webrtcIndicator.js
@@ +17,5 @@
>    document.title =
>      gStringBundle.formatStringFromName("webrtcIndicator.windowtitle",
>                                         [brandShortName], 1);
>  
> +  window.addEventListener("mousedown", PositionHandler, false);

The third parameter of addEventListener is optional, you can remove it to simplify :-).

@@ +20,5 @@
>  
> +  window.addEventListener("mousedown", PositionHandler, false);
> +  window.addEventListener("unload", function() {
> +    window.removeEventListener("mousedown", PositionHandler, false);
> +  });

Why do we need this unload handler, wouldn't the mousedown event handler be GC'ed at the same time as the window?

@@ +133,5 @@
>    activeStreams[0].browser.ownerDocument.defaultView.focus();
>  }
> +
> +let PositionHandler = {
> +  positionCustomized:  false,

nit: double space.

@@ +138,5 @@
> +  shouldDrag: function(aEvent) {
> +    if (aEvent.button != 0 || aEvent.defaultPrevented)
> +      return false;
> +
> +    return true;

This can be simplified:
 return aEvent.button == 0 && !aEvent.defaultPrevented;

Comment 5

[:Gijs (he/him)]

Philipp, can we make just the Firefox button drag the window? It's rather awkward to drag with the other buttons which have popups, because on e.g. Linux the popup opens on mousedown, so you end up dragging the thing around while the popup is open (which also interferes with the platform dragging code).

There's also the fact that some users are used to clicking and then "dragging" and releasing the mouse to open menu items. It would be hard to distinguish that from a drag motion otherwise...

Comment 6

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Right, I didn't consider the popups. They should be an edge case since they only show up if you're sharing with more than one tab at the same time. But if the behavior completely breaks on some platforms, I'm fine with just using the Firefox button.

Comment 7

[Florian Quèze [:florian]]

Attachment: Patch v2

I think this is ready, but I'll test it on more OSes before requesting review.

Comment 8

[Florian Quèze [:florian]]

Attachment: Patch v3

Turns out we need to take into account screen.availLeft for a correct behavior on Windows if the taskbar has been moved to the left or right side of the screen.

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 8464021 [details] [diff] [review]
Patch v3

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

::: browser/base/content/webrtcIndicator.js
@@ +68,5 @@
>  
>    // Resize and center the window.
>    window.sizeToContent();
> +  if (!PositionHandler.positionCustomized) {
> +    window.moveTo((screen.width - document.documentElement.clientWidth) / 2, 0);

Seems like it would be good to have this code consistent with the setXPosition() code... Specifically, using availLeft and availWidth to determine the midpoint.

Bonus points for just doing:

  if (!PositionHandler.positionCustomized)
    PositionHandler.setDefaultPosition();
  else
    PositionHandler.setXPosition(window.screenX);

Comment 10

[Florian Quèze [:florian]]

(In reply to Justin Dolske [:Dolske] from comment #9)

> > +  if (!PositionHandler.positionCustomized) {
> > +    window.moveTo((screen.width - document.documentElement.clientWidth) / 2, 0);
> 
> Seems like it would be good to have this code consistent with the
> setXPosition() code... Specifically, using availLeft and availWidth to
> determine the midpoint.

I had thought about it, and decided that we probably wanted to put the indicator at the center of the screen (which means using 'width' rather than availWidth and availLeft) rather than at the center of the available area. Of course the difference will only be noticeable if you've got a large taskbar on the left or right side of your screen.
If you think strongly otherwise I don't mind changing it.

For bound checks, we do want the available area though.

> Bonus points for just doing:
> 
>   if (!PositionHandler.positionCustomized)
>     PositionHandler.setDefaultPosition();
>   else
>     PositionHandler.setXPosition(window.screenX);

Maybe we could even put all of these lines inside the PositionHandler object.

Comment 11

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/40f6d5c240cd

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/40f6d5c240cd

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8464021 [details] [diff] [review]
Patch v3

Approval Request Comment
[Feature/regressing bug #]: bug 1037408
[User impact if declined]: the global indicator may hide an important area of the user's screen.
[Describe test coverage new/current, TBPL]: currently in m-c.
[Risks and why]: Low, self contained change.
[String/UUID change made/needed]: none.

Comment 14

[Bogdan Maris, Desktop QA]

Tested on Windows 7 64bit, Windows 8.1 Surface Pro 2 and Ubuntu 12.04 32bit using latest Nightly 34.0a1, the indicator can be dragged from left to right. Found one issue that will be tracked separately, marking this verified as fixed based on my testing. Please flip back the [qa+] keyword as soon as the fix lands in Aurora. 
Thanks.

Comment 15

[Florian Quèze [:florian]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f950b4da1887

Comment 16

[Bogdan Maris, Desktop QA]

Tested on Windows 7 64bit, Windows 8.1 Surface Pro 2 and Ubuntu 12.04 32bit using latest Aurora 33.0a2, the indicator can be dragged from left to right.