1180179 - Share the standalone media layout with desktop

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

When I reworked the standalone layout for text chat, I created a new "media-layout" div with pretty much its own css and was intended to be shared with desktop.

With the ux-refresh coming up, we should try and make that sharing happen, as it should simplify the refresh work.

Comment 1

[Mark Banner (:standard8)]

Attachment: Part 1. Adjust standalone layout ready for a central media layout component.

First step - Defines a new standalone specific class to adjust the media layout height.

It also moves the conversation toolbar out of the media-layout div. It doesn't need to be in there, and its easier if we don't include it as part of the layout.

This can land as-is (subject to review), hence getting the review process started.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8630438 [details] [diff] [review]
Part 1. Adjust standalone layout ready for a central media layout component.

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

r=dmose, with any appropriate changes.

::: browser/components/loop/content/shared/css/conversation.css
@@ +1094,5 @@
> +}
> +
> +.standalone-room-wrapper > .media-layout {
> +  /* 50px is the header, 64px for toolbar, 3em is the footer. */
> +  height: calc(100% - 50px - 64px - 3em);

I'm a little surprised that all this calc() stuff is necessary, since we have (somewhat?) switched to flexbox.

If we could avoid doing it, then maybe we could avoid cascading here entirely.

@@ +1191,5 @@
> +  }
> +
> +  .standalone-room-wrapper > .media-layout {
> +    /* 50px is height of header, 38px is height of toolbar, 25px is height of footer. */
> +    height: calc(100% - 50px - 38px - 25px);

Given that this is a duplicate of the stuff in the other media query, should we consider hoisting it out of the media queries altogether?  Or is that just going to complicate things elsewhere...

Comment 3

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #2)
> > +.standalone-room-wrapper > .media-layout {
> > +  /* 50px is the header, 64px for toolbar, 3em is the footer. */
> > +  height: calc(100% - 50px - 64px - 3em);
> 
> I'm a little surprised that all this calc() stuff is necessary, since we
> have (somewhat?) switched to flexbox.

We only switched the media layout view to flexbox.

> If we could avoid doing it, then maybe we could avoid cascading here
> entirely.

The UX-refresh is changing the standalone so that there's no header/footer as we have now, and the toolbar will also be "in the middle". Hence we won't have any need for this after the UX-refresh, so I don't think its worth changing now.

> > +  .standalone-room-wrapper > .media-layout {
> > +    /* 50px is height of header, 38px is height of toolbar, 25px is height of footer. */
> > +    height: calc(100% - 50px - 38px - 25px);
>
> Given that this is a duplicate of the stuff in the other media query, should
> we consider hoisting it out of the media queries altogether?  Or is that
> just going to complicate things elsewhere...

The values are different, this equation doesn't appear anywhere else in the file.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/787935a30ed8

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/787935a30ed8

Comment 6

[Mark Banner (:standard8)]

Attachment: Part 2. Create a new shared media layout component.

This second part moves the main media-layout div construct from the standalone rooms view into a component that lives in the shared views.jsx. The next step will be to actually use it in some of the desktop views.

Comment 7

[Mark Banner (:standard8)]

Attachment: WIP Part 3. Use the shared media layout component in direct calls.

WIP part 3. Comments coming.

Comment 8

[Mark Banner (:standard8)]

The aims of the part 3 patch are to:

- Replace the media layout of direct calls with that from the new shared component in part 2.
- As a side effect, enable text chat and adjust the layout for the conversation window so that displaying various parts of text chat would be done in a similar way to desktop, e.g. just the input box if there's no entries, and then the entries view when we do get values.

The reason for allowing the text chat here, is that it was a) easy (just include the views), b) it'd give us a better stepping stone into rooms.

The reason I moved to direct call rather than rooms was due to the invitation and context edit overlays adding slightly more complexity to the layout and were still being changed, so direct call was a smaller step up.

Intentions in the patches:

- As far as possible, stop relying on .fx-embedded and the standalone equivalent. The conversation window is just a very narrow version of the standalone (< 300px).
- There's a few css changes in the patch to keep link-generator rooms on the old css.

What's left to do on the patch:

- Basically, fix the direct call layout for text chat.
-- At the moment it just about does the right thing when opening the window - i.e. displays the text input box only.
-- If you pop-out the window, and then make it narrow as possible, extend the height and then shrink it again, the text chat is lost and the internals don't resize for some reason.
-- The intermediate width (> 300px, < 640px) view is currently broken.

Extra details:

- The flex model for < 640px width is based on rows. It overflows intentionally onto the next row. Although for desktop this isn't currently necessary, when we add screen sharing it'll make it easier - see attachment 8569948 [details], also see what standalone does at < 640px.
-- It does though make the height calculations more difficult. I've been starting to think that somewhere near the .media-wrapper class we also need a flag for if there's entries in text chat or not - however, that's hard to get at as MediaLayoutView doesn't have that data.

- If we really can't make the flex model work, then the only way I could see to change it would be to re-position the elements in the DOM tree via react, but I think we want to avoid that if we can.

Other notes:

- MediaLayoutView takes lots of parameters. I think we could improve on that, but later.
- We're probably going to end up with some similar functions across different parent views, but again, we can tidy that later (I think we might end up with an additional RoomLayoutView shared across standalone & desktop).
- UI-showcase doesn't support text chat very well (Dan's filed a bug on text chat stores in the showcase somewhere, but I can't find it atm).

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8632014 [details] [diff] [review]
Part 2. Create a new shared media layout component.

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

This looks good to me, but I'm wondering what the value could be for roomViews? This is the view that's used most by the desktop code, so unifying between standalone & desktop makes the most sense to me.
The fact that direct calls will benefit here is of course fantastic!

Also, the newly introduced view could use a fresh unit test ;-) If I overlooked something, please tell me, but I think you forgot about it....

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8632014 [details] [diff] [review]
Part 2. Create a new shared media layout component.

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

::: browser/components/loop/content/shared/js/views.jsx
@@ +950,5 @@
> +      displayScreenShare: React.PropTypes.bool.isRequired,
> +      isLocalLoading: React.PropTypes.bool.isRequired,
> +      isRemoteLoading: React.PropTypes.bool.isRequired,
> +      isScreenShareLoading: React.PropTypes.bool.isRequired,
> +      // The poster URLs are for UI-showcase testing and development

nit: missing dot.

@@ +979,5 @@
> +        "media-wrapper": true,
> +        "receiving-screen-share": this.props.displayScreenShare,
> +        "showing-local-streams": this.props.localSrcVideoObject ||
> +          this.props.localPosterUrl,
> +        "rendering-remote-video": this.props.renderRemoteVideo && (

nit: please end with the operand and put the brace on the newline.

@@ +987,5 @@
> +      return (
> +        <div className="media-layout">
> +          <div className={mediaWrapperClasses}>
> +            <span className="self-view-hidden-message">
> +              {"self_view_hidden_message"}

no l10n?

Comment 11

[Mark Banner (:standard8)]

Attachment: Part 2. Create a new shared media layout component. v2

Updated patch and with tests.

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8637202 [details] [diff] [review]
Part 2. Create a new shared media layout component. v2

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

I say: nothing short of awesome!

Comment 13

[Pulsebot]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa418b120a3b

Comment 15

[Mark Banner (:standard8)]

Attachment: Part 3. Use the shared media layout component in direct calls.

This does enough to get direct calls working.

In the end, to get the local video positioned correctly I decided to change the DOM tree. There was something weird with getting the sizes of the elements in the ui-showcase, its possible something I was doing was wrong, but I think this ends up cleaner anyway, as we're not having to pipe styles through js.

In the css, I've done some extra limiting so as not to affect desktop room conversations.

There's also a bit of matchMedia hacking - the showcase/react doesn't set things up correctly currently, so I've had to take Dan's hack from a while ago and use it here as well.

Comment 16

[Mark Banner (:standard8)]

Attachment: Part 3.1 - Fix ui-showcase screen share standalone room views

This fixes the ui-showcase views relating to the screen share on standalone. The rework in the part 3 patch exposed some places which weren't being handled correctly (like specifying a screen share url when it was meant to be loading), and some other mistakes.

This fixes those - for landing, I'll roll it into part 3.

Comment 17

[Mark Banner (:standard8)]

Attachment: Part 4. Use the shared media layout component in Loop's room views.

Most of this is about sharing the view, and adjusting the final layout parts to work with the invitation overaly.

I couldn't figure why desktop-local's warning count had gone up by one as none of them come from areas I've touched. I've upped it for now, we should nuke this completely at some stage.

I've also tidied up some of the styles I knew we definitely no longer need, and took out some old things we also don't need.

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8637930 [details] [diff] [review]
Part 3. Use the shared media layout component in direct calls.

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

LGTM! I'll land this.

::: browser/components/loop/content/shared/css/conversation.css
@@ +1509,4 @@
>    display: none;
>  }
>  
> +

nit: superfluous newline.

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +205,5 @@
>  active_screenshare_button_title=Stop sharing
>  inactive_screenshare_button_title=Share your screen
>  share_tabs_button_title2=Share your Tabs
>  share_windows_button_title=Share other Windows
> +self_view_hidden_message=Self-view hidden but still being sent; resize window to show

Oooh, well done, we got this on desktop too now indeed!

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8638495 [details] [diff] [review]
Part 3.1 - Fix ui-showcase screen share standalone room views

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

rs=me.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/245aec9f19a2
https://hg.mozilla.org/integration/fx-team/rev/0ccdbc708362

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8638505 [details] [diff] [review]
Part 4. Use the shared media layout component in Loop's room views.

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

Ship it!

::: browser/components/loop/test/desktop-local/index.html
@@ +94,5 @@
>      });
>  
>      describe("Unexpected Warnings Check", function() {
>        it("should long only the warnings we expect", function() {
> +        chai.expect(caughtWarnings.length).to.eql(28);

Nownow, didn't want to deal with this here, huh? ;-)

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4791b25ea75b

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/245aec9f19a2
https://hg.mozilla.org/mozilla-central/rev/0ccdbc708362
https://hg.mozilla.org/mozilla-central/rev/4791b25ea75b

Comment 24

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #21)
> ::: browser/components/loop/test/desktop-local/index.html
> @@ +94,5 @@
> >      });
> >  
> >      describe("Unexpected Warnings Check", function() {
> >        it("should long only the warnings we expect", function() {
> > +        chai.expect(caughtWarnings.length).to.eql(28);
> 
> Nownow, didn't want to deal with this here, huh? ;-)

See comment 17 - I couldn't find the direct reason for the increase.

Comment 25

[Stefan Plewako [:stef]]

https://hg.mozilla.org/mozilla-central/diff/245aec9f19a2/browser/locales/en-US/chrome/browser/loop/loop.properties
> +self_view_hidden_message=Self-view hidden but still being sent; resize window to show

What the heck is self-view and what it means that it is still being sent?

Comment 26

[Mark Banner (:standard8)]

(In reply to Stefan Plewako [:stef] from comment #25)
> https://hg.mozilla.org/mozilla-central/diff/245aec9f19a2/browser/locales/en-
> US/chrome/browser/loop/loop.properties
> > +self_view_hidden_message=Self-view hidden but still being sent; resize window to show
> 
> What the heck is self-view and what it means that it is still being sent?

Self view is the view of the local video. The warning is present because someone may think that the video is no longer being transmitted if it isn't  on the display - hence a possible privacy issue.