Closed Bug 1041667 Opened 6 years ago Closed 6 years ago

Fix positioning of the global webrtc sharing indicator if there are multiple screens

Categories

(Firefox :: Site Permissions, defect)

All
Windows 7
defect
Not set
normal
Points:
1

Tracking

()

RESOLVED FIXED
Firefox 34
Iteration:
34.2
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(1 file)

The global sharing indicator that landed in bug 1037408 is sometimes mis-centered when there's an external screen plugged in.

I've seen this only on Mac (where we no longer show that popup-indicator), but I expect the bug to also exist on Windows/Linux.

By using nsIScreenManager.primaryScreen instead of window.screen, we may get a more correct value for the width of the screen.
Flags: firefox-backlog+
Blocks: 1040061
Whiteboard: [sceensharing-uplift]
Attached patch PatchSplinter Review
Now that I've figured out which screen coordinates we want to use while working on bug 1041663, it made sense to also fix this.

This bug is visible only if:
- the Firefox browser window is displayed on the external monitor at the time we show the webrtc indicator.
- the external screen and the primary screen don't have the same width.
Assignee: nobody → florian
Attachment #8466226 - Flags: review?(dolske)
QA Whiteboard: [qa+]
Added to Iteration 34.1
Status: NEW → ASSIGNED
Iteration: --- → 34.1
Comment on attachment 8466226 [details] [diff] [review]
Patch

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

::: browser/base/content/webrtcIndicator.js
@@ +133,5 @@
> +      // Until we have moved the window to y=0, 'screen.width' may give a value
> +      // for a secondary screen, so use values from the screen manager instead.
> +      let width = {};
> +      Cc["@mozilla.org/gfx/screenmanager;1"].getService(Ci.nsIScreenManager)
> +        .primaryScreen.GetRectDisplayPix({}, {}, width, {});

Hmm. When there are multiple screens, what does WebRTC share? Is it just one of the screens (and which one), or does it share everything (union rect)?

If it's sharing only 1 physical screen, the indicator should be centered there. If it's all screens, I'm not sure if there's a clearly preferred place to put the indicator (especially given variations in physical arrangement)... The primary seems reasonable (since that's where the Windows taskbar / OS X menubar is, and presumably similar for Linux). OTOH one could argue for it being the screen where the current Firefox window is, or perhaps having an indicator on every screen to ensure (1) visibility and (2) clarity that everything is being shared.

The last point maybe merits some thinking -- it might feel like overkill (I _think_ most legit uses tend to show your self-sharing view, so it's obvious what's being sent), but it could be a serious privacy issue if someone thinks only one physical screen is being shared, and starts doing something they don't want shared on another screen.

What do other screensharing apps do?
Philipp: any opinion here?
Flags: needinfo?(philipp)
(In reply to Justin Dolske [:Dolske] from comment #3)

> ::: browser/base/content/webrtcIndicator.js
> @@ +133,5 @@
> > +      // Until we have moved the window to y=0, 'screen.width' may give a value
> > +      // for a secondary screen, so use values from the screen manager instead.
> > +      let width = {};
> > +      Cc["@mozilla.org/gfx/screenmanager;1"].getService(Ci.nsIScreenManager)
> > +        .primaryScreen.GetRectDisplayPix({}, {}, width, {});
> 
> Hmm. When there are multiple screens, what does WebRTC share? Is it just one
> of the screens (and which one), or does it share everything (union rect)?

Firefox 33 is only able to share the primary screen.

In the future (34+ but it's not implemented yet), we are hoping to display a list of screens in the permission prompt and to let the user select the screen he wants to share (like we let the user select the application he wants to share). Once we do that, we will probably want to rethink on which screen the sharing indicator should be displayed.

For now, this bug is just about centering correctly the indicator on the screen where it is currently displayed.
Ah, ok, that makes this bug simpler then.
Flags: needinfo?(philipp)
Attachment #8466226 - Flags: review?(dolske) → review+
Comment on attachment 8466226 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 1037408
[User impact if declined]: the global indicator may appear at the wrong place if the browser window is displayed on an external screen.
[Describe test coverage new/current, TBPL]: will be verified by QA once it reaches m-c.
[Risks and why]: Low, self contained change.
[String/UUID change made/needed]: none.
Attachment #8466226 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a4c0a19cf1cf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Hi Florin, can a QA contact be assigned for verification of this bug.
Iteration: 34.1 → 34.2
Flags: needinfo?(florin.mezei)
Attachment #8466226 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(florin.mezei)
QA Contact: florin.mezei → bogdan.maris
I tried to reproduce the initial issue on four different machines using Nightly build (21.07.2014) but with no success. Tried on Windows 7 and Ubuntu OSs.

Steps:
1. Changed the resolution of the secondary screen to 800x600 and the primary to 1920x1080
2. Started Nightly on the secondary screen
3. Visited http://queze.net/goinfre/ff_gum_test.html
4. Click on Video and Share

Actual: The global indicator opens in the primary screen and it`s centered.

Setting this [qa-] for now since I can`t reproduce the initial issue. If anyone else can reproduce on a build before the fix and can then confirm that this is fixed please flip the [qa+] keyword back.
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.