handle dragging the global webrtc sharing indicator

VERIFIED FIXED in Firefox 33

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Blocks 1 bug)

Trunk
Firefox 34
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox33 verified, firefox34 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Flags: firefox-backlog+
Marco -- Will anyone be working on this bug during this current sprint?  This is an important bug for screen sharing in Fx 33.  Thanks.
Flags: needinfo?(mmucci)
(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.
Flags: needinfo?(mmucci)
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.
Attachment #8461761 - Flags: review?(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;
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...
Flags: needinfo?(philipp)
Whiteboard: [sceensharing-uplift]
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.
Flags: needinfo?(philipp)
Posted patch Patch v2 (obsolete) — Splinter Review
I think this is ready, but I'll test it on more OSes before requesting review.
Assignee: nobody → florian
Attachment #8461761 - Attachment is obsolete: true
Attachment #8461761 - Flags: review?(florian)
Status: NEW → ASSIGNED
Iteration: --- → 34.1
QA Whiteboard: [qa?]
Posted patch Patch v3Splinter Review
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.
Attachment #8463993 - Attachment is obsolete: true
Attachment #8464021 - Flags: review?(dolske)
QA Whiteboard: [qa?] → [qa+]
QA Contact: cornel.ionce
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);
Attachment #8464021 - Flags: review?(dolske) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/40f6d5c240cd
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
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.
Attachment #8464021 - Flags: approval-mozilla-aurora?
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
QA Contact: cornel.ionce → bogdan.maris
Attachment #8464021 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
QA Whiteboard: [qa+] → [qa!]
Whiteboard: [sceensharing-uplift]
Component: General → Device Permissions
You need to log in before you can comment on or make changes to this bug.