1131581 - Show a dropdown menu when the screen share button is clicked in the conversation window

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mike de Boer [:mikedeboer]]

The expected end result is that the 'click' action of the screen share button will show a dropdown with one clickable option: 'Share other Windows'.
This option will take the old click functionality of the screen share button.

The dropdown is not expected to visually extend outside the conversation window. We can reuse the existing DropdownMixin.

Comment 1

[:shell escalante]

triaging to blocking-loop:Fx38+ for tracking this release, all bugs in 38+ and 38? now, will move to "backlog+" for Fx39.  After that - we won't triage to a specific release any longer.  we will work from an ordered backlog.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: move screensharing options into a dropdown anchored to the screenshare toolbar button

This was harder than I expected. I had to jump through a few hoops to make the dropdown show up as it does now.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: move screensharing options into a dropdown anchored to the screenshare toolbar button

s/dropdown/menu/

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8563449 [details] [diff] [review]
Patch v1.1: move screensharing options into a dropdown anchored to the screenshare toolbar button

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

r- due to the standalone issue. Otherwise it looks good.

I'm assuming we might want to improve the style of the dropdown menu in a follow-up - possibly bug 1131584.

::: browser/components/loop/content/shared/css/conversation.css
@@ +7,5 @@
>    position: relative;
>  }
>  
>  .conversation-toolbar {
> +  z-index: 1020; /* required to have it superimposed to the video element */

What's the reason for this change? It breaks the standalone UI, as we have a z-index of somewhere around 1000 for the local stream.

::: browser/components/loop/content/shared/js/views.jsx
@@ +130,3 @@
>          "disabled": this.props.state === SCREEN_SHARE_STATES.PENDING
>        });
> +      var dropdownMenuClasses = React.addons.classSet({

nit: use cx.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8563449 [details] [diff] [review]
Patch v1.1: move screensharing options into a dropdown anchored to the screenshare toolbar button

As discussed over video this is r=Standard8 with:

- the z-index change to 1020 limited to desktop only (its needed to ensure the room-invitation-overlay is behind the menu)
- the previously mentioned nit fixed

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Mark Banner (:standard8) from comment #5)
> As discussed over video this is r=Standard8 with:
> 
> - the z-index change to 1020 limited to desktop only (its needed to ensure
> the room-invitation-overlay is behind the menu)
> - the previously mentioned nit fixed

Done!

Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/2737d0c34922

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2737d0c34922

Comment 8

[Francesco Lodolo [:flod]]

> share_windows_button_title=Share other Windows
Just to confirm: is the case correct? Based on string with similar IDs, I'd expect sentence case.

http://hg.mozilla.org/mozilla-central/diff/2737d0c34922/browser/locales/en-US/chrome/browser/loop/loop.properties

Comment 9

[Bogdan Maris, Desktop QA]

Verified that the dropdown menu was implemented and works as expected across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit) on latest Nightly and latest Aurora.