Closed Bug 1112264 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Points:
2

Tracking

(Not tracked)

RESOLVED FIXED
mozilla37
Iteration:
37.1
Blocking Flags:
backlog Fx35+

People

(Reporter: dmose, Assigned: dmose)

References

Details

(Keywords: privacy, Whiteboard: [standalone][rooms])

Attachments

(2 files, 1 obsolete file)

+++ 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.
backlog: --- → Fx35+
Assignee: nobody → dmose
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.
Flags: needinfo?(matej)
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.
Attachment #8538778 - Flags: review?(nperriault)
Blocks: 1113393
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?
(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.
Flags: needinfo?(matej)
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”
Attachment #8538778 - Flags: review?(nperriault) → review+
Points: --- → 2
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.
Attachment #8539404 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4cef5d18f14f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Iteration: --- → 37.1
You need to log in before you can comment on or make changes to this bug.