1092953 - Loop "delete a room" operation should ask end-user confirmation

Status: VERIFIED FIXED

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

Description

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

We can't use a system confirm() modal prompt here, because of unwanted side effects:

- it closes (hides) the panel once the prompt is opened;
- it resets the CSS "active" state applied to any clicked element (the delete button), making styling using CSS animations really difficult (not impossible; just *very* difficult)

We should probably invent a tiny widget to require user confirmation inline, next to the delete button (an in-document html modal confirmation is another option). 

Note: With proper iconing, this shouldn't require new strings.

Comment 1

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

NI :darrin here.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: show a modal confirm dialog when a user attempts to delete a room

Mark, this is the bare minimum to get to into an acceptable state. There are too many possible side-effects when using an HTML prompt.

Comment 3

[Marco Mucci [:MarcoM]]

Added to IT 36.3

Comment 4

[Sevaan Franks [:sevaan]]

Attachment: Room deletion confirmation

Comment 5

[Darrin Henein [:darrin]]

Comment on attachment 8524820 [details]
Room deletion confirmation

Looks good, I think this fits well with the design language already established and conveys the severity of the action.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8524619 [details] [diff] [review]
Patch v1: show a modal confirm dialog when a user attempts to delete a room

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

Looks good, but please add a unit test for the panel change.

