Closed Bug 1000131 Opened 10 years ago Closed 10 years ago

Standalone UI for link clickers needs expired URLs notification.

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
34 Sprint 1- 8/4

People

(Reporter: RT, Unassigned)

References

Details

(Whiteboard: p=?)

User Story

As a WebRTC browser URL clicker but not on Firefox, I get informed that the URL expired as I click it.

Todo:

For the standalone link clicker side only:

- Detect differences of not being able to contact server versus "400 Bad Request"
- In the bad request case, display "Sorry, this link is no longer valid" (get UX review on text).

Attachments

(3 files, 11 obsolete files)

28.31 KB, patch
dmosedale
: review+
Details | Diff | Splinter Review
39.13 KB, image/png
Details
27.35 KB, patch
dmosedale
: review+
Details | Diff | Splinter Review
      No description provided.
User Story: (updated)
User Story: (updated)
Summary: Link clicker UI needs to handle expired URLs → Standalone UI for link clickers needs expired URLs notification.
User Story: (updated)
Depends on: 1000216
Blocks: 1000778
No longer blocks: 994274
User Story: (updated)
Priority: -- → P2
Whiteboard: [p=?, s=UI32]
Target Milestone: --- → mozilla32
Priority: P2 → P3
Target Milestone: mozilla32 → mozilla33
Priority: P3 → P1
User Story: (updated)
Whiteboard: [p=?, s=UI32] → [p=1]
validate verbiage - "link has expired" or "you may want to ask user to resend" or something to send like that.
Flags: needinfo?(dhenein)
I'm leaning towards something more generic, like "This URL is no longer available" or "Oops! There's nothing here!"

This page will show up after expiration, revocation, wrong url/typo (no call ever existed there), etc.

Thoughts?
Flags: needinfo?(dhenein)
More generic seems friendlier.  Perhaps adding some verbiage guiding the user about what to do next would be nice?  e.g. "Try checking with the person who gave you this link to see if they have another"

