1220627 - As a desktop client user, I want to be able to share simply the Hello URL through a sharing panel after I create a room.

Status: RESOLVED FIXED

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

Comment 1

[Romain Testard [:RT]]

As a desktop client user, I want to be able to share simply the Hello URL through a sharing panel after I create a sharing session. 

Acceptance criteria:
- The share panel slides after room creation
- Copy, Facebook and E-mail buttons are available on the share panel:
--- If the user hits "Copy", the button turns into "Link copied"
--- If the user hits the "email" button, the mailto:// handler opens (pop-up if Firefox is the mailto:// handler)
--- If the user hits the "facebook" button, a pop-up opens with the Facebook Send dialogue
--- If the user click away from the panel, the panel disappears and the conversation window appears with the persistent sharing infobar

Comment 2

[Romain Testard [:RT]]

Attachment: Share_panel_visual.png

Share panel visual spec

Comment 3

[Romain Testard [:RT]]

Visual spec and acceptance criteria now finalized, this is ready now to be taken for bug breakdown.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

A few questions about the visual spec:

I'm guessing that the intent is for:  

** the top panel to slide into view from the right
** the back panel to be darkened while the top panel is opened?

Both of these probably want to be added to the acceptance criteria if they're correct.

* the top panel to be about 80% of the width of the bottom panel, correct?

Finally, as far as I can tell, the bottom panel in the visual spec is the wrong width (at least compared to the current github Mac version), which makes me wonder if top panel in the visual spec is also the wrong width.

Guidance would be appreciated....

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Also, I assume the "your link" text box is intended to be a read-only textbox, not just some text with an outline?

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

As currently written, the breakdown looks something like this:

