Rework link-clicker display for text chat

VERIFIED FIXED in Firefox 41

Status

defect
P1
normal
Rank:
13
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla41
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox41 verified)

Details

(Whiteboard: [chat])

User Story

Rework the existing standalone display such that:

- Main video stream is on the left
- Room context, text chat, and secondary video streams are down the right.
- If Chat is not enabled, then both text entry and text display should be hidden (as they are today).

The room context should be shown above the text chat for the time being.

If possible, consider mobile layout during implementation.

What's not included:

- Moving room context inside the text chat display element (it should be shown above it in the "column", but not inside.
- Re-arranging header/footer.
- Re-arranging the call control buttons (leave in footer)
- Layout work within the text entry/text boxes

Attachments

(3 attachments, 3 obsolete attachments)

For text chat, we need to rework the standalone layout as required by the latest mock-ups (to be attached).

See user story for what needs doing.
User Story: (updated)
Flags: qe-verify+
Flags: firefox-backlog+
Rank: 20
Whiteboard: [chat]
Rank: 20 → 21
Assignee: nobody → standard8
Iteration: --- → 41.2 - Jun 8
Blocks: 1171933
Rank: 21 → 13
Priority: P2 → P1
Depends on: 1119000
Iteration: 41.2 - Jun 8 → 41.3 - Jun 29
Depends on: 1174714
Ok, these patches apply on top of the two in bug 1173909.

This first patch strips out the existing dynamic placement of the video for the standalone view. It also removes the existing StandaloneRoomContext* as they aren't going to be used any more.

I've left some of the video dimensions things in the mixin, as its unclear yet if we will need them or not to fix the remaining issues (I'll do a follow-up comment in a moment).
Attachment #8623640 - Flags: review?(mdeboer)
This is the main part of the work. It drops the old layout, and creates a new one, currently based around a "media-layout" div. This is flex based, with the first column forced to contain just the remote video (or screen share), and then everything else pushed into subsequent columns by the use of wrapping.

The aim of the new media-layout div was to give us a fresh "component" from which to hang our styles. I'm also expecting that we'll turn it into its own view and share it across desktop rooms & direct calls. However that'll take a bit more work, so I haven't done that yet.

There were some other styles that were altering things in the tree, so I've scoped those to the older ".conversation" class for the old call-urls, until we manage to finally deprecate those.

The ui-showcase has also been updated with another narrow-width view. The narrow width views aren't the best, but they're better than what we have now, and we can iterate more as we head towards the main refresh.

The rest of the ui-showcase views are all representative of the real thing, and were really helpful in debugging this.
Attachment #8623643 - Flags: review?(mdeboer)
This is a small patch on top, to fix some of the more obvious RTL issues, now that the layout of media elements is much better.

I've not touched things like the conversation toolbar - that needs a more complex patch, and it doesn't look dreadfully bad in rtl mode anyway (unlike what the rest of it looked like).
Attachment #8623644 - Flags: review?(mdeboer)
Things that still need addressing (for other bugs!):

- Are we happy to use object-fit? (need to discuss with product/ux, will do so today).
- Using a super-short display causes bad effects.
- If we need to handle different ratios for local video other than 4:3 (we need to display all the video, and if so, we should adjust the height of the chat section to accommodate display).
Updated a little bit of the css layout, following a ui-review from sevaan over irc - the footer & header are now 10px margins, the help icon is more vertically central, but the FF logo is still overlapped by the beta logo a little but not too much.
Attachment #8623643 - Attachment is obsolete: true
Attachment #8623643 - Flags: review?(mdeboer)
Attachment #8623856 - Flags: review?(mdeboer)
Blocks: 1175825
Attachment #8623640 - Flags: review?(mdeboer) → review+
Comment on attachment 8623644 [details] [diff] [review]
Part 3. Fix some RTL issues associated with the new layout for Loop's text-chat.

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

::: browser/components/loop/standalone/content/css/webapp.css
@@ +353,5 @@
>  .standalone .room-conversation-wrapper .ended-conversation .feedback {
>    right: 35%;
>  }
>  
> +html[dir="rtl"] .standalone .room-conversation-wrapper .ended-conversation .feedback {

ugh, super in-efficient selector. But ok.
Attachment #8623644 - Flags: review?(mdeboer) → review+
Comment on attachment 8623856 [details] [diff] [review]
Part 2. Adjust Loop's Standalone UI to include text chat for conversations.

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

I believe this is all good, but I'd really like to see an effort to use fewer sibling selectors and more child-selectors. Our CSS is getting slower and slower to process and reflow.

::: browser/components/loop/content/shared/css/conversation.css
@@ +1161,5 @@
> +}
> +
> +@media screen and (max-width:640px) {
> +  .media-layout {
> +    /* XXX Doc me */

Please follow-up here ;)

@@ +1571,5 @@
>  
>  .remote-video {
>    width: 100%;
>    height: 100%;
> +  /* XXX

Care to complete this work?
Attachment #8623856 - Flags: review?(mdeboer) → review-
(In reply to Mike de Boer [:mikedeboer] from comment #7)
> I believe this is all good, but I'd really like to see an effort to use
> fewer sibling selectors and more child-selectors. Our CSS is getting slower
> and slower to process and reflow.

Whoops, I meant descendent selectors, not sibling selectors :-/
Updated the css to use child selectors. I've only touched the ones I've added. I'm hoping we'll be able to get rid of the older css soon.
Attachment #8624171 - Flags: review?(mdeboer)
Attachment #8623856 - Attachment is obsolete: true
Correct the text chat layout.
Attachment #8624171 - Attachment is obsolete: true
Attachment #8624171 - Flags: review?(mdeboer)
Attachment #8624176 - Flags: review?(mdeboer)
Comment on attachment 8624176 [details] [diff] [review]
Part 2. Adjust Loop's Standalone UI to include text chat for conversations.

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

All done! Cool!

::: browser/components/loop/content/shared/css/conversation.css
@@ +1247,5 @@
> +    /* Screen shares have remote & local video side-by-side on narrow screens */
> +    order: 3;
> +    flex: 1 1 auto;
> +    height: 20%;
> +    /* Ensure no previously specified widths take affect, and we take up no more

nit: s/affect/effect/

@@ +1251,5 @@
> +    /* Ensure no previously specified widths take affect, and we take up no more
> +       than half the width. */
> +    width: auto;
> +    max-width: 50%;
> +    /* This cancels out the absolute positioning when its just remote video. */

nit: s/its/it's/
Attachment #8624176 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/mozilla-central/rev/ec9c9d0b9d75
https://hg.mozilla.org/mozilla-central/rev/f160bb5abec1
https://hg.mozilla.org/mozilla-central/rev/5442bb7741fc
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
QA Contact: bogdan.maris
I did some exploratory testing around this fix using Firefox Developer Edition 41.0a2 across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit) and I can confirm the layout implementation is according to user story (the other way around RTL builds though - expected), found no new issue so I'll mark this as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.