1112264 - self-image can be cropped by being out of scroll (standalone client)

Status: RESOLVED FIXED

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

Description

[Dan Mosedale (:dmosedale, :dmose)]

+++ This bug was initially created as a clone of Bug #1101378 +++

Now in theory, they should be able to guess this by virtue of visible scrollbars on the window, which appear to be there at least on OSX Yosemite using Nightly, and presumably also on Windows and Linux.  I suspect there are still problems with the mobile situation, however.

In any case, this is fragile, and it would be a better user experience to simply make it impossible for the user to put themselves in this situation.

Some possibilities to investigate include position: fixed on the publisher image, and playing with max-height and overflow on the window.  If position: fixed isn't good enough, we might be able to use a position: sticky polyfill, since that isn't yet implemented everywhere.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Firefox_Hello_and_loop_properties_-__loop__-_gecko-dev_-____r_gecko-dev__-_IntelliJ_IDEA__Cassiopeia__IU-139_791_2.png

I've got a basic patch that hides the self-view.  Here's a screenshot of how that looks.

I'll speculate that this verbiage is a reasonable start, and I don't think we should block on perfecting it.  That said, matej, if you've got any thoughts on improving it, now would be a great time to shout.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Hide loop self-view with message if it would be incompletely shown

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8538778 [details] [diff] [review]
Hide loop self-view with message if it would be incompletely shown

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

This patch is a fragile fix based on a heuristic related to what the current layout looks like.  It fixes the problem for users resizing the window and it going out of scroll.  It doesn't not fix the problem when it's caused by folks using cmd + and cmd - key combinations.

I propose to land this now, and file a spin-off bug about what we really want to do, which is:

* implement a more robust mechanism that won't break as the layout changes.  I have some ideas which I can put in that bug.
* ensure that we handle the cmd + and cmd - cases as well.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Spin-off bug is 1113393.  A few other notes about this patch:

* I've tested it with a super long string for localizations like German, and it wraps the text quite reasonably.

* I didn't make an analogous change to sharedViews.ConversationView, because it only appears to be used in the OutgoingConversationView in the standalone, which I _think_ is only for the "link clicker call" scenario that will never be generated by Fx 35 and later.  Is this assumption correct?

Comment 5

[Matej Novak [:matej]]

(In reply to Dan Mosedale (:dmose) - use needinfo? (not reading bugmail regularly) from comment #1)
> Created attachment 8538776 [details]
> Firefox_Hello_and_loop_properties_-__loop__-_gecko-dev_-____r_gecko-dev__-
> _IntelliJ_IDEA__Cassiopeia__IU-139_791_2.png
> 
> I've got a basic patch that hides the self-view.  Here's a screenshot of how
> that looks.
> 
> I'll speculate that this verbiage is a reasonable start, and I don't think
> we should block on perfecting it.  That said, matej, if you've got any
> thoughts on improving it, now would be a great time to shout.

I think we could remove the "please" on this one, but otherwise it looks OK to me.

Comment 6

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8538778 [details] [diff] [review]
Hide loop self-view with message if it would be incompletely shown

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

Looks good and addresses the problem, r=me with comments addressed.

::: browser/components/loop/content/shared/css/conversation.css
@@ +955,5 @@
>      display: none;
>    }
>  }
> +
> +.please-resize {

Nit: This one deserves a a comment.

@@ +967,5 @@
> +@media screen and (max-height:160px) {
> +
> +  /* disable the self view */
> +  .standalone .OT_publisher {
> +    display: none;

We hide the video, but it's still streaming… Makes me thing that…

::: browser/components/loop/standalone/content/l10n/en-US/loop.properties
@@ +40,5 @@
>  legal_text_and_links=By using {{clientShortname}} you agree to the {{terms_of_use_url}} and {{privacy_notice_url}}
>  terms_of_use_link_text=Terms of use
>  privacy_notice_link_text=Privacy notice
>  invite_header_text=Invite someone to join you.
> +self_view_hidden_message=Self-view hidden; please resize window to show

… we should probably reflect it in the message, eg. “Your self-video is hidden but still streaming; enlarge window to show”

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Hide loop self-view with message if it would be incompletely shown

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8539404 [details] [diff] [review]
Hide loop self-view with message if it would be incompletely shown

Updated patch addressing review comments.  Talked to Niko and he confirmed my assumption in comment 4.  Carrying forward r=Niko.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/fx-team/rev/4cef5d18f14f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4cef5d18f14f