Closed Bug 1047284 Opened 5 years ago Closed 5 years ago

Update the Loop toolbar icon for error and disabled state changes

Categories

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

defect
Points:
2

Tracking

(firefox34 verified)

VERIFIED FIXED
mozilla34
Iteration:
34.3
Tracking Status
firefox34 --- verified

People

(Reporter: mikedeboer, Assigned: MattN)

References

()

Details

Attachments

(1 file, 2 obsolete files)

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.
Iteration: 34.1 → ---
User Story: (updated)
Moving to FF35 as not necessary for initial release
Priority: P1 → P2
Target Milestone: mozilla34 → mozilla35
Attachment #8467132 - Flags: review?(standard8)
Attachment #8467132 - Flags: review?(standard8)
Attachment #8467132 - Attachment is obsolete: true
Attachment #8467133 - Attachment is obsolete: true
Priority: P2 → P3
Target Milestone: mozilla35 → mozilla34
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)?
Flags: needinfo?(dhenein)
There may be tooltips coming from Arcadio & Co., I would check with him.
Flags: needinfo?(dhenein) → needinfo?(alainez)
Arcadio -- If you want tooltips, would you be ok if they came in Fx35?  (To de-risk Fx34)
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
Flags: needinfo?(alainez)
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
Flags: needinfo?(alainez)
Flags: qe-verify+
Whiteboard: [qa+]
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.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 34.3
Flags: firefox-backlog+
Summary: Update the Loop toolbar icon upon state changes → Update the Loop toolbar icon for error and disabled state changes
QA Contact: anthony.s.hughes
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.
Flags: needinfo?(MattN+bmo)
Whiteboard: [strings]
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.
Attachment #8481370 - Flags: review?(standard8)
Attachment #8481370 - Flags: review?(jaws)
Flags: needinfo?(MattN+bmo) → needinfo?(dhenein)
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.
Flags: needinfo?(dhenein)
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.
Flags: needinfo?(alainez)
Whiteboard: [strings]
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.
Attachment #8481370 - Flags: review?(standard8) → feedback+
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?
Attachment #8481370 - Flags: review?(jaws) → review+
Iteration: 34.3 → 35.1
(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.
Iteration: 35.1 → 34.3
Whiteboard: [fixed-in-fx-team]
Iteration: 34.3 → 35.1
https://hg.mozilla.org/mozilla-central/rev/a7819b6a9411
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Marco, this got resolved today which is still iteration 34.3 (as I understand it) so moving it back.
Iteration: 35.1 → 34.3
I confirm this is working in today's Nightly 34.0a1 build.

Does this have sufficient automation or is a Moztrap test desired?
Status: RESOLVED → VERIFIED
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.
(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.
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.
You need to log in before you can comment on or make changes to this bug.