1000771 - Desktop client needs to let non signed-in user copy a call-back URL

Status: VERIFIED FIXED

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

Comment 1

[:shell escalante]

you can do this with normal copy & paste from OS today, but the new UI has a button in Loop to do the copy to the system clipboard (pre-called, not signed in).  this may be another bug that requires a service on the API, so slightly more complicated than it looks.

Comment 2

[:shell escalante]

can do today using computer copy and paste and our first release users should know how to do this.  pushed to later when we have broader user adoption.

Comment 3

[Romain Testard [:RT]]

I agree although this will break the balance of the UX.
I NeedInfo Darrin for confirmation once he gets back.

Comment 4

[Darrin Henein [:darrin]]

My only concern is that users will not immediately know to select + copy that link. The copy button actually serves two purposes: to actually copy the URL, but also to cue the user that copy/paste is the primary method to transmit the link right now.

If it is indeed a lot of work, it can be pushed, but it is a critical part of the usage path and may affect success rates.

Comment 5

[Romain Testard [:RT]]

Thanks Darrin, good point and I agree we should include this in FF34.
Maire, it looks to be a p=2 delivering good user value - I'm marking it as FF34 P2 - we can discuss on our weekly call tomorrow.

Comment 6

[Paul Silaghi, QA [:pauly]]

The shorter URL (mockup: http://talk.mozilla.org/Jc70K6OH vs nightly: https://call.mozilla.com/#call/0btpBYHGUYw) - is it handled somewhere in some bug?

Comment 7

[Paul Silaghi, QA [:pauly]]

Same question for the lock next to the URL in the mockup

Comment 8

[Romain Testard [:RT]]

(In reply to Paul Silaghi, QA [:pauly] from comment #6)
> The shorter URL (mockup: http://talk.mozilla.org/Jc70K6OH vs nightly:
> https://call.mozilla.com/#call/0btpBYHGUYw) - is it handled somewhere in
> some bug?

It was not, thanks for raising!
I have now created https://bugzilla.mozilla.org/show_bug.cgi?id=1046114 to track the implementation of the final URL format we'll want for Beta launch.
Until beta I suggest we keep https://call.mozilla.com/#call/<token> 
I needInfo Arcadio as he suggested we may want to use https://webrtc.firefox.com/<token> although I'm not sure it makes sense to implement that change given it won't be valid for a long period of time.

Comment 9

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

So for some reason I'm having a hard time to track, the clipboard API doesn't seem to work in Firefox as it's supposed to (see http://caniuse.com/clipboard).

Here's a minimal example reproducing the issue: http://jsbin.com/volej/7/edit

I suggest we go with relying on exposing that feature through the mozLoop API for now. And if so, that's probably something a gecko developer wants to take.

NI mreavy for decision making here.

Comment 10

[Maire Reavy [:mreavy]]

Niko, based on our conversation today after the standup, are you going to pair with someone on this?  (Feel free to still say, "no thanks".)

Comment 11

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

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #10)
> Niko, based on our conversation today after the standup, are you going to
> pair with someone on this?  (Feel free to still say, "no thanks".)

Mike sent me a pastebin (https://pastebin.mozilla.org/5840382) to get started, I should be all fine but will ask for help if needed :)

Comment 12

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

Attachment: Copy Loop call URL.

This patch adds a new "Copy" button in the panel for when a call URL has been generated. It also refactors a little the way the "Email" button was handled, and related tests.

Following a discussion we had with Mike on IRC, the mozLoop.getString method isn't tested, as the gecko clipboard API is already, and we don't do much more than proxying it through the mozLoop object.

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8469207 [details] [diff] [review]
Copy Loop call URL.

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

The approach looks solid, so we're past the feedback stage (would be f+), so I'm going with r-.

::: browser/components/loop/MozLoopAPI.jsm
@@ +227,5 @@
>          });
>        }
>      },
> +
> +    copyString: {

This needs a nice doc-block comment explaining what this function does, like the other API methods.

::: browser/components/loop/content/js/panel.jsx
@@ +263,5 @@
>    var PanelView = React.createClass({
>      propTypes: {
>        notifier: React.PropTypes.object.isRequired,
> +      client: React.PropTypes.object.isRequired,
> +      callUrl: React.PropTypes.string // Mostly used for UI components showcase

nit: put the comment above the addressed line and add 'and unit tests' to it.

::: browser/components/loop/content/shared/css/panel.css
@@ +102,5 @@
> +}
> +
> +.share .action .url-actions .btn:first-child {
> +  margin-right: 1em;
> +}

This will become fun when we start supporting RTL mode. Why not not `-moz-margin-end: 1em` on both buttons?

Also, please try to use child-selectors here.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +320,5 @@
> +              .to.equal(encodeURI(mailto));
> +      });
> +
> +      it("should feature a copy button capable of copying the call url when " +
> +         "clicked", function() {

nit: I don't see value in splitting this string across multiple lines.

Comment 14

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

Attachment: Copy Loop call URL.

Adressed review comments.

Comment 15

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

Attachment: Copy Loop call URL.

Reverted to use -moz-margin-end as discussed with Mike on IRC.

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8469257 [details] [diff] [review]
Copy Loop call URL.

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

Nice, thanks!!

::: browser/components/loop/MozLoopAPI.jsm
@@ +228,5 @@
>        }
>      },
> +
> +    /**
> +     * Copies passed string into end user's clipboard.

nit: can you change this to 'onto the system clipboard' ?

Comment 17

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

Attachment: Copy Loop call URL.

Fixed latest nits.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8469267 [details] [diff] [review]
Copy Loop call URL.

Per irc, we agreed to carry forward the r=mikedeboer

Comment 19

[Mark Banner (:standard8)]

(In reply to Romain Testard [:RT - on PTO back on August 14th] from comment #8)
> It was not, thanks for raising!
> I have now created https://bugzilla.mozilla.org/show_bug.cgi?id=1046114 to
> track the implementation of the final URL format we'll want for Beta launch.
> Until beta I suggest we keep https://call.mozilla.com/#call/<token> 
> I needInfo Arcadio as he suggested we may want to use
> https://webrtc.firefox.com/<token> although I'm not sure it makes sense to
> implement that change given it won't be valid for a long period of time.

This is already dealt with in other bugs, clearing the needinfo request.

Comment 20

[Mark Banner (:standard8)]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[u279076]

Verified fixed in the latest Nightly.