Closed Bug 1200693 Opened 4 years ago Closed 4 years ago

Modify aspect ratio so that top and bottom black bars are not seen when waiting alone in the conversation window

Categories

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

defect

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Iteration:
44.2 - Oct 19
Tracking Status
firefox44 --- fixed

People

(Reporter: RT, Assigned: mancas)

References

Details

(Whiteboard: [visual refresh])

User Story

- Adjust height of minimal conversation window to eliminate the bars.
- Check the direct call views still look ok
- Check the room views still look ok
- Check the edit context view still looks ok

Attachments

(3 files, 3 obsolete files)

No description provided.
We agreed that we should modify the aspect ratio of the video self video when waiting alone in the conversation (desktop client UI) so that the top and bottom black bars are not displayed.
Blocks: 1179164
Rank: 17
Priority: -- → P1
Whiteboard: [visual refresh]
If we shorten it, then we'll need to re-check all the views, so adding to the user story.
User Story: (updated)
there is a fixed width when docked. height varies based on text and context.  should be way to minimize the bars though.
added see also bug 1205684 - as fixing one will impact the other (or should check to see what happens)
See Also: → 1205684
Assignee: nobody → dmose
Iteration: --- → 43.3 - Sep 21
moved to Manuel and Fernando - to at least work out what is wanted and it's related to bug 1205684 

reach out to Mark/Dan as they are different parts of code - just same part of UX
Assignee: dmose → b.mcb
Rank: 17 → 20
Priority: P1 → P2
Why we don't use object-fit: cover to solve this issue? AFAIK, it's the same we're doing with the remote video.
Flags: needinfo?(dmose)
(In reply to Manuel Casas Barrado [:mancas] from comment #7)
> Why we don't use object-fit: cover to solve this issue? AFAIK, it's the same
> we're doing with the remote video.

This is for privacy reasons. We don't want to display a local video stream where you can't see everything that you're sending.

For example, if you had something in the room you didn't want to show, with the cover mode, you'd think that it wasn't being captured because its not in shown in the window. However, when your friend connects, then it is suddenly shown.

There's also the case I've had where someone else comes into the room and doesn't like to be seen, so keeps off-camera and uses the local video that they can see as a guide.
Flags: needinfo?(dmose)
I see your point Mark. I have a question related to the conversation window. The minimal height of the chatbox is set in |browser.css:828| as you can see in the DOM Inspector, right? However, this file is auto-generated in the build process isn't it?
Flags: needinfo?(standard8)
Comment on attachment 8667279 [details] [diff] [review]
Fix: Top and bottom black bars are not seen anymore

Hey Dan, could you take a look at the patch when you get a chance, and give me some feedback if needed?
Attachment #8667279 - Flags: review?(dmose)
Comment on attachment 8667279 [details] [diff] [review]
Fix: Top and bottom black bars are not seen anymore

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

I'm not going to have time to look at this today, so I'm adding a couple of other potential reviewers.  If they don't get to it, I will tomorrow.
Attachment #8667279 - Flags: review?(standard8)
Attachment #8667279 - Flags: review?(mdeboer)
Attachment #8667279 - Flags: review?(mdeboer)
Attachment #8667279 - Flags: review?(dmose)
Comment on attachment 8667279 [details] [diff] [review]
Fix: Top and bottom black bars are not seen anymore

The min & max height are interfering with the custom sizes we use for adjusting the height of the room when there's context added, or when a remote user joins the room and starts doing text chat.

I would suggest just changing the height to 255px, and do the same in .chatbar-innerbox lower down.

However, with no context and no-one in the room we also need to adjust the edit context view - on Mac, we're just starting to get scrollbars, I suspect on windows/linux it might be worse.
Attachment #8667279 - Flags: review?(standard8) → review-
Attached image chatboxes.png
Hey Pau, as we talked offline, I've increase the width and height of the conversation window. Now the top and bottom black bars are not shown. Could you take a quick look at the different states of the conversation window and check that everything looks good?
Attachment #8669616 - Flags: ui-review?(b.pmm)
Comment on attachment 8669616 [details]
chatboxes.png

Awesome! Thanks, Manu.
Attachment #8669616 - Flags: ui-review?(b.pmm) → ui-review+
Attachment #8667279 - Attachment is obsolete: true
Attachment #8669628 - Flags: review?(standard8)
Comment on attachment 8669628 [details] [diff] [review]
Fix: Top and bottom black bars are not shown

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

Ok, this looks fine, but please update the two extra instances that I've commented on. r=Standard8 with those fixed.

::: browser/base/content/browser.css
@@ +923,5 @@
>    width: 260px; /* CHAT_WIDTH_OPEN in socialchat.xml */
>  }
>  
>  chatbox[customSize] {
> +  width: 350px; /* CHAT_WIDTH_OPEN_ALT in socialchat.xml */

Please update the value in socialchat.xml as well.

@@ +928,4 @@
>  }
>  
>  #chat-window[customSize] {
>    min-width: 300px;

I think this should be kept in sync with the width, i.e. 350px.
Attachment #8669628 - Flags: review?(standard8) → review+
Attachment #8669628 - Attachment is obsolete: true
Attachment #8671209 - Flags: review+
New patch with the minor changes that :Standard 8 said in Comment 18. Keeping the r+ since there are just a few css changes but feel free to make a quick review, if you think it's necessary
Flags: needinfo?(standard8)
It wasn't quite right. Looking back at comment 18 I could have been clearer (and the context there didn't help). CHAT_WIDTH_OPEN_ALT was the one to alter in socialchat.xml (as that's the one that Loop uses).

This is with the change applied to the ALT version rather than the other one.
Attachment #8671209 - Attachment is obsolete: true
Attachment #8671262 - Flags: review+
Flags: needinfo?(standard8)
https://hg.mozilla.org/mozilla-central/rev/b96e1e328e6d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Iteration: 43.3 - Sep 21 → 44.2 - Oct 19
You need to log in before you can comment on or make changes to this bug.