Closed Bug 1131688 Opened 8 years ago Closed 8 years ago

In the standalone view the conversation window we're currently overlaying the toolbar on top of the remote video

Categories

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

defect
Points:
2

Tracking

(firefox38 fixed)

RESOLVED FIXED
mozilla38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- fixed
backlog Fx38+

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

Currently the conversation toolbar overlaps the remote video on the standalone UI. This means we loose the lower part of the video - this is most obvious with screen sharing.

I also want to fix this ahead of bug 1126321.
This fixes the issue by changing the height of the remote media to be reduced by the height of the conversation toolbar - except when we're in-call, or if we're shrunk down small.

I didn't want to change the height of the video wrapper/media elements as that affects the local media as well.
Attachment #8562272 - Flags: review?(dmose)
Comment on attachment 8562272 [details] [diff] [review]
In the standalone view the conversation window we're currently overlaying the toolbar on top of the remote video.

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

::: browser/components/loop/content/shared/css/conversation.css
@@ +32,5 @@
>  }
>  
> +.standalone .remote-stream {
> +  /* Set at maximum height, minus height of conversation toolbar */
> +  height: calc(100% - 64px);

Does this work when you resize the browser, as in doesn't the resizing code override this style rule?

I'd expect that the toolbar element would reside _outside_ of the stream, so that it'd be positioned relatively to the stream container. Wouldn't that be easier?
(In reply to Mike de Boer [:mikedeboer] from comment #2)
> > +.standalone .remote-stream {
> > +  /* Set at maximum height, minus height of conversation toolbar */
> > +  height: calc(100% - 64px);
> 
> Does this work when you resize the browser, as in doesn't the resizing code
> override this style rule?

If you're thinking of the resizing code in updateVideoContainer to set it to 100%, then no it doesn't affect this - as that's on a different element.

> I'd expect that the toolbar element would reside _outside_ of the stream, so
> that it'd be positioned relatively to the stream container. Wouldn't that be
> easier?

I tried it, and it gave an issue with the inset stream - which would then also be shifted up 64px. In retrospect I think we might be able to adjust where the inset stream is, and force it over the toolbar - I'll have a look another look at that approach and see if its any simpler.
Comment on attachment 8562272 [details] [diff] [review]
In the standalone view the conversation window we're currently overlaying the toolbar on top of the remote video.

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

Stealing review, Mark and I discussed this over Vidyo and looks like a big improvement.
Attachment #8562272 - Flags: review?(dmose) → review+
Just to clarify a couple of things for future reference - Moving the conversation toolbar in the dom tree also affects where the local video gets displayed. So we'd need to adjust its absolute position (vertically) to overlap with the toolbar.

Additionally, the media elements heights are currently set to 100%, so we'd also need to calculate the height minus the toolbar as well.

Hence, the additional work necessary doesn't seem worth it.
https://hg.mozilla.org/integration/fx-team/rev/34652ec8485c
Target Milestone: --- → mozilla38
https://hg.mozilla.org/mozilla-central/rev/34652ec8485c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.