204.42 KB, image/png
558.16 KB, image/png
1.47 KB, patch
|Details | Diff | Splinter Review|
118.83 KB, image/png
640.67 KB, image/png
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.
I believe this is all about aspect ratios, e.g. on my setup, the white space is less than that (though still there).
Summary: Unnecessary white space in conversation window → [UX] Unnecessary white space in conversation window
Flags: qe-verify- → qe-verify?
Summary: [UX] Unnecessary white space in conversation window → Unnecessary white space in conversation window
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.
"Big" size. As you see I just changed the width like 30px and the video changed so much.
I can work on it but I need UX help with comment 2.
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?
Assignee: nobody → tkolodziejski
Status: NEW → ASSIGNED
Hi Tomasz, can you provide a point estimate and QE verification.
Iteration: --- → 36.1
Points: --- → 2
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.
Attachment #8509816 - Flags: review?(mdeboer)
The white-space is gone. The video covers the entire space available. I can also move the top of the video below the toolbar.
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.)
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.
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.
So what do we do with white-space and cover/fit problem :darrin?
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.
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.
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 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.
Attachment #8509816 - Flags: review?(mdeboer) → review+
Marking as checkin-needed. Aspect-ratio issues should be addressed by bug 1046789.
Whiteboard: [UX] → [UX][fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [UX][fixed-in-fx-team] → [UX]
Target Milestone: --- → mozilla36
Pauly -- Can you verify this in Nightly? Thanks!
(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.
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 on attachment 8509816 [details] [diff] [review] loop-whitespace.patch Approval Request Comment Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8509816 - Flags: approval-mozilla-aurora?
Attachment #8509816 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: VERIFIED → RESOLVED
Closed: 5 years ago → 3 years ago
Resolution: FIXED → WONTFIX
You need to log in before you can comment on or make changes to this bug.