Dan
Flags: needinfo?(dhenein)
The UI is simple (https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#start-call-unavailable) we just need to decide on the string(s).

Something like "This URL is unavailable" seems most generic (handles all cases best). I like Dan's suggestion but it seems a bit verbose... could we have a "Get Link" button here that just opens the panel and selects the new link text box maybe?
Flags: needinfo?(rtestard)
Flags: needinfo?(dmose)
Flags: needinfo?(dhenein)
This bug is for the standalone UI for link clickers whereas the UI here is for the client UI. Can we have standalone client UI too please?

Not sure I understand the question "could we have a "Get Link" button here that just opens the panel and selects the new link text box maybe?" This UI is for link clickers (i.e not someone who generated the URL) on the Web clicker UI. The Web clicker UI user can not generate URLs.
Flags: needinfo?(rtestard) → needinfo?(dhenein)
Yeah, there are no panels in the standlone UI, and that context doesn't know what channel the link came from.  You're right that my suggested text was verbose.  Maybe "Oops, there's nothing here -- try asking for a new link"?  It's sort of opaque, but I'm not sure how to do better.  

I like the "Oops..." wording you suggested, because it feels like it moves Firefox in the more playful, humane direction described by the design values doc you sent out some time ago.
Flags: needinfo?(dmose)
My bad, was thinking of the desktop client UI. Agreed, this will be simple and will be included in the spec for the standalone app UI.
Flags: needinfo?(dhenein)
One quick comment on this -- we aren't going to be able to tell the difference between an expired URL and a URL that is otherwise invalid (e.g., was damaged during cut-and-paste or was modified by the user). The UX should take this into account.
Assignee: nobody → dhenein
Status: NEW → ASSIGNED
Whiteboard: [p=1] → p=1 s=33.3 [qa-]
Whiteboard: p=1 s=33.3 [qa-] → p=?
(In reply to sescalante from comment #9)
> UI: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired

I don't entirely grasp what to do with the "Download Firefox to make free audio and video calls!" invite and associated "Get Firefox" button; what if the user is already using Firefox, should I hide these?

Also, I'm wondering if I should match the new design in this very bug, or if it should be done in a separated, dedicated bug? The thing is that applying new styles matching this new screen is likely to break other parts, so maybe proceeding with the redesign as one step could be easier to review, and land. Thoughts?
Flags: needinfo?(standard8)
Flags: needinfo?(dmose)
Flags: needinfo?(dhenein)
Assignee: nobody → nperriault
(In reply to Nicolas Perriault (:NiKo`) from comment #10)
> (In reply to sescalante from comment #9)
> > UI: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired
> 
> I don't entirely grasp what to do with the "Download Firefox to make free
> audio and video calls!" invite and associated "Get Firefox" button; what if
> the user is already using Firefox, should I hide these?
> 
Yes I agree, the message prompting the user to download Firefox and the "download" button should be hidden in case the user is on Firefox.


> Also, I'm wondering if I should match the new design in this very bug, or if
> it should be done in a separated, dedicated bug? The thing is that applying
> new styles matching this new screen is likely to break other parts, so maybe
> proceeding with the redesign as one step could be easier to review, and
> land. Thoughts?
(In reply to Nicolas Perriault (:NiKo`) from comment #10)
> Also, I'm wondering if I should match the new design in this very bug, or if
> it should be done in a separated, dedicated bug? The thing is that applying
> new styles matching this new screen is likely to break other parts, so maybe
> proceeding with the redesign as one step could be easier to review, and
> land. Thoughts?

Its hard to know how much that would break, without digging through the css. Maybe if you do it as two separate patches - one for the changes in this bug, one for the redesign, and then maybe that will be easier to see as it moves on, how much it would really break.
Flags: needinfo?(standard8)
So reading the new loop-server code (master and 0.9.0, which is roughly MVP if I'm correct), the format used for errors in served JSON responses has changed recently: https://github.com/mozilla-services/loop-server/blob/77dde7ee23e02bedc402f4212cbb9b3bf8a444b9/loop/utils.js#L9-L29

The Loop desktop client in nightly is still relying on the old format, which is still deployed on production (https://loop.services.mozilla.com/ declares using a 0.5.0). 

I need to update the standalone client code to match the new error format to address this bug, but be aware that it needs to land along with deploying the new loop-server error format to production; and right now, I don't know if we have a plan for that. 

Putting :abr and :tarek as needinfo and will update the related thread on dev-media.
Flags: needinfo?(tarek)
Flags: needinfo?(adam)
Wondering if bug 1000131 should be a blocker for this one.
It seems to be that the server team is deploying both components, so I think we can leave it up to them to ensure that they go live at the same time, so long as we've made the corresponding client changes in time.

Tarek: does that make sense?
Flags: needinfo?(adam)
Flags: needinfo?(dmose)
If by client you mean the code that is in loop-client, yes this can probably happen. Adding Bob and Benson to have a 100% confirmation from them.
Flags: needinfo?(tarek)
Flags: needinfo?(bwong)
Flags: needinfo?(bobm)
Blocks: 1024222
Mark, Niko, can we meet about this tomorrow ? e.g. fixing+tagging loop-client that matches, so it's available for deployment.
OK from discussion w/ Tarek we are going to deploy loop-server 0.10.0 and loop-client 0.1.1 to prod at the same time.
Flags: needinfo?(bwong)
(In reply to Ben Wong [:mostlygeek] from comment #19)
> OK from discussion w/ Tarek we are going to deploy loop-server 0.10.0 and
> loop-client 0.1.1 to prod at the same time.

Clearing my needinfo.
Flags: needinfo?(bobm)
This is the first part of the patch:

- fixed loop-server new JSON error format handling
- new `session:expired` ConversationModel event
- reactified webapp.js, where a new CallUrlExpiredView component has been added
- as loop-server sends 404 - Not Found and INVALID_TOKEN for expired urls, updated notification message to "This call URL is no longer available and has probably expired." (adding :darrin for feedback)

WiP branch matching that patch has been pushed to my gecko-dev fork: bug-1000131-expired-urls

Part 2 will care of webdesign updates.
Attachment #8458594 - Flags: review?(dmose)
Attachment #8458594 - Flags: feedback?(dhenein)
Comment on attachment 8458594 [details] [diff] [review]
Part 1: fixed server json error handling, expired urls notification

r=dmose, once the comments we discussed over video are addressed
Attachment #8458594 - Flags: review?(dmose)
Attachment #8458594 - Flags: review+
Attachment #8458594 - Flags: feedback?(dhenein)
Comment on attachment 8459146 [details] [diff] [review]
Part 1: fixed server json error handling, expired urls notification - rev2

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

I found a problem, unfortunately, which needs some work, so I need to r- until this gets fixed.

::: browser/components/loop/content/shared/js/models.js
@@ +214,5 @@
> +     *
> +     * @param  {Error} err Error object.
> +     */
> +    _handleServerError: function(err) {
> +      var jsonErr = err.jsonErr || {};

The problem with this initialization is that if we hit the {} clause, the switch statement will throw an exception trying to access an undefined property.

This clause can actually hit if the catch clause in standaloneClient.js:requestCallInfo's req.done gets invoked.  Additionally, it will also be invoked to handle errors from callbacks from requestCall{s,}Info from client.js, and those functions have a bunch of cases that don't inject jsonErr.

So I think we probably want to declare that this method supports handling Error objects without .jsonErr properties, update the docs, write a test for that, and make it pass.

Also, boy do we want integration testing.  :-)
Attachment #8459146 - Flags: review?(dmose) → review-
Addressed latest comments.
Attachment #8459146 - Attachment is obsolete: true
Attachment #8459356 - Flags: review?(dmose)
(In reply to Romain Testard [:RT] from comment #11)
> (In reply to Nicolas Perriault (:NiKo`) from comment #10)
> > (In reply to sescalante from comment #9)
> > > UI: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired
> > 
> > I don't entirely grasp what to do with the "Download Firefox to make free
> > audio and video calls!" invite and associated "Get Firefox" button; what if
> > the user is already using Firefox, should I hide these?
> > 
> Yes I agree, the message prompting the user to download Firefox and the
> "download" button should be hidden in case the user is on Firefox.
> 
Please note the amended design:
Non Firefox link clicker: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired
Firefox link clicker: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired-ffx
(In reply to Romain Testard [:RT] from comment #26)
> Please note the amended design:
> Non Firefox link clicker:
> https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired
> Firefox link clicker:
> https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired-ffx

Thanks. Now I think about it, shouldn't we use the Hello logo instead of the Firefox one for the FF version? Maybe even in both cases actually.
Comment on attachment 8459356 [details] [diff] [review]
0001-Bug-1000131-Expired-Loop-call-url-notification-part1-rev3.patch

This patch is broken. I need to find a better strategy for dealing with server error identifiers.
Attachment #8459356 - Attachment is obsolete: true
Attachment #8459356 - Flags: review?(dmose)
Fixed previous patch with a simpler approach, attaching only the `errno` code to the Error object.

What we really want in the future is a typed Error object for server errors, eg. ServerError, which should be shared across all clients. Though this is probably out of the scope of this bug, hence why I'll create a dedicated bug once this patch is validated.
Attachment #8459471 - Flags: review?(dmose)
(In reply to Nicolas Perriault (:NiKo`) from comment #27)
> (In reply to Romain Testard [:RT] from comment #26)
> > Please note the amended design:
> > Non Firefox link clicker:
> > https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired
> > Firefox link clicker:
> > https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired-ffx
> 
> Thanks. Now I think about it, shouldn't we use the Hello logo instead of the
> Firefox one for the FF version? Maybe even in both cases actually.

I have questions regarding logos pending with Arcadio (a branding discussion really). I also question if we should use the Mozilla logo at all given it may cause confusion for users who will know the product as Firefox Hello.
I needInfo Arcadio who we'll be discussing this with tonight so hoping to have an answer later today.
Flags: needinfo?(alainez)
Attachment #8459471 - Attachment is obsolete: true
Attachment #8459471 - Flags: review?(dmose)
Attachment #8459747 - Attachment is obsolete: true
Attachment #8459751 - Attachment is obsolete: true
Attachment #8459775 - Attachment description: Expired Loop call url notification, r=dmose → [checked-in] Expired Loop call url notification, r=dmose
Attachment #8459775 - Flags: review+
(In reply to Romain Testard [:RT] from comment #30)
> I needInfo Arcadio who we'll be discussing this with tonight so hoping to
> have an answer later today.

Any news? Also, provided mockups (https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired) uses an image for the Hello Logo, and a retina version isn't provided. So I'd like to know:

- if any branding issue so this is blocked,
- or if this is fine, who should I ask a retina version for — :darrin being currently on PTO.
decision to use Loop in aurora - and Hello in Fx34.  won't go to beta.
Flags: needinfo?(dhenein)
Flags: needinfo?(alainez)
Target Milestone: mozilla33 → 34 Sprint 1- 8/4
This is the second part of the patch, bringing styling to the view previously landed. Fixed a bunch of broken common styles and fixed the readme.html file accordingly.
Attachment #8460320 - Flags: review?(dmose)
Comment on attachment 8460320 [details] [diff] [review]
Part 2: updating expired url view to match the styles in the mockups

r=dmose with the tweaks suggested in the review at <https://github.com/n1k0/gecko-dev/commit/37d37ce9605807b7e8dd31f7d40030717c2ce538#diff-f67d8232f05108a1c64b104ae33eca6bR170>.
Attachment #8460320 - Flags: review?(dmose) → review+
(In reply to Nicolas Perriault (:NiKo`) from comment #36)

> Also, provided mockups
> (https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired) uses
> an image for the Hello Logo, and a retina version isn't provided. So I'd
> like to know:
> 
> - if any branding issue so this is blocked,
> - or if this is fine, who should I ask a retina version for — :darrin being
> currently on PTO.

mmaslaney  should be able to provide a retina version once he receives the OK from branding.
Comment on attachment 8460324 [details]
Part 2: screen capture for UX review

Hi NiKo, that's looking good.

The spec, however, has a max-width of 400px and the text where it says "Oops" and "The URL is no longer available" are darker in the spec.

Oops should be #666 and the URL string should be #AAA.

Also, missing the logo, but you mentioned why above.
Attachment #8460324 - Flags: ui-review?(sfranks)
Copy question for Matej...

:dmose brought to my attention the string underneath the "Oops" box which says "Download Firefox to make free audio and video calls!"

He brought up the valid point that Firefox Hello will not be in wide versions of Firefox for a bit, so the string is not entirely accurate as this stage (even misleading).

a) Is this a concern?
b) If so, could you provide an alternate string to entice users to download Firefox (not necessarily Loop related...this could just be a promotional spot)?

Here is the spec for the page I am referring to: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired
Flags: needinfo?(Mnovak)
Can I get a little more info about when and where this will show up? If it's an error message for Firefox Hello, wouldn't it already be available and in use? In that case the string wouldn't be inaccurate, would it?

I'm actually more concerned about the exclamation mark after Firefox Hello at the top. Is that deliberate?

Thanks.
Flags: needinfo?(Mnovak)
(In reply to Matej Novak [:matej] from comment #44)
> Can I get a little more info about when and where this will show up? If it's
> an error message for Firefox Hello, wouldn't it already be available and in
> use? In that case the string wouldn't be inaccurate, would it?
> 
> I'm actually more concerned about the exclamation mark after Firefox Hello
> at the top. Is that deliberate?
> 
> Thanks.

Hey Matej,

If a call link is no longer valid, users of Firefox see this page which basically tells them the URL is unavailable: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired-ffx

For users using other browsers, this is a good spot to entice them to download Firefox, so at the footer of the page we have the aforementioned string and a button which takes the user to the download Firefox page: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired

Hello is only available in Nightly/Aurora and not our release version of Firefox...so if someone sees that ad when clicking on a link through Chrome, they may think "Oh, I can make calls too!" and download Firefox only to find the functionality is not available.

So until Hello is available in all versions, maybe there is some other way we can entice users of other browsers to give Firefox a try.
(In reply to Matej Novak [:matej] from comment #44)
 
> I'm actually more concerned about the exclamation mark after Firefox Hello
> at the top. Is that deliberate?


As for the exclamation mark, I am not sure. Arcadio Lainez (alainez) has been handling the branding and might be able to chime in on whether it's "Firefox Hello" or "Firefox Hello!"
Flags: needinfo?(alainez)
Thanks for the clarification. I love the idea of getting people to download Firefox from this page with the hook of being able to make free calls. It's a clear, easy fit.

I worry about doing it before Hello is available in all versions, though. Telling them the link is unavailable, then asking them to download the browser for an unrelated reason feels like a bait and switch. It may not send the right message or give them the best first impression of Firefox.

Can we remove it until Hello lands in GA?

As for the exclamation mark, that definitely isn't right. The wordmark lockup doesn't look right either (I don't think it's the right font). If formatted that way, it should be an image and use Meta. Otherwise it should be live text, but then not using different font weights for Firefox and Hello.

I'm adding John Slater who can comment further on the branding.
(In reply to Matej Novak [:matej] from comment #47)
> As for the exclamation mark, that definitely isn't right. The wordmark
> lockup doesn't look right either (I don't think it's the right font). If
> formatted that way, it should be an image and use Meta. Otherwise it should
> be live text, but then not using different font weights for Firefox and
> Hello.

Thanks Matej. I definitely agree with the above comments...we shouldn't be using the exclamation point with this, and the wordmark formatting looks off. There are wordmark guidelines at the bottom of this page that will help: https://www.mozilla.org/en-US/styleguide/communications/typefaces/. Alternately, my team can put something together if we need it.

Beyond that, if there are branding decisions being made about how we represent Hello in the product (wordmark or no wordmark, etc) then I'd like Matej and I to be in the loop on those.

Thanks!
(In reply to Sevaan Franks [:sevaan] from comment #46)
> (In reply to Matej Novak [:matej] from comment #44)
>  
> > I'm actually more concerned about the exclamation mark after Firefox Hello
> > at the top. Is that deliberate?
> 
> 
> As for the exclamation mark, I am not sure. Arcadio Lainez (alainez) has
> been handling the branding and might be able to chime in on whether it's
> "Firefox Hello" or "Firefox Hello!"

Please note that as per discussion with Arcadio yesterday we should not be using the "Firefox Hello" brand for now (likely we'll use it when we get to beta). For Nightly and Aurora we should replace "Hello" by "WebRTC" wherever "Hello" appears in the UX and also use the Firefox logo instead of the speech bubble for now.
Updated patch with :dmose and :sevaan comments addressed. No app logo is part of this patch, so there will probably be a Part 3 (or another bug, maybe)… but I'd really like this to land because it really improves the current app look and feel :)
Attachment #8460320 - Attachment is obsolete: true
Attachment #8460806 - Flags: review?(dmose)
(In reply to Romain Testard [:RT] from comment #49)
> (In reply to Sevaan Franks [:sevaan] from comment #46)
> > (In reply to Matej Novak [:matej] from comment #44)
> >  
> > > I'm actually more concerned about the exclamation mark after Firefox Hello
> > > at the top. Is that deliberate?
> > 
> > 
> > As for the exclamation mark, I am not sure. Arcadio Lainez (alainez) has
> > been handling the branding and might be able to chime in on whether it's
> > "Firefox Hello" or "Firefox Hello!"
> 
> Please note that as per discussion with Arcadio yesterday we should not be
> using the "Firefox Hello" brand for now (likely we'll use it when we get to
> beta). For Nightly and Aurora we should replace "Hello" by "WebRTC" wherever
> "Hello" appears in the UX and also use the Firefox logo instead of the
> speech bubble for now.

In that case "Firefox WebRTC" should all be live text and in the same font weight so it doesn't look like a wordmark.

Thanks.
Regarding the string about making free calls...

(In reply to Matej Novak [:matej] from comment #47) 
> Can we remove it until Hello lands in GA?

Yes. Let's remove the string from that page for now, however, we'll keep the "Get Firefox" button.
Regarding the string about making free calls...

(In reply to Matej Novak [:matej] from comment #47) 
> Can we remove it until Hello lands in GA?

Yes. Let's remove the string from that page for now but we'll still keep the "Get Firefox" button.
Comment on attachment 8460806 [details] [diff] [review]
Part 2: updating expired url view to match the styles in the mockups (rev2)

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

r=dmose, with the changes requested by Sevaan and Matej.
Attachment #8460806 - Flags: review?(dmose) → review+
Refreshing patch for testing moz-git-tools git-bz command.
Attachment #8460806 - Attachment is obsolete: true
Rebased patch against latest master.
Attachment #8463294 - Attachment is obsolete: true
Rebased patch against real upstream master (sorry for the noise).
Attachment #8463353 - Attachment is obsolete: true
Attachment #8463559 - Flags: review?(dmose) → review+
https://hg.mozilla.org/mozilla-central/rev/e566ade6f17e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to Sevaan Franks [:sevaan] from comment #45)
> If a call link is no longer valid, users of Firefox see this page which
> basically tells them the URL is unavailable:
> https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired-ffx
Does https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#start-call-unavailable still stands for other cases?
If not, how can the link-expired be triggered (tested) ?
Flags: needinfo?(rtestard)
(In reply to Paul Silaghi, QA [:pauly] from comment #62)
> (In reply to Sevaan Franks [:sevaan] from comment #45)
> > If a call link is no longer valid, users of Firefox see this page which
> > basically tells them the URL is unavailable:
> > https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired-ffx
> Does
> https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#start-call-
> unavailable still stands for other cases?
> If not, how can the link-expired be triggered (tested) ?

Yes, there was no bug and I just created on to track the implementation of it: https://bugzilla.mozilla.org/show_bug.cgi?id=1047399
Flags: needinfo?(rtestard)
This landed without the branding, which will be handled by a different bug. Hence, removing the need info request from comment 46.
Flags: needinfo?(alainez)
(In reply to Romain Testard [:RT] from comment #63)
> (In reply to Paul Silaghi, QA [:pauly] from comment #62)
> > (In reply to Sevaan Franks [:sevaan] from comment #45)
> > > If a call link is no longer valid, users of Firefox see this page which
> > > basically tells them the URL is unavailable:
> > > https://people.mozilla.org/~dhenein/labs/loop-link-spec/#link-expired-ffx
> > Does
> > https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#start-call-
> > unavailable still stands for other cases?
> > If not, how can the link-expired be triggered (tested) ?
> 
> Yes, there was no bug and I just created on to track the implementation of
> it: https://bugzilla.mozilla.org/show_bug.cgi?id=1047399

Adding dependency since this was duped. Let's move follow-up testing to bug 1000202.
Status: RESOLVED → VERIFIED
Depends on: 1000202
QA Contact: paul.silaghi
Whiteboard: p=? → p=? [qa+]
No longer depends on: 1000202
Flags: qe-verify+
Whiteboard: p=? [qa+] → p=?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: