1077610 - Unnecessary white space in conversation window

Status: RESOLVED FIXED

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

Description

[:shell escalante]

from the UX review - https://bug1065441.bugzilla.mozilla.org/attachment.cgi?id=8497452

#12 & #15 shows the area - Darrin asked for removing the white space or if that is too long a fix, making it black to match the rest of the window.  no need to move the bar (which is mentioned in the visual #15).  Ideally we dynamically size in future versions.

Comment 1

[Mark Banner (:standard8)]

I believe this is all about aspect ratios, e.g. on my setup, the white space is less than that (though still there).

Comment 2

[Tomasz Kołodziejski [:tomasz]]

I think this is pretty important bug in general. There are two modes that are natural and it would be nice to have them both:

1. Similar background-size: cover.
2. Video fit (like "Page Fit" in pdf.js).

This is important because calling from horizontal device like desktop to (naturally) vertical device like mobile makes the cover impractical. Other scenario is calling from a wide-ratio device like, say 2:1 to 4:3. Then I want 2. as well. This is also the case when I have a phone (also close to 2:1 I guess) and I'm called with a 4:3 video.

When the ratio is similar cover works great.

I'm also seeing a bug with with the sudden change of heights. Screenshots coming.

Comment 3

[Tomasz Kołodziejski [:tomasz]]

Attachment: small.png

Small size.

Comment 4

[Tomasz Kołodziejski [:tomasz]]

Attachment: big.png

"Big" size. As you see I just changed the width like 30px and the video changed so much.

Comment 5

[Tomasz Kołodziejski [:tomasz]]

I can work on it but I need UX help with comment 2.

Comment 6

[Darrin Henein [:darrin]]

Is it possible to have both, and make the right decision based on the video we receive? So use cover or fit (from comment 2) when it makes most sense?

Comment 7

[Marco Mucci [:MarcoM]]

Hi Tomasz, can you provide a point estimate and QE verification.

Comment 8

[Tomasz Kołodziejski [:tomasz]]

Attachment: loop-whitespace.patch

So this change basically sets the height of various element to 100% always and not only if the media size is something. It makes much more sense like that. Especially it eliminates the resize flicker when resizing (see attachment 8506400 [details] and attachment  8506401 [details]).

Right now the video is always using "background: cover" style to fill the space reserved for it. I think we'll need the UI to switch it sooner or later to fit the screen as described in comment 2. But this patch does not try to fix it.

Screenshots coming.

Comment 9

[Tomasz Kołodziejski [:tomasz]]

Attachment: after-panel.png

The white-space is gone. The video covers the entire space available. I can also move the top of the video below the toolbar.

Comment 10

[Tomasz Kołodziejski [:tomasz]]

Attachment: after-detached.png

This is the view after detaching the panel to become window.

(Side note:
The strange thing for me is that we have different rules for standalone player and this one. Especially, the standalone player when resized to have small width stacks the videos one below the other. This behavior is not present in the current desktop client.)

Comment 11

[Mark Banner (:standard8)]

Assuming your cameras were in the same setup for after-panel and after-detached, then it looks like the in-browser panel version is missing some of the width of the view.

Hence, what's the priority - to display all of the video feed, or to display a segment of it as big as we can - but where the caller might not realise the callee can't see everything?

Additionally, have you tested with different aspect-ratio cameras, e.g. the camera when using a flame device?

This feels like it needs ux-review before landing.

Comment 12

[Tomasz Kołodziejski [:tomasz]]

Yes, it is how it works right and I didn't change that behavior -- just try resizing the detached window. And that's the reason I described it in comment 2 -- it needs some more discussion. Ideally, there should be some UI to switch between the cover and fit mode.

Comment 13

[Tomasz Kołodziejski [:tomasz]]

So what do we do with white-space and cover/fit problem :darrin?

Comment 14

[Tomasz Kołodziejski [:tomasz]]

So I tested it with my phone and it always behaves as cover. I feel that the aspect-ratio should be a follow-up bug as there are other issues with it like video playback should always show the exact video that is played to the other party. That's not always the case since when I resize the window to have strange aspect ratio the playback window gets strange aspect ratio as well.

Comment 15

[Darrin Henein [:darrin]]

We've already considered aspect ratios in https://bugzilla.mozilla.org/show_bug.cgi?id=1008935, can you take a look and see if that answers your questions? If not, we can discuss here.

Comment 16

[Tomasz Kołodziejski [:tomasz]]

So I read the discussion and the conclusion is a bug 1046789. So this bug is not about fixing aspect-ratio. I'll just wait for the code-review in this case because this code should still be submitted as now we're just not filling the entire chatbox which looks bad (the video feedback is in the wrong place). So this change is necessary. And with mentioned 1046789 bug the concern that we're not possibly showing the entire video will be fixed.

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8509816 [details] [diff] [review]
loop-whitespace.patch

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

Thanks Tomasz! The screenshots were also very helpful, thanks.

Comment 18

[Tomasz Kołodziejski [:tomasz]]

Marking as checkin-needed. Aspect-ratio issues should be addressed by bug 1046789.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/8c85981bfad4

Comment 21

[Maire Reavy [:mreavy]]

Pauly -- Can you verify this in Nightly?  Thanks!

Comment 22

[u279076]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #21)
> Pauly -- Can you verify this in Nightly?  Thanks!

Paul, when testing this be sure to test multiple platforms with some varying screen resolutions and DPI modes.

Comment 23

[Paul Silaghi, QA [:pauly]]

Tested 36.0a1 (2014-11-06) on Win 7, OS X 10.9.5, Ubuntu 13.04 with several resolutions/dpis.
Beside the aspect-ratio issue (now the people's faces are a bit elongated), everything looks good.

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6d62a7439d3f

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8509816 [details] [diff] [review]
loop-whitespace.patch

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only

Comment 26

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 35b3 Win 7

Comment 27

[:shell escalante]

Please ignore.  This is a test on 2 bugs that i'm hoping only I'll notice (since they are so old) to see if this type of multi-change works.  will change status back to resolved fix just to keep clean data.