1020445 - Google Chrome link clicker UI layout is broken

Status: RESOLVED FIXED

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

Description

[Romain Testard [:RT]]

Attachment: Chrome UI.png

Testing environment:
Firefox latest build on Elm on Windows 8.0 on link generator side
Google Chrome 35.0.1916.114 m on Windows 7

The link clicker UI is broken on Google chrome, attaching screenshot.

Comment 2

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

This seems to be a regression of bug 991122 after we upgraded the sdk to v2.2.5.

Comment 3

[Romain Testard [:RT]]

Is there any update on this please? Any chance to have it fixed soon or is this blocked by TokBox?

Comment 4

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

I'm on other fronts right now (the react port), but I'll switch back to investigating this tomorrow. Last time I tried debugging this I went nearly mad inspecting & debugging all the DOM generated by the sdk… so if anybody wants to take that bug, feel free to :)

Comment 5

[Maire Reavy [:mreavy]]

We just got a new version of the TB SDK (v2.2.6): see Bug 1032469.  I think we should update the SDK and then see if we can fix this bug ourselves or if we need to reach out to TB.  We'll definitely pull this bug for the next sprint (which starts next week).

Comment 6

[Mark Banner (:standard8)]

With the 2.2.6 upgrade, it appears this has changed slightly again wrt css on chrome. I couldn't see an easy way to fix it, though I'm not that experienced with fancy css.

Comment 7

[Mark Banner (:standard8)]

This comment may help: https://bugzilla.mozilla.org/show_bug.cgi?id=1008935#c7

Comment 8

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

So after trying to use the suggested approach — which didn't quite work — two things are puzzling:

- Whatever configuration we use, custom styles are applied inline to the wrapper element generated by the SDK, which makes styling inner child elements hard;
- I don't see any obvious reason for the layout to break specifically on Google Chrome ; styles should have the very same effects on both platforms (in a perfect world).

I don't want to spend too much time investigating what exactly produces the break on Chrome, because DOM/CSS stuff generated by the SDK is a bit out of my control and my understanding of it. I could spend more time trying to find a way to override things generated by the SDK, but this strategy is unlikely to scale over time from a maintenance perspective.

Really what would be great is to have an option to get the raw video elements with no styling applied, but I don't know if that's possible.

Comment 9

[mozilla-support]

- Whatever configuration we use, custom styles are applied inline to the wrapper element generated by the SDK, which makes styling inner child elements hard;
If you set insertMode: append as a property when creating publisher and subscriber and keep CSS to your own elements and not style SDK generated elements, does this still happen?

- I don't see any obvious reason for the layout to break specifically on Google Chrome ; styles should have the very same effects on both platforms (in a perfect world).
I agree. I can inspect on some of the elements in Chrome and report any anomalies I find.

- Really what would be great is to have an option to get the raw video elements with no styling applied, but I don't know if that's possible.
This is not possible, unfortunately

Comment 10

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: tb-inline-computed-styles-chrome-firefox.png

