Closed Bug 1008935 Opened 10 years ago Closed 10 years ago

Loop desktop UI needs to handle different aspect ratios better

Categories

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

defect
Points:
3

Tracking

(firefox34 fixed)

RESOLVED FIXED
mozilla34
Iteration:
34.1
Tracking Status
firefox34 --- fixed

People

(Reporter: standard8, Assigned: sevaan)

References

Details

(Whiteboard: [p=1, investigation])

Attachments

(5 files)

Using my Android phone (Galaxy S4 mini) as a link clicker, when I view the video on the desktop, the video is taller than the conversation window, and requires scrolling to see the local video.

This also applies when the conversation window is expanded out into a separate window.
Assignee: nobody → nperriault
Priority: -- → P2
Whiteboard: [p=?, s=UI32]
Target Milestone: --- → mozilla32
This is using Firefox for Android, I assume?
Firefox for Android was the link clicker end. No idea if Chrome works there yet or not, but I suspect the aspect ratio issue would be the same.
Priority: P2 → P1
Whiteboard: [p=?, s=UI32] → [p=?]
Target Milestone: mozilla32 → mozilla34
Assignee: nperriault → sfranks
Status: NEW → ASSIGNED
Whiteboard: [p=?] → p=3 s=33.3 [qa-]
Attached image Portrait (Scrolling)
Attached image illustrates the problem.
Attachment: When the phone is rotated to landscape, scrolling is no longer an issue.
I haven't looked into this particular case, but it's at least conceivable that fixing <https://bugzilla.mozilla.org/show_bug.cgi?id=624647> would make fixing this easier.
I'm actually suspecting that some of this could be due to the way the TokBox sdk is handling this, or our interactions with it.
One suggestion is that you can create a container element for the subscribed stream, styled and positioned the way you want it to. Then when you call session.subscribe(...,..., properties), you can specify:
insertMode: "append" => append the stream element to the container you have created
width: "100%" => the stream element will fill up container's width
height: "100%" => the stream element will fill up container's height

see if that helps?
Thanks for jumping in, Tokbox. Can an engineer confirm whether this helps?
Attached image tb_inline-styles.png
(In reply to mozilla-support from comment #7)
>
> see if that helps?

Thanks. Well, even with insertMode set to "append", inline styles are style applied to the DOM wrappers generated by the SDK — see attached capture — which makes styling any parent container element rather hard, to say the least.

Is there a way to prevent these styles to be applied inline? to be applied at all?
Flags: needinfo?(mozilla-support)
The best practice is to not add any styling to SDK generated elements. Best practice is to set insertMode to append in the JS and the SDK will append a generated element to your own container element (which the SDK will not touch). When you create the subscriber in the JS and pass in width:100% and height:100%, the SDK generated elements should fill your container 100%. If you keep css styling to your own elements, the inline styling applied to SDK generated elements should not be causing problems.
Flags: needinfo?(mozilla-support)
Whiteboard: p=3 s=33.3 [qa-] → p=3 s=34.1 [qa-]
Can someone clarify whether this is a UX/Design issue, or whether there is an engineering solution to this bug?
There is inter-related UX and engineering work here. 

There are a variety of things we could consider doing when we receive portrait video that is taller than the receiving window:

* add a vertical scrollbar (current behavior)

* crop the top and bottom of the video until the height matches that of landscape mode, using existing window size

* scale down the height of the video to match the window size.  Either accept that we will have pillarboxing, or resize the window horizontally.

* force the window to be tall enough to (within some bound) to accomodate the incoming video.  Consider making the window narrower at the same time, so that we don't need to have blank bars on the sides ("pillarboxing").

There are other variants as well, but these are the most likely.

Given the current constraints posed by the SDK, it's not entirely clear to me which ones of these are easy/hard/possible.  Perhaps :NiKo` knows.  I think that having bug 624647 implemented in Gecko is very likely to make more things easier or even possible, so I've added that as a dependency.
Depends on: 624647
(In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #12)

Thanks for putting your thoughts down, dmose.

Out of those options, my original thinking was to scaling down the height of the video to match the window size. I'm okay with the pillarboxing (learned a new word) as it just shows you're chatting with someone using their phone.

:NiKo, if you could let us know your thoughts, that would be awesome.

Thanks, all!
Flags: firefox-backlog+
For what it's worth Skype implements "scale down the height of the video to match the window size.  Either accept that we will have pillarboxing, or resize the window horizontally."

Another thing we may want to consider is that the mobile user will turn his phone and go "portrait mode" whilst on the call.

Right now on Loop when a link clicker is on FF for ANdroid, the conversation window on the browser client side displays a vertical scrolbar and adapts automatically as the mobile goes portrait mode.
Iteration: --- → 34.1
Points: --- → 3
Whiteboard: p=3 s=34.1 [qa-] → [qa-]
QA Contact: anthony.s.hughes
I think suggestions in this thread can help with this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1020445#c15

When user goes to landscape mode from portrait mode or vice versa, there will be a streamPropertyChanged event fired with new width/height and you can adjust accordingly.
Whiteboard: [qa-] → [p=1, investigation, qa-]
Hi all,

Ideally we should be resizing the link clicker's video to fit the Loop window.

If the link clicker is using their phone in portrait mode, the height of the video should match the height of the window, with the width scaled to the correct proportions.

If the link clicker is using their phone in landscape mode, the width of the video should match the width of the window, with the height scaled to the correct proportions.

Any unused space will be kept dark.

Niko, can you confirm that this is feasible?
Flags: needinfo?(nperriault)
(In reply to Sevaan Franks [:sevaan] from comment #16)
> Niko, can you confirm that this is feasible?

It *should*, though with using the SDK it might get complicated because it generates the video elements and associated styles for us, so that makes us overriding styles and/or styling these elements using Javascript, where size & aspect ratio computation is easier.

needinfo Tokbox for thoughts here.

Also a remark regarding the provided mockup: as both standalone and desktop full screen conversation window will use a fluid layout (resizable browser windows), we probably want to ensure this strategy keeps matching all possible use cases (eg. I remember we went with falling back to responsive side-by-side video display for wide fullscreen conversation window in Talkilla.)
Flags: needinfo?(nperriault) → needinfo?(mozilla-support)
First, you can detect if stream is in portrait or landscape mode with the stream videoDimensions property by comparing width/height: http://tokbox.com/opentok/libraries/client/js/reference/Stream.html

Sometimes, the user might the phone in the middle of the call. You can detect changes to/from landscape to portrait by listening to the streamPropertyChanged event and checking the stream's videoDimensions: http://tokbox.com/opentok/libraries/client/js/reference/StreamPropertyChangedEvent.html

After identifying the stream dimensions, you can style the video:

* If the link clicker is using their phone in portrait mode, the height of the video should match the height of the window, with the width scaled to the correct proportions.
    
    function updateStreamHeight(stream){
      var dim = stream.videoDimensions;
      var width = (100*dim.width)/dim.height; // retrieve height percentage: ?/100% = dim.width/dim.height
      $(".remote_wrapper").css("padding-bottom", "100%");
      $(".remote_wrapper").css("width", width+"%");
    }


* If the link clicker is using their phone in landscape mode, the width of the video should match the width of the window, with the height scaled to the correct proportions.
    
    function updateStreamHeight(stream){
      var dim = stream.videoDimensions;
      var height = (100*dim.height)/dim.width; // retrieve height percentage: 100%/? = dim.width/dim.height
      $(".remote_wrapper").css("padding-bottom", height+"%");
      $(".remote_wrapper").css("width", "100%");
    }

Sample HTML for this example:
  <div class="conversation">
    <div class="media nested">
      <div class="remote_wrapper">
        <div class="remote"></div>  
      </div>      
      <div class="local"></div>
    </div>
  </div>

Sample CSS for this example: 
.conversation .media.nested {
  position: relative;
  text-align:center;
}
 
.conversation .media.nested .remote {
  display: inline-block;
  position: absolute;
  top: 0; bottom: 0; left: 0; right: 0;
  background: #000;
}
    
.conversation .media.nested .remote_wrapper {
  display: inline-block;
  position: relative;
  background: #0f0;
  width: 60%;
  padding-bottom: 45%;
}
 
.conversation .media.nested .local {
  display: inline-block;
  position: absolute;
  bottom: 4px;
  right: 0;
  width: 30%;
  max-width: 140px;
  height: 75px;
  z-index: 2;
  background-color:#ff0;
}

gist: https://gist.github.com/songz/c7fbd10cb6389b7c2cab
^^^
Flags: needinfo?(mozilla-support)
Thanks, Song. Niko, shall we mark this bug as resolved and move work to an implementation bug?
Flags: needinfo?(nperriault)
(In reply to Sevaan Franks [:sevaan] from comment #20)
> Thanks, Song. Niko, shall we mark this bug as resolved and move work to an
> implementation bug?

Sure.
Flags: needinfo?(nperriault)
Implementation work to take place in Bug 1046789.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 1046789
Whiteboard: [p=1, investigation, qa-] → [p=1, investigation] [qa-]
Flags: qe-verify-
Whiteboard: [p=1, investigation] [qa-] → [p=1, investigation]
You need to log in before you can comment on or make changes to this bug.