::: browser/components/loop/MozLoopAPI.jsm
@@ +357,5 @@
> +          buttonFlags = Services.prompt.STD_YES_NO_BUTTONS;
> +          callback = okButtonMessage;
> +          okButtonMessage = cancelButtonMessage = null;
> +        } else {
> +          buttonFlags = 

nit: trailing whitespace

::: browser/components/loop/content/js/panel.js
@@ +503,5 @@
>        }));
>        this.setState({urlCopied: true});
>      },
>  
>      handleDeleteButtonClick: function(event) {

Please can you add a unit test for this function.

Comment 7

[:Paolo Amadini]

Comment on attachment 8524619 [details] [diff] [review]
Patch v1: show a modal confirm dialog when a user attempts to delete a room

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

Looks good as a stopgap in order not to regress the notification added for Contacts.

I agree that an in-line UI would be better, though it requires much more effort.

::: browser/components/loop/MozLoopAPI.jsm
@@ +355,5 @@
> +        // A simple yes/ no prompt does not have button captions.
> +        if (arguments.length == 2) {
> +          buttonFlags = Services.prompt.STD_YES_NO_BUTTONS;
> +          callback = okButtonMessage;
> +          okButtonMessage = cancelButtonMessage = null;

I'd rather have an explicit buttonTypes indication than an arguments.length detection.

Since the callback must be the last argument and we now need a variable number of arguments, I recommend changing the signature to function(options, callback).

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: show a modal confirm dialog when a user attempts to delete a room

I'll add a mochitest in a separate patch.

Comment 9

[Mark Banner (:standard8)]

A mochitest is good, but we should also unit test panel.jsx in the style that we have been already - actually, I just realised, we have a test for this already:

http://hg.mozilla.org/mozilla-central/annotate/6ce1b906c690/browser/components/loop/test/desktop-local/panel_test.js#l736

I suspect that's broken with the current patch (I forgot to run the test suites).

Comment 10

[:Paolo Amadini]

Comment on attachment 8526036 [details] [diff] [review]
Patch v2: show a modal confirm dialog when a user attempts to delete a room

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

Thanks! r+ with the following change, and getting review on tests is required before landing.

::: browser/components/loop/content/js/contacts.js
@@ +407,5 @@
> +            message: mozL10n.get("confirm_delete_contact_alert"),
> +            buttons: [
> +              mozL10n.get("confirm_delete_contact_remove_button"),
> +              mozL10n.get("confirm_delete_contact_cancel_button")
> +            ]

I'd make clear which button is which.

buttons: {
  yes: "...",
  no: "...",
}

or

yesButton: "...",
noButton: "...",

I'd also throw if only one of these properties is specified.

Comment 11

[Mike de Boer [:mikedeboer]]

Attachment: Patch v3: show a modal confirm dialog when a user attempts to delete a room

Paolo, you mean like this?

Comment 12

[:Paolo Amadini]

Comment on attachment 8526071 [details] [diff] [review]
Patch v3: show a modal confirm dialog when a user attempts to delete a room

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +352,5 @@
>        writable: true,
> +      value: function(options, callback) {
> +        if (!("message" in options) ||
> +            !("okButton") in options ||
> +            !("cancelButton" in options)) {

Something strange is going on here...

@@ +364,3 @@
>              (Ci.nsIPrompt.BUTTON_POS_0 * Ci.nsIPrompt.BUTTON_TITLE_IS_STRING) +
>              (Ci.nsIPrompt.BUTTON_POS_1 * Ci.nsIPrompt.BUTTON_TITLE_IS_STRING);
> +        } else {

Maybe a simple way to do the check could be:

if (options.okButton && options.cancelButton)
  ...
else if (!options.okButton && !options.cancelButton)
  ...
else
  error

Rest looks good!

Comment 13

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.4: show a modal confirm dialog when a user attempts to delete a room

Carrying over r=paolo.

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: pdate the room delete button test to take the confirm dialog into account

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8529112 [details] [diff] [review]
Patch 2: pdate the room delete button test to take the confirm dialog into account

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

Right ideas, I think there's a few improvements that could be made. I'm giving you r+ since it is test-only code and I think you'll be able to get what I'm intending from the comments. If you want me to take another look at it at it later tonight I can do, alternately I'm sure NiKo will be happy to take a look tomorrow.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +58,5 @@
>            callback(null, []);
>          },
>          on: sandbox.stub()
> +      },
> +      confirm: function(options, callback) {

if you make this a sandbox.stub() instead of a custom function, then below...

@@ +786,5 @@
>        it("should not display a delete button by default", function() {
>          expect(deleteButton).to.not.equal(null);
>        });
>  
>        it("should call the delete function when clicked", function() {

nit: would be good to change this to "should dispatch a delete action when confirmation is granted"

@@ +789,5 @@
>  
>        it("should call the delete function when clicked", function() {
>          sandbox.stub(dispatcher, "dispatch");
>  
> +        confirmResult = true;

you can make this:

navigator.mozLoop.confirm.callsArgWith(1, true);

You could possibly alias navigator.mozLoop.confirm to something like "mozLoopConfirmStub" to be clearer if you want.

@@ +794,3 @@
>          TestUtils.Simulate.click(deleteButton);
>  
> +        expect(confirmCalled).to.equal(1);

I don't think you need this here - if the confirm isn't called, then one of these tests are going to fail.

If you do want to keep it, then I suggest another test:

it("should confirm deletion")

which can check:

sinon.assert.calledOnce(navigator.mozLoop.confirm);

@@ +798,5 @@
>          sinon.assert.calledWithExactly(dispatcher.dispatch,
>            new sharedActions.DeleteRoom({roomToken: roomData.roomToken}));
>        });
> +
> +      it("should cancel the delete action when appropraite", function() {

nit: "should not dispatch an action when the confirmation is cancelled".

@@ +801,5 @@
> +
> +      it("should cancel the delete action when appropraite", function() {
> +        sandbox.stub(dispatcher, "dispatch");
> +
> +        confirmResult = false;

You can obviously do similar changes in this test - just change the parameter.

Comment 16

[Mike de Boer [:mikedeboer]]

Thanks for the ideas!

Pushed to fx-team as:

https://hg.mozilla.org/integration/fx-team/rev/5bd85d90593f
https://hg.mozilla.org/integration/fx-team/rev/ab05a2c29e73

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5bd85d90593f
https://hg.mozilla.org/mozilla-central/rev/ab05a2c29e73

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8529066 [details] [diff] [review]
Patch 1.4: show a modal confirm dialog when a user attempts to delete a room

Approval Request Comment
[Feature/regressing bug #]: Rooms

[User impact if declined]: No confirmation asked for for room delete, and we can't use the normal modal dialog per bug.

[Describe test coverage new/current, TBPL]: Includes tests in patch 2

[Risks and why]: UI issue identified once we started using Rooms.  Relatively simple patch.  

[String/UUID change made/needed]: none

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7365fec8ba5c
https://hg.mozilla.org/releases/mozilla-aurora/rev/84888237ea90

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Verified on Linux (Yes, No, room open or not, etc)

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d8986f09d880
https://hg.mozilla.org/releases/mozilla-beta/rev/2508ac2d3332

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Verified on beta builds as well

Comment 23

[Paul Silaghi, QA [:pauly]]

Verified fixed 37.0a1 (2014-12-11) Win 7.
Also verified FF 36, 35 in comments 20, 22