(In reply to mozilla-support from comment #9)
> If you set insertMode: append as a property when creating publisher and
> subscriber and keep CSS to your own elements and not style SDK generated
> elements, does this still happen?

Yes. Basically even when using {insertMode: "append", width: "100%", height: "100%"}, the SDK wraps the inserted video element in some inline styled element (which class name is "OT_video-container"). 

The values seems to be computed in the fixAspectRatio function here:
https://github.com/mozilla/gecko-dev/blob/e69d9785528ce1406e535c39cafa8a9424647ad1/browser/components/loop/content/libs/sdk.js#L3188-L3242

Attached screenshot shows the different result with these two generated values for inline styles in both Chrome and Firefox.

Please note that the height is computed as 218,75%; if I change this to something like 105px, it fixes the situation in Chrome and keeps working on Firefox. The problem is I have no way to override this height value, as it's applied inline. And even if I can override it (eg. using js or very very smart css selectors), this is definitely not likely to scale in the future, so I won't go that path.

> - Really what would be great is to have an option to get the raw video
> elements with no styling applied, but I don't know if that's possible.
> This is not possible, unfortunately

Technically impossible, or politically? Any reasoning behind this impossibility? Really, that would help a lot developers wanting to keep full control on their design.

Comment 11

[Maire Reavy [:mreavy]]

The layout in Google Chrome was fine (and working) until we landed Bug 1017206, which upgraded the SDM from v2.2 to v2.2.5.

Comment 12

[mozilla-support]

Thanks for the additional input. I've created some sample code, hopefully this would resolve some issues. If not, we can go from here: https://gist.github.com/songz/c40c78959543478170e2

After connecting to a session and subscribing to a stream, the dom structure for a subscriber looks like this:
-> div.mySubscriber (my div element dynamically created to contain stream)
  -> div.OT_subscriber.OT_root ... (Created by SDK)
    -> div.OT_video-container ... (Created by SDK, has inline styling)
      -> video 

If I style div.mySubscriber (to a specific width, height, position, display inline-block), the video stream displays in a way I expected it to. Can you try this approach (creating a div dynamically for each stream and styling the dynamically created stream)? 
If you use this approach, do remember to delete the div for streamDestroyed events.

If this suggestion does not work for you, can you tell me what you are doing and how I can replicate it to break the sample code to get an isolated case (helps with debugging and faster fix).

We don't have option to get give raw video at this point because that option was never built into the SDK and the container divs gives us the ability to add voice/mic controls (which is useful to most of our developers). We definitely want developers to have full control of their design and if none of our suggestions work then its a bug on our end and we'll fix it.

Comment 13

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

So after a long debugging session with Songz from TokBox, it appears there might be a bug in the SDK when it comes to computing the height of elements in order to size the video element with the right aspect ratio. Eg. in our case, using `max-height` instead of `height` makes that computation fails.

Songz will report this issue internally and will think of a possible solution.

Comment 14

[mozilla-support]

Apparently its not a bug with the SDK. Height always defaults to 198 if it is 0 or not defined. If the container element does not have height defined, the SDK will create a video with height 198px. Documentation: http://tokbox.com/opentok/libraries/client/js/reference/Session.html#subscribe

However, I have found a way to make the width/height always 4:3 ratio. Here's the stack overflow explaining how it works: http://stackoverflow.com/questions/1495407/css-a-way-to-maintain-aspect-ratio-when-resizing-a-div

Once you have the remote video container created in the right aspect ratio, calling session.subscribe should cause the stream to fill the container completely. Building from the demo you gave me earlier, here's a sample where the subscriber's video is always 60% of the width of the page and height automagically adjusts: http://jsbin.com/tawom/10/edit

gist: https://gist.github.com/songz/c7fbd10cb6389b7c2cab

Hope that helps

Comment 15

[mozilla-support]

Adding on top of the previous suggestions, to make width/height ratio dynamic to fit the stream instead of a fixed 4:3 ratio, you can do something like this:

    function updateStreamHeight(stream){
      if($(".remote_wrapper").data("streamId") !== stream.streamId) return;
      var dim = stream.videoDimensions;
      var height = (60*dim.height)/dim.width;
      console.log("resizing height to: "+height);
      $(".remote_wrapper").css("padding-bottom", height+"%");
    }

Call updateStreamHeight for streamCreated and streamPropertyChanged events, for example:

    session.on({
      "streamCreated": function(event){
        session.subscribe(event.stream, document.querySelector(".remote"), property);      
        $(".remote_wrapper").data("streamId", event.stream.streamId);
        updateStreamHeight(event.stream);
      },
      "streamPropertyChanged": function(event){
        updateStreamHeight(event.stream)
      }
    }); 

Demo: http://jsbin.com/tawom/17/edit

Relevant documentation: 
StreamProperty Changed Event: 
  http://tokbox.com/opentok/libraries/client/js/reference/StreamPropertyChangedEvent.html
Stream's video Dimensions property:
  http://tokbox.com/opentok/libraries/client/js/reference/Stream.html#

Comment 16

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1020445-Fixed-Loop-conversation-layout-on-Google.patch

This patch fixed the layout broken conversation layout on Google Chrome. It assumes video streams will always be available using a 4:3 ratio (according to TokBox, see previous comment from them) — which is something we'll want to fix later.

For now, this makes the standalone webapp useable by Google Chrome users, which is a great improvement.

Comment 17

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(Too much joy made me make lots of typos. Oh well.)

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Did the previous layout assume 4:3 aspect ratio as well, or is that a regression?

Comment 19

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #18)
> Did the previous layout assume 4:3 aspect ratio as well, or is that a
> regression?

I never had access to any webcam with aspect ratio being different than 4:3, so I don't know :/

stream.videoDimensions, as highlighted in a previous comment from TokBox, will presumably help fixing any pb with different aspect ratios in a next iteration (but I'd really want current patch to land asap).

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8459736 [details] [diff] [review]
0001-Bug-1020445-Fixed-Loop-conversation-layout-on-Google.patch

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

Sounds good; I'll make the comment changes and land.  r=dmose.

::: browser/components/loop/content/shared/css/conversation.css
@@ +125,5 @@
>  }
>  
>  .conversation .media.nested .remote {
>    display: inline-block;
> +  position: absolute;

I'm going to add a comment here...

@@ +141,5 @@
>    right: 0;
>    width: 30%;
>    max-width: 140px;
> +  height: 22.5%;
> +  max-height: 105px;

And here.

::: browser/components/loop/content/shared/js/views.jsx
@@ +207,2 @@
>        width: "100%",
> +      height: "100%",

I'll update the comment before landing.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85a8356b418b

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Patch landed, with comment and jshint ignore changes.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/85a8356b418b

Comment 24

[Romain Testard [:RT]]

Attachment: Call set-up (before the other party answers)

I am using Chrome 36.0.1985.125 m on Windows 7 and the layout is still broken for me.

Comment 25

[Romain Testard [:RT]]

Attachment: In call UI

Comment 26

[Maire Reavy [:mreavy]]

The standalone app on the Loop server hasn't been updated yet.  The tentative plan for now is update it every 2 weeks (after each sprint) and move toward updating it more frequently. I believe the next server update of the standalone app will be early next week. 

My end goal is to get the app updated automatically every Night.

Comment 27

[u279076]

This looks fine on Mac but on Windows 8 I'm hitting an issue.

1. In Firefox 34, I click the "invite someone to talk" button to generate a URL then click copy
2. In Chrome 36, I paste the generated URL, press ENTER, then click "start the call"
3. In Firefox I click "answer"
4. In Chrome the video stream appears black for a few seconds then times out

Result: 
Chrome shows an error, "Your call did not go through"
Firefox shows a message, "Your peer ended the conversation"

Should I report this separately?

Comment 28

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #27)
> Result: 
> Chrome shows an error, "Your call did not go through"

Does it work using FF?

Comment 29

[u279076]

(In reply to Nicolas Perriault (:NiKo`) from comment #28)
> (In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #27)
> > Result: 
> > Chrome shows an error, "Your call did not go through"
> 
> Does it work using FF?

Yeah, it works in Firefox but not with Chrome.

Comment 30

[Paul Silaghi, QA [:pauly]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #27)
> Result: 
> Chrome shows an error, "Your call did not go through"
> Firefox shows a message, "Your peer ended the conversation"
Chrome 37 vs Nightly 35.0a1 (2014-09-09), Win 7 x64 - same behavior except no error message shows up in Chrome