show a preview when the user selects a screen/window/application to share

VERIFIED FIXED in Firefox 52

Status

()

Firefox
Device Permissions
P2
normal
VERIFIED FIXED
10 months ago
3 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

unspecified
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 months ago
As part of the screensharing whitelist removal work, we want to implement this new permission UI that shows a live preview of the window the user is about to share: https://mozilla.invisionapp.com/share/AF71R266U#/screens/152725629
(Assignee)

Updated

10 months ago
Depends on: 1284909
(Assignee)

Updated

10 months ago
Depends on: 1284680, 1284683
P1 when we are unblocked.
Priority: -- → P2

Updated

9 months ago
Whiteboard: [fxprivacy][triage] → [fxprivacy]

Updated

8 months ago
Flags: qe-verify?
(Assignee)

Updated

8 months ago
Assignee: nobody → florian
Flags: qe-verify? → qe-verify+
(Assignee)

Comment 2

7 months ago
Created attachment 8794223 [details] [diff] [review]
WIP v1

WIP, but most of the way there :-).

What remains to do:
- cleanup streams when the panel is closed / when selecting a different window
- maybe some theming polish, need to look again at the mockup
- see if some of this can be covered by automated tests.

If you want to try this, you need to apply first the r-'ed patch from bug 1284680.
(Assignee)

Comment 3

6 months ago
Created attachment 8804812 [details] [diff] [review]
WIP v2

This doesn't leave streams behind anymore, and includes the hack we decided to use in bug 1284680.
(Assignee)

Updated

6 months ago
Attachment #8794223 - Attachment is obsolete: true
(Assignee)

Comment 4

6 months ago
I filed bug 1313324 to ensure this is well covered by tests. The reason for not handling testing in this bug is that this needs to land before bug 1284878, which will contain a couple localized strings. We want to land the screensharing whitelist removal before 52 merges to aurora, so that we don't have to support the whitelist for all of the next ESR cycle, which will be based on 52. The time before 52 merges to aurora is short at this point, and tests is something we can uplift, so I think it makes sense to prioritize landing all the UI pieces first. I'll get the test coverage sorted out before 52 reaches beta.
(Assignee)

Comment 5

6 months ago
Created attachment 8805070 [details] [diff] [review]
Patch v3

Removed a leftover debugging dump. I think this is ready for review, as I'll handle testing in a separate bug, as explained in comment 4.
Attachment #8805070 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

6 months ago
Attachment #8804812 - Attachment is obsolete: true
Comment on attachment 8805070 [details] [diff] [review]
Patch v3

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

