Closed Bug 1086512 Opened 10 years ago Closed 10 years ago

Feedback form support in rooms for Desktop

Categories

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

x86
All
defect
Points:
3

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.3
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
backlog Fx35+

People

(Reporter: shell, Unassigned)

References

Details

(Whiteboard: [rooms][good first verify])

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.

Attachments

(3 files, 3 obsolete files)

      No description provided.
backlog: --- → Fx35?
Whiteboard: [rooms]
(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?
Flags: needinfo?(dhenein)
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.
Flags: needinfo?(dhenein)
backlog: Fx35? → Fx35+
Priority: -- → P1
Blocks: 1074659
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.
Flags: needinfo?(standard8)
I'm wrong -- there is a bug for standalone bug for the feedback forms: bug 1079225.
Flags: needinfo?(standard8)
Summary: Feedback form support in rooms → Feedback form support in rooms for Desktop
I believe Niko is starting on this tomorrow morning.
Assignee: nobody → nperriault
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
Flags: needinfo?(standard8)
Flags: needinfo?(rtestard)
Flags: needinfo?(mreavy)
Flags: needinfo?(dhenein)
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.
Depends on: 1102354
Discussed on irc, Sevaan will follow-up with UX recommendation.
Flags: needinfo?(dhenein) → needinfo?(sfranks)
Attached image Leave Room Icon
(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.
Flags: needinfo?(sfranks)
(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?
Flags: needinfo?(mreavy) → needinfo?(sfranks)
(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.
Flags: needinfo?(standard8)
Flags: needinfo?(rtestard)
This requires patch for bug 1076754, which should land soon.
Attachment #8526733 - Flags: review?(standard8)
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!
Flags: needinfo?(mmaslaney)
Attached image exit_room_icon.svg
SVG of icon attached.
Flags: needinfo?(sfranks)
Flags: needinfo?(mmaslaney)
Rebased on top of latest fx-team, added the SVG icon to icons-16x16.svg.
Attachment #8526733 - Attachment is obsolete: true
Attachment #8526733 - Flags: review?(standard8)
Attachment #8527809 - Flags: review?(standard8)
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.
Attachment #8527809 - Flags: review?(standard8) → review-
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.
Attachment #8527809 - Attachment is obsolete: true
Attachment #8527934 - Flags: review?(standard8)
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.
Attachment #8527934 - Flags: review?(standard8) → review+
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.
Attachment #8527934 - Flags: review+ → review-
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!
Attachment #8528302 - Flags: review?(standard8) → review+
https://hg.mozilla.org/mozilla-central/rev/54655536563e
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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
Attachment #8528302 - Flags: approval-mozilla-aurora?
Flags: qe-verify-
Flags: in-moztrap-
Whiteboard: [rooms] → [rooms][good first verify]
Attachment #8528302 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.)
Flags: needinfo?(paul.silaghi)
Whilst its not the centre of the remote video - its the centre of the display. We should discuss it in the follow-up.
Depends on: 1105809
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #32)
> Pauly -- Can you file a new bug
bug 1105809
Flags: needinfo?(paul.silaghi)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: