1047284 - Update the Loop toolbar icon for error and disabled state changes

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

To show a different icon, set the `state` attribute on the #loop-call-button to one of the following values, as defined in the Loop MVP spec (https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#toolbar) :

 - 'active': in a call, outgoing call, FxA verified.
 - 'error': connection error, authentication error.
 - 'action': incoming call.

To trigger the unavailable state (offline/ DND), simply set the `disabled` attribute on the button to 'true'.
Since this will make the button truly disabled (a click will do nothing) this probably needs to change to a 'disabled' value for the `state` attribute.

Comment 1

[Romain Testard [:RT]]

Moving to FF35 as not necessary for initial release

Comment 2

[Romain Gauthier [:tOkeshu]]

Attachment: Add a button to send url by email

Comment 3

[Romain Gauthier [:tOkeshu]]

Attachment: share-email-button4.png

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8467132 [details] [diff] [review]
Add a button to send url by email

This patch appears to be on the wrong bug. Please can you also request review from NiKo on this patch (the email one), and extend the l10n note to explain what \r\n are.

Comment 5

[Maire Reavy [:mreavy]]

Darrin, Do we need any strings for this bug?  I'm thinking specifically about tooltips.  Or can we go just with icons (especially for Fx34)?

Comment 6

[Darrin Henein [:darrin]]

There may be tooltips coming from Arcadio & Co., I would check with him.

Comment 7

[Maire Reavy [:mreavy]]

Arcadio -- If you want tooltips, would you be ok if they came in Fx35?  (To de-risk Fx34)

Comment 8

[alainez]

I'm generally OK with de-risking 34. I think we need tooltips for users that are not aware of the feature/icon but if the icon is going to be in the customize menu as default, the person that moved the icon to the toolbar will already know that it's for starting a video conversation. 

So I am ok if it unblocks something on your side but we do have a string ready to present shortly. 

Let me know if that helps. 

arcadio

Comment 9

[Matthew N. [:MattN]]

The bug is specifically about tooltips to explain the different toolbar button colors/states. That may mean something like a suffix or replacement for the default tooltip.

See https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#toolbar

Comment 10

[Matthew N. [:MattN]]

I'm taking this now for the error and disabled states to lay the foundation for UI listening for observer notifications so I can build upon it to let the panel know about state changes for bug 1047144.

Comment 11

[Maire Reavy [:mreavy]]

Matt -- Do you have the info you need to pre-land this for strings?  I added {strings] in the whiteboard so I think all the FxA bugs that have strings are now marked.

It looks like bug 1047181 won't need string changes.

Comment 12

[Matthew N. [:MattN]]

Attachment: v.1 Error and DND

It's still not clear whether the tooltip should change for the error or DND states so I'm not doing that for now. I've asked Darrin about it on IRC in #ux.

Comment 13

[Darrin Henein [:darrin]]

I don't think we need specific tooltips based on the icon state. The idea is to use the icon state to prompt/notify the user that there is something in the panel worth checking out -- we don't want/need to use a tooltip to further communicate that.

Comment 14

[Matthew N. [:MattN]]

OK, thanks. I don't think there are other strings needed for this bug then. If the existing Loop tooltip needs to change then that's a separate bug which should land before Tuesday.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8481370 [details] [diff] [review]
v.1 Error and DND

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

This looks reasonable to me. I'm a bit concerned the UI may need to have its own error tracking state as well, but I think that's something we can resolve as we improve the notifications there.

Only giving f+ as I've not looked at this in detail. Hopefully Jaws can.

::: browser/themes/osx/browser.css
@@ +1326,1 @@
>    #loop-call-button[disabled="true"] > .toolbarbutton-badge-container {

If we're not using disabled=true, it might be an idea to remove these old ones.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8481370 [details] [diff] [review]
v.1 Error and DND

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

::: browser/components/loop/MozLoopService.jsm
@@ +281,5 @@
>          // early return.
>          if (!this.storeSessionToken(response.headers))
>            return;
>  
> +        this.clearError("registration");

