(Commenting on User Story) > As a FF browser user closing a room (x at the top right), I can provide > feedback on my calling experience so that I can let Mozilla know how I feel > about the product. > Design: Same as Fx34 but when the user closes the room window, a new small > window at the bottom routed to the feedback form appears. This seems pretty jarring from a user perspective. Would it not be better to provide some prompt or something? Also, how does this work on the standalone side? Finally, are we using the existing feedback layout/text?

Agreed, its not the smoothest transition, but its the quickest way to having something implemented. Standalone side is similar to now, where if a user leaves the room using our controls, we display the feedback form in content. If they close the tab/window, we can't show them anything either way. As for layout/text, I believe so, though I would confirm with Romain/Arcadio.

Mark -- Do you want to have 2 separate bugs for "Feedback form support" for Rooms: one for desktop and one for standalone? Currently, we only have this one bug.

I'm wrong -- there is a bug for standalone bug for the feedback forms: bug 1079225.

I believe Niko is starting on this tomorrow morning.

Just marked bug 1076754 as a dependency of this, because we want to reuse the Flux goodness we introduced lately and which has been prove a time saver.

So after a little investigation, two things: - The Loop desktop conversation window doesn't trigger any "beforeunload" event, so we can't "hook" before the window closes to display the feedback form; - … and anyway, whenever a regular "beforeunload" event is caught and default-prevented, it will still always prompt the user for confirmation using a system confirm()[0], which is not what we want here(?). I can't see any other option than exposing something we can subscribe to through the mozLoop API… Now let me read again the user story: > When the user closes the room window, a new small > window at the bottom routed to the feedback form appears. If that's a *new window*, it's a bit different as this could be dealt with from chrome (assuming we can detect that a room conversation window has just been closed). Thoughts? [0]: https://developer.mozilla.org/en-US/docs/Web/Events/beforeunload

More thoughts: [18:36:50] NiKo`: maybe we could use that confirm() dialog though [18:37:11] NiKo`: eg. if confirm == true -> close the window, else we've been displaying the feedback form in the meanwhile [18:37:28] NiKo`: that should work just fine, actually [18:37:48] NiKo`: (if a confirm() prompt is acceptable)

Whatever solution we want, I've filed bug 1102354 which I think is a blocker anyway here.

Discussed on irc, Sevaan will follow-up with UX recommendation.

(In reply to Darrin Henein [:darrin] from comment #3) > Agreed, its not the smoothest transition, but its the quickest way to having > something implemented. Standalone side is similar to now, where if a user > leaves the room using our controls, we display the feedback form in content. > If they close the tab/window, we can't show them anything either way. The experience in rooms should be exactly the same as described above. Closing X should never result in a decision tree...X means close and that's it. However, if a user leaves the room using our controls, the feedback form is displayed in the window. This means we will need to add a "Leave Room" button much like our "Hang Up" button. I've attached a mockup of what that icon could look like.

(In reply to Sevaan Franks [:sevaan] from comment #13) > This means we will need to add a "Leave Room" button much like our "Hang Up" > button. I've attached a mockup of what that icon could look like. This looks reasonable to me.

(In reply to Adam Roach [:abr] from comment #14) > (In reply to Sevaan Franks [:sevaan] from comment #13) > > This means we will need to add a "Leave Room" button much like our "Hang Up" > > button. I've attached a mockup of what that icon could look like. > > This looks reasonable to me. + 1 from me too Sevaan -- Can we get an svg of this "leave room" icon? If so, how soon? :-) Many thanks for jumping on this so quickly! Niko -- I assume this new approach will work well for you?

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #15) > Niko -- I assume this new approach will work well for you? Yes, and as a bonus, minus the icon everything is already in place, just hidden :)

Clearing needinfos.

This requires patch for bug 1076754, which should land soon.

Doing a needinfo to Mike as well Hi Mike -- We need a svg for the "leave room" icon that Sevaan put up a ping for. Is that something you can get for us today (ASAP)? I asked Sevaan, but then it occurred to me that you usually handle svg icons IIRC. NOTE: We have a patch up for review using a placeholder (an empty red button). We could use the phone icon temporarily, but that would be confusing and ugly since this is all about rooms and not calls. So we'd really like to have this icon when we land. Thanks!

SVG of icon attached.

Rebased on top of latest fx-team, added the SVG icon to icons-16x16.svg.

Comment on attachment 8527809 [details] [diff] [review] Added feedback form to Loop desktop room window, patch v2. Review of attachment 8527809 [details] [diff] [review]: ----------------------------------------------------------------- This would be r+ if it wasn't for the slight hiccup of the leave button in standalone display. I also ponder if the leave button should be available if the remote party has left the conversation - but lets leave that until later (we'll do a rooms review with UX). ::: browser/components/loop/content/shared/css/conversation.css @@ +745,5 @@ > } > > +.room-conversation .conversation-toolbar .btn-hangup { > + /* XXX background image: leave-room */ > + background-image: url("../img/icons-16x16.svg#leave"); This breaks the standalone display - you get the icon overlaid on top of the leave button.

I realized the previous patch was updated against the wrong version; here's a new one matching the previous improvements (notably sound handling), plus your comment addressed.

Comment on attachment 8527934 [details] [diff] [review] Added feedback form to Loop desktop room window. Review of attachment 8527934 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the additional change suggested below. ::: browser/components/loop/content/shared/js/activeRoomStore.js @@ +38,5 @@ > FAILED: "room-failed", > // The room is full > + FULL: "room-full", > + // The room conversation has ended > + ENDED: "room-ended" I think you need to add this new state to "notConnectedToRoom" in mixins.js - to ensure we get the room-left sound when the feedback is displayed.

Comment on attachment 8527934 [details] [diff] [review] Added feedback form to Loop desktop room window. As discussed on irc, this doesn't adjust the StandaloneRoomView#componentWillUpdate function to re-setup the media elements, which means the media doesn't get prompted for when rejoining the room.

Revised patch.

Comment on attachment 8528302 [details] [diff] [review] Added feedback form to Loop desktop room window, patch v2. Review of attachment 8528302 [details] [diff] [review]: ----------------------------------------------------------------- That's better, thanks!

Comment on attachment 8528302 [details] [diff] [review] Added feedback form to Loop desktop room window, patch v2. Approval Request Comment [Feature/regressing bug #]: Rooms [User impact if declined]: No feedback from users using rooms (as we get from direct FxA calls, and we did get before rooms using link-clicking). [Describe test coverage new/current, TBPL]: Includes tests [Risks and why]: Fairly low; most of the changes required are small as the functionality already existed before Rooms. [String/UUID change made/needed]: none

Note that the feedback form is not centered to the video screen for the standalone client. http://i.imgur.com/c00Jv5j.png

(In reply to Paul Silaghi, QA [:pauly] from comment #31) > Note that the feedback form is not centered to the video screen for the > standalone client. > http://i.imgur.com/c00Jv5j.png Pauly -- Can you file a new bug against the standalone client for this? (This only affects Rooms, not calls. So it doesn't affect fx34 calling.)

Whilst its not the centre of the remote video - its the centre of the display. We should discuss it in the follow-up.

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #32) > Pauly -- Can you file a new bug bug 1105809