- refactor btn-copy out of DesktopRoomInvitationView in roomViews.jsx into it's own React component in a new file (so that it can be used easily from both the conversation window and the panel)
- layout and style share panel with just email button
- render the share panel above the existing panel content using absolute positioning
- add optional textbox functionality to btn-email (possibly moving the Copied/Link copied behaviors into a callback prop specified by the embedding widget
- refactor btn-email similarly
- refactor btn-facebook similarly
- implement the darkening
- implement the slide-in, probably using a CSS transition

I suspect it's reasonable to do this in one bug. 

Thoughts on the above appreciated...

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Hmm, we'll probably also want to add ui-showcase views for both the DesktopRoomInvitationView as well as the SharePanelView as well.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

:sevaan, :pau, we realized that this has surprising UX flow implications in triage today.  I don't think it makes sense to move forward until we've had some clarification, which I suspect may involve some design adjustments.

In the current code, if one clicks the "Browse this page with a friend" button, it opens up the conversation panel at the bottom of the screen and starts sharing their tab.

In the new flow, it apparently doesn't.  Presumably one of two things happens, either of which seems like it has surprising consequences:

* it could open up as soon as the user mouses off of the panel.  This seems like it would feed odd.

* it could open as soon as the other person joins and a notification is clicked.  It seems like users might feel confused while they're learning the product, since they have no indication that they don't have to do anything, and they'll be notified in the future.

This second scenario also trips on problem with notifications that we already have: if somebody clicks on a notification, it seems very likely that they won't be expecting to suddenly be sharing whatever page they're looking at (maybe their private email!) with whoever is in the room.

In whatever solution we end up with, do we want to completely remove the sharing mechanism from the invitation view?  Or would we leave it there in case they want to try some other sharing method because perhaps the first one didn't work?

Comment 9

[Romain Testard [:RT]]

> This second scenario also trips on problem with notifications that we
> already have: if somebody clicks on a notification, it seems very likely
> that they won't be expecting to suddenly be sharing whatever page they're
> looking at (maybe their private email!) with whoever is in the room.

If you click on the notification, Firefox is brought to foreground and a new tab opens opening the Room URL that the user initially wanted to share, and sharing starts. So there is no risk of sharing something you did not expect to share.

Comment 10

[Romain Testard [:RT]]

Lowering priority since we want to discuss with UX before it gets picked-up

Comment 11

[Sevaan Franks [:sevaan]]

As soon as someone clicks the "Browse this page with a friend" button, the infobar is supposed to drop. We could also just show the conversation window at the same time. The flow doesn't have to change much...it's just a more in-your-face sharing option that makes it clear to a user what to do next.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Do we want to completely remove the sharing mechanism from the invitation view?  Or would we leave it there in case they want to try some other sharing method because perhaps the first one didn't work?

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

If we have both open at the same time as suggested in comment 11, and do keep the old buttons in the invitation view, then we have sharing affordances in two parts of the screen at once, which seems a little curious, but maybe is fine.

Also, the questions in comment 4 and comment 5 still want answers...

Comment 14

[Pau Masiá [:Pau]]

I think we can solve this by graying out the conversation window when it first appears and once the user has shared the Hello Room by any means, the share panel pops out and the conversation window becomes active (with the current sharing buttons also enabled).

Comment 15

[Sevaan Franks [:sevaan]]

We could also leave the conversation window empty (or show a spinner maybe) if the share panel is open, and then when a user clicks off the share panel, move the share instructions to the conversation window. It might be cleaner.

What do you think Pau?

Comment 16

[Pau Masiá [:Pau]]

I agree. It can work as well.

Comment 17

[Romain Testard [:RT]]

Sounds good, could you please provide UX details for this?
Also can you provide answers to Dan's comment 4 and comment 5?

Comment 18

[Sevaan Franks [:sevaan]]

Attachment: Share Panel Spec (Updated Sizes)

> I'm guessing that the intent is for:  
> 
> ** the top panel to slide into view from the right
> ** the back panel to be darkened while the top panel is opened?
> 
> * the top panel to be about 80% of the width of the bottom panel, correct?

This already exists in Firefox. You can see exactly how this should work by clicking on the History item in the hamburger menu (move it there if you need it).

(In reply to Dan Mosedale (:dmose) from comment #5)
> Also, I assume the "your link" text box is intended to be a read-only
> textbox, not just some text with an outline?

Yes, it's a text box.

> Finally, as far as I can tell, the bottom panel in the visual spec is the
> wrong width (at least compared to the current github Mac version), which
> makes me wonder if top panel in the visual spec is also the wrong width.

You're right. Updated spec attached with new widths.

Comment 19

[Sevaan Franks [:sevaan]]

Attachment: Conversation Window contents for sharing

(In reply to Romain Testard [:RT] from comment #17)
> Sounds good, could you please provide UX details for this?

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

    I've spun off 1242706 for the conversation window part of this latest mock.  This bug also wants to be retriaged, since my understanding from today's planning meeting is that that bug is needed for 45, while this one is not.

Comment 21

[Ian Bicking (:ianbicking)]

I think it is important for us to see how this feels and works before committing the feature.  I believe Manu has already done work on this, so assigning to him.  We'd like to see a working prototype (in a branch) earlier than later before doing all the work necessary to prepare the feature to land.

Comment 22

[GitHub Autolander Bot]

Attachment: [loop] mancas:bug1220627 > mozilla:master

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 8717830 [details] [review]
[loop] mancas:bug1220627 > mozilla:master

I know Ian and myself wanted to review the UX and feel of this before it landed as there are some parts of the flow that feel strange for us. 

Therefore redirecting to Ian to get that discussion started.

Comment 24

[Ian Bicking (:ianbicking)]

In addition to regular code review, we also want to see how this experience works in the product, including the corner cases, before shipping.  For this release we haven't had sufficient time between landing and shipping to see how a feature interacts with the flow of the project.  We should make this a priority after the next version ships so we can see how it interacts with other flows.

Comment 25

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8717830 [details] [review]
[loop] mancas:bug1220627 > mozilla:master

We need to review this code although Ian and Mark want to test the flow so assigning reviewers again

Comment 26

[Romain Testard [:RT]]

We agreed to review and land this bug so we have enough time to test the flow.

Comment 27

[Mike de Boer [:mikedeboer]]

Comment on attachment 8717830 [details] [review]
[loop] mancas:bug1220627 > mozilla:master

Tapping out ;-)

But this is one case that really shows how inappropriate Github PRs can be to review large patches. It's just too overwhelming for reviewers to start digging in, whereas bitesized patched that can be reviewed individually are much nicer.

The only workaround I can think of at the moment is to use Bugzilla/ Splinter:
1) Split the work into different commits
2) Attach a git diff of each commit to the bug and request review
3) Re-request review for follow-up commits (interdiffs)
4) Squash & land.

...which is faaaaaaar from ideal.

Comment 28

[Ed Lee :Mardak]

How should this behave if there aren't many recently browsed sessions? I.e., the panel will be short.

Comment 29

[Chris Rafuse :crafuse]

Lots of Travis CI errors.  Need to fix before review.  Use make test to see locally.

Comment 30

[Manuel Casas Barrado [:mancas]]

:Mardak we have a problem when there is no room in the list. So we need to think how to solve it, the easier way is to increse the minimum height of the panel to fit the share panel.

:crafuse linter errors fixed

Comment 31

[Romain Testard [:RT]]

Sevaan, would increase the minimum panel height to the seize of the share panel be acceptable?

Comment 32

[Sevaan Franks [:sevaan]]

(In reply to Romain Testard [:RT] from comment #31)
> Sevaan, would increase the minimum panel height to the seize of the share
> panel be acceptable?

Yes. This also currently exists in product. If you have a short menu panel with the history icon in it (for example), when you click the history button, the panel grows.

Comment 33

[Ed Lee :Mardak]

Attachment: history panel resizing video

(In reply to Sevaan Franks [:sevaan] from comment #32)
> If you have a short menu panel with the history icon in it (for example),
> when you click the history button, the panel grows.
Oh neat. Here's a video to compare to show what sevaan is talking about.

Comment 34

[Ed Lee :Mardak]

Looks like the menu panel resizes automatically on overflowing content (thanks Gijs for the reference):

https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.xml#288

I'm guessing the panel we're in doesn't support arbitrary dynamic resizing and only multiples of room list sizes that dcritch implemented?

Comment 35

[David Critchley (:dcritch)]

The panel will dynamically resize to fit the content within it. Latest build should include the FTU within the empty roomlist which demonstrates this function. 

The type of panel used for the roomlist, however, does have a minimum height set in Firefox, so if we want the panel to shrink smaller than that set min height, we will, I believe, need to change code within the browser.

Comment 36

[Mark Banner (:standard8)]

Attachment: Screen shot on Mac

This is how I see he patch on my Mac. It appears it doesn't fully fit in the display...

Comment 37

[Mark Banner (:standard8)]

Comment on attachment 8717830 [details] [review]
[loop] mancas:bug1220627 > mozilla:master

I've taken a look and put some architecture comments in the bug.

Comment 38

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8717830 [details] [review]
[loop] mancas:bug1220627 > mozilla:master

Comments addressed. Please take a look when you get a chance

Thanks

Comment 39

[Mark Banner (:standard8)]

Comment on attachment 8717830 [details] [review]
[loop] mancas:bug1220627 > mozilla:master

Ok, I definitely like this architecture a lot more - much better.

I've been through it and left comments on the PR.

Comment 40

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8717830 [details] [review]
[loop] mancas:bug1220627 > mozilla:master

Mark I've addressed your comments but I have a few problems with the functional tests. We need to wait for the share panel to be displayed, then click in the overlay to launch the conversation window. I've added a new function but not seems to work, can you give me an advice to implement the test?

Comment 41

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8717830 [details] [review]
[loop] mancas:bug1220627 > mozilla:master

Functional test issue fixed

Comment 42

[Mark Banner (:standard8)]

Manu, I've started commenting. I think I've just got the tests to check over. I'll will get to them tomorrow afternoon at the latest.

Comment 43

[Manuel Casas Barrado [:mancas]]

Attachment: Min-height

Mark, I'm having problems with the auto-resize panel. Setting the height of the share panel to 100%, if there is no enough room for it, the result is a cropped panel. however, if setting a min-height the result is a bigger panel. Thoughts?

Note: min-height: 200px

Comment 44

[Mark Banner (:standard8)]

(In reply to Manuel Casas Barrado [:mancas] from comment #43)
> Created attachment 8728845 [details]
> Min-height
> 
> Mark, I'm having problems with the auto-resize panel. Setting the height of
> the share panel to 100%, if there is no enough room for it, the result is a
> cropped panel. however, if setting a min-height the result is a bigger
> panel. Thoughts?
> 
> Note: min-height: 200px

I think what is happening is that the .share-panel-container element isn't positioned absolutely.

So, the layout engine, sees the min-height of 200px, and adds that onto the height of the room list and footer.

I think what you need to do is to is to change it to an absolutely positioned container, but add another optional class on the .panel-container div to force the minimum size if the share panel is displayed.

Comment 45

[Mark Banner (:standard8)]

Comment on attachment 8717830 [details] [review]
[loop] mancas:bug1220627 > mozilla:master

The tests are largely good - I left a couple of additional comments in the PR.

Cancelling review so I can check it over once the layout issue is fixed.

Comment 46

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8717830 [details] [review]
[loop] mancas:bug1220627 > mozilla:master

Comments addressed and layout issue fixed

Comment 47

[Mark Banner (:standard8)]

Comment on attachment 8717830 [details] [review]
[loop] mancas:bug1220627 > mozilla:master

That's great, much better. r=Standard8

Comment 48

[Manuel Casas Barrado [:mancas]]

Landed in master: https://github.com/mozilla/loop/commit/4c2f954a0b155fb0200ed70a7f05989533e93166