Should we instead clear the error at the start of the function? Before the promise has been resolved/rejected? Doing so would clear any visible error state when the registration retries, whereas this patch will keep it in error state until success has been determined. I'm not sure what is best.

What do you think? (I personally would prefer that we clear error state earlier, but if it happens really quickly we wouldn't want flickering of the icon.)

@@ +303,5 @@
>          }
>  
>          // XXX Bubble the precise details up to the UI somehow (bug 1013248).
>          Cu.reportError("Failed to register with the loop server. error: " + error);
> +        this.setError("registration", error);

Is `registration` the only "error" that is implemented so far?

Comment 17

[Matthew N. [:MattN]]

(In reply to Mark Banner (:standard8) from comment #15)
> If we're not using disabled=true, it might be an idea to remove these old
> ones.

I think it's still needed for when buttons are disabled by other parties e.g. customization mode.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> Review of attachment 8481370 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/MozLoopService.jsm
> @@ +281,5 @@
> >          // early return.
> >          if (!this.storeSessionToken(response.headers))
> >            return;
> >  
> > +        this.clearError("registration");
> 
> Should we instead clear the error at the start of the function? Before the
> promise has been resolved/rejected? Doing so would clear any visible error
> state when the registration retries, whereas this patch will keep it in
> error state until success has been determined. I'm not sure what is best.
> 
> What do you think? (I personally would prefer that we clear error state
> earlier, but if it happens really quickly we wouldn't want flickering of the
> icon.)

I was trying to avoid the flickering as it's how I thought I've seen errors handled in other applications. I think ideally there should be a separate progress/activity indicator for that gives feedback as soon as a retry button is clicked and the error message and red icon should stay visible. I think we can refine this.

> @@ +303,5 @@
> >          }
> >  
> >          // XXX Bubble the precise details up to the UI somehow (bug 1013248).
> >          Cu.reportError("Failed to register with the loop server. error: " + error);
> > +        this.setError("registration", error);
> 
> Is `registration` the only "error" that is implemented so far?

Yes, bug 1047164 will add FxA error messages and others should probably be added too as we find the need.

Pushed: https://hg.mozilla.org/integration/fx-team/rev/a7819b6a9411

QA verification instructions:
* Change the pref loop.server to an invalid server so it gets a 404 e.g. http://loop.dev.lcip.org/v-1 and then restart and open the loop panel. The Loop icon should turn red.

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a7819b6a9411

Comment 19

[Matthew N. [:MattN]]

Marco, this got resolved today which is still iteration 34.3 (as I understand it) so moving it back.

Comment 20

[u279076]

I confirm this is working in today's Nightly 34.0a1 build.

Does this have sufficient automation or is a Moztrap test desired?

Comment 21

[u279076]

PS

Just confirming this bug report has changed to only cover 404 error state. I assume other types of errors and the various states mentioned in comment 0 are going to be covered in their own bugs. Please confirm if that's a correct assumption.

Comment 22

[Matthew N. [:MattN]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #20)
> I confirm this is working in today's Nightly 34.0a1 build.
> 
> Does this have sufficient automation or is a Moztrap test desired?

It has automation to test that the icon changes with the proper setError calls.

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #21)
> PS
> 
> Just confirming this bug report has changed to only cover 404 error state. I
> assume other types of errors and the various states mentioned in comment 0
> are going to be covered in their own bugs. Please confirm if that's a
> correct assumption.

Sorry for the confusion, it's not just 404s but that's one of the types of errors during registration with the Loop server that is caught.

I should have also mentioned to test the "disabled" state by setting availability to "do not disturb" and checking that the button becomes grey.

Comment 23

[u279076]

Okay, thanks Matt. I'll make sure we have a Moztrap smoketest for "do not disturb" and will rely on the automation for further testing of Loop server errors. Please needinfo me to request testing above and beyond that.