::: browser/modules/ContentWebRTC.jsm
@@ +292,5 @@
>    }
>  
>    for (let contentWindow of contentWindows) {
> +    if (contentWindow.document.documentURI == kBrowserURL) {
> +      // There may be a preview showed at the same time as other streams.

Nit: shown

::: browser/modules/webrtcUI.jsm
@@ +375,5 @@
>        }
>  
> +      // Clean-up video streams of screensharing previews.
> +      if ((aTopic == "dismissed" || aTopic == "removed") &&
> +          requestTypes.indexOf("Screen") != -1) {

requestTypes.includes("Screen")

@@ +511,5 @@
>          chromeDoc.getElementById("webRTC-selectWindow-menulist").removeAttribute("value");
>          chromeDoc.getElementById("webRTC-all-windows-shared").hidden = true;
> +        if (menupopup._commandEventListener)
> +          menupopup.removeEventListener("command", menupopup._commandEventListener);
> +        menupopup._commandEventListener = event => {

How is this removed if prompt() is not called again? Looks like the menupopup stays in the DOM, but really this listener should go away when the overall prompt disappears (e.g. in the same handler where you 'clean up' the video), or its references in this app-global JSM might leak the window in question...

@@ +533,5 @@
> +
> +          let constraints = { video: { mediaSource: type, deviceId: {exact: deviceId } } };
> +          let chromeWin = chromeDoc.defaultView;
> +          chromeWin.navigator.mediaDevices.getUserMedia(constraints).then(stream => {
> +            video.src = chromeWin.URL.createObjectURL(stream);

This races - the event listener could have been called again by now, for a different item.

::: browser/themes/shared/notification-icons.inc.css
@@ +154,5 @@
>  .screen-icon.blocked-permission-icon {
>    list-style-image: url(chrome://browser/skin/notification-icons.svg#screen-blocked);
>  }
>  
> +html|*#webRTC-previewVideo {

Do you need the html| for this to work? I would have expected just the ID selector to be fine, and for you to only need to namespace the tagname, which isn't relevant here.

@@ +158,5 @@
> +html|*#webRTC-previewVideo {
> +  width: 300px;
> +  /* If we don't set the min-width, width is ignored. */
> +  min-width: 300px;
> +  max-height: 200px;

Will this warp if used with a screen/window that is higher than it is wide ("portrait"), as you're forcing the width to be big and the height to be small?
Attachment #8805070 - Flags: review?(gijskruitbosch+bugs) → review-
(Assignee)

Comment 7

6 months ago
Created attachment 8806832 [details] [diff] [review]
Patch v4

(In reply to :Gijs Kruitbosch from comment #6)

> ::: browser/modules/webrtcUI.jsm
> @@ +375,5 @@
> >        }
> >
> > +      // Clean-up video streams of screensharing previews.
> > +      if ((aTopic == "dismissed" || aTopic == "removed") &&
> > +          requestTypes.indexOf("Screen") != -1) {
>
> requestTypes.includes("Screen")

The reason why I used indexOf was that there's another indexOf call a few lines above, and I like consistency. I've now changed all 3 occurrences to use includes instead.

> @@ +511,5 @@
> >          chromeDoc.getElementById("webRTC-selectWindow-menulist").removeAttribute("value");
> >          chromeDoc.getElementById("webRTC-all-windows-shared").hidden = true;
> > +        if (menupopup._commandEventListener)
> > +          menupopup.removeEventListener("command", menupopup._commandEventListener);
> > +        menupopup._commandEventListener = event => {
>
> How is this removed if prompt() is not called again? Looks like the
> menupopup stays in the DOM, but really this listener should go away when the
> overall prompt disappears (e.g. in the same handler where you 'clean up' the
> video), or its references in this app-global JSM might leak the window in
> question...

The reference is attached to the menupopup element which is in the same window, so unless there are implicit references (eg. due to the function being created from a different compartment), I don't think this would leak... I changed it anyway, as it would be easy in the future to add references without noticing the effect.

> @@ +533,5 @@
> > +
> > +          let constraints = { video: { mediaSource: type, deviceId: {exact: deviceId } } };
> > +          let chromeWin = chromeDoc.defaultView;
> > +          chromeWin.navigator.mediaDevices.getUserMedia(constraints).then(stream => {
> > +            video.src = chromeWin.URL.createObjectURL(stream);
>
> This races - the event listener could have been called again by now, for a
> different item.

I thought about this at the time and decided it wasn't worth complicating the code to address this unlikely edge case, as getUserMedia would return results in the same order as the calls and all the race would cause is a stream starting for a short amount of time before it gets replaced by another one. But thinking about this some more, there was another race that worried me more: if the panel is closed before getUserMedia completed, the cleanup code doesn't run and we leave an active stream until the browser window is closed (or the panel used again). The new version of the patch fixes both issues at once.

> ::: browser/themes/shared/notification-icons.inc.css
> @@ +154,5 @@
> >  .screen-icon.blocked-permission-icon {
> >    list-style-image: url(chrome://browser/skin/notification-icons.svg#screen-blocked);
> >  }
> >
> > +html|*#webRTC-previewVideo {
>
> Do you need the html| for this to work? I would have expected just the ID
> selector to be fine, and for you to only need to namespace the tagname,
> which isn't relevant here.

Yes, I do. And like you, I also initially thought I only needed to specify the namespace for tag names.

> @@ +158,5 @@
> > +html|*#webRTC-previewVideo {
> > +  width: 300px;
> > +  /* If we don't set the min-width, width is ignored. */
> > +  min-width: 300px;
> > +  max-height: 200px;
>
> Will this warp if used with a screen/window that is higher than it is wide
> ("portrait"), as you're forcing the width to be big and the height to be
> small?

The aspect ratio will be preserved, and empty areas will appear on the sides. I worked on this part of the CSS some more for bug 1284878 to ensure there's always enough space to display the warning.
Attachment #8806832 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

6 months ago
Attachment #8805070 - Attachment is obsolete: true
Attachment #8806832 - Flags: review?(gijskruitbosch+bugs) → review+
Thanks for the thorough explanations!
(Assignee)

Comment 9

6 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5c8208b0139c97d9470ea64251884acfcc6ac82
Bug 1284877 - show a preview when the user selects a screen/window/application to share, r=Gijs.
Status: NEW → ASSIGNED

Comment 10

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f5c8208b0139
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

Updated

6 months ago
Depends on: 1316465
Duplicate of this bug: 1329496
Depends on: 1336066
Depends on: 1336067
Verified fixed FX 52b2 Win 7, Ubuntu 16.04, OS X 10.11.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.