1387042 - When adding "Copy link" to the address bar, "Copy link" doesn't have any feedback when the link has been copied

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Tim Nguyen :ntim]

STR:
- Open page action menu
- Right click on "Copy Link"
- Click on "Add to address bar"
- Click on the "Copy Link" button in the address bar.


AR:
You have absolutely no indication that the link has been copied.

ER:
I would expect an animation, or a panel saying the link has been copied

Comment 1

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

Bryan / Stephen, what do you think we should do here?

Comment 2

[Tim Nguyen :ntim]

I think it would help if the icons had a hover/active state like described in the spec. Right now, the button looks disabled.

Comment 3

[Stephen Horlander [:shorlander]]

Aaron had some ideas for this.

Comment 4

[Aaron Benson]

Yeah, I think the three dots should have a simple animation where they morph (somehow) into a checkmark .. we could even give it the same "pop" as the Bookmarking icon when it turns into the checkmark. I think we just need a simple touch to show that FF did the thing you asked it to do. We had discussed maybe doing a toast-style notification here but I tend to think that's overkill since what you really want is a simple reassurance that your action was "processed". 

Amy, want to work up some animations around this concept?

Comment 5

[Aaron Benson]

In thinking about this some more, the animation probably shouldn't be dependent on the three dots since the "Copy URL" action can be placed inside the URL bar (which was the point of this bug but I misread it a little). In my comments above I was only thinking of how it would behave if you clicked on "Copy URL" from the Page Action Menu.

In either case, a simple animation with a checkmark to indicate 'task completed' would be my recommendation .. and working in the same "pop" effect that the bookmark icon has would be a nice way to carry on that animation element.

Comment 6

[Amy Lee [:amylee] UX]

Link to spec for Copy URL confirmation message 

https://mozilla.invisionapp.com/share/CFD5CY9KW

Comment 7

[Amy Lee [:amylee] UX]

Attachment: Copy URL .mp4

Comment 8

[Amy Lee [:amylee] UX]

Attachment: checkmark.svg

Comment 9

[Amy Lee [:amylee] UX]

Attachment: Icon - Copied URL.mp4

Comment 10

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

Attachment: Bug 1387042 - Add a toast notification that will be shown when the Copy Link or Send to Device page actions are used.

Review commit: https://reviewboard.mozilla.org/r/172354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172354/

Comment 11

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

Comment on attachment 8900918 [details]
Bug 1387042 - Add a toast notification that will be shown when the Copy Link or Send to Device page actions are used.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172354/diff/1-2/

Comment 12

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

Hey David, in this bug we're adding a transient popup notification next to the "Copy URL" button in the location bar. The popup notification will say "Copied!" when the user clicks on and the current page's URL is put in their clipboard.

The popup has role="alert" on it so as I understand it screenreaders should handle it correctly. The popup will autoclose on its own or can close if the user clicks elsewhere. Right now we have it implemented to close after 1 second of being open since the alert text is so terse. Is there a general guideline of how much time is acceptable for users to have enough time to read something like this?

If you would like to try it out, here's a link to a trypush:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cebc19ea381bc70db7a4a6423a78678f153b3ac2&selectedJob=125762766

You can make the popup stay open longer by adding an Integer pref to about:config with the name of "browser.pageActions.feedbackTimeoutMS", setting the value to the number of milliseconds the popup should stay open. It defaults to 1120ms if the pref doesn't exist.

Comment 13

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

Comment on attachment 8900918 [details]
Bug 1387042 - Add a toast notification that will be shown when the Copy Link or Send to Device page actions are used.

Hey Drew, can you please review the changes to Page Actions?

Comment 14

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8900918 [details]
Bug 1387042 - Add a toast notification that will be shown when the Copy Link or Send to Device page actions are used.

https://reviewboard.mozilla.org/r/172354/#review178140

I'll attach a screenshot of the popup on Ubuntu. I'm not sure what is going there - possibly an artifact of the larger default font-size there, or Ubuntu not knowing how to draw a popup that small? Otherwise, with the comments below this looks good.

::: browser/base/content/browser-pageActions.js:723
(Diff revision 2)
> +      this.feedbackAnimationBox.removeAttribute("animate");
> +    }, {once: true});
> +
> +    // The timeout value used here allows the panel to stay open for
> +    // 1 second after the text transition (duration=120ms) has finished.
> +    setTimeout(() => {

Maybe use requestAnimationFrame inside the setTimeout callback here? See thread around https://bugzilla.mozilla.org/show_bug.cgi?id=1388183#c13

::: browser/themes/shared/icons/check-animation.svg:7
(Diff revision 2)
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="266" height="14">
> +  <svg>
> +    <defs>
> +      <mask id="a">

Can you file a follow-up bug optimize this SVG? 
Looks like some of these masks use the same path, and I think we may be able to bake the translate adjustments into the paths for less runtime computation

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8900918 [details]
Bug 1387042 - Add a toast notification that will be shown when the Copy Link or Send to Device page actions are used.

https://reviewboard.mozilla.org/r/172354/#review178172

::: browser/themes/shared/icons/check-animation.svg:5
(Diff revision 2)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="266" height="14">
> +  <svg>

This first element is 100% masked, so you can just remove it.

::: browser/themes/shared/icons/check-animation.svg:157
(Diff revision 2)
> +        <path fill="#fff" stroke="#fff" stroke-width="30" d="M-2.91 5.508v7.07h4.488v-7.07H-2.91"/>
> +      </mask>
> +    </defs>
> +    <path fill="#FFF" d="M6 14a.997.997 0 0 1-.707-.293l-3-3a1 1 0 0 1 1.414-1.414l2.157 2.157 6.316-9.023a1 1 0 0 1 1.64 1.146l-7 10a1 1 0 0 1-.733.427A1.262 1.262 0 0 1 6 14z" mask="url(#s)" transform="translate(-.94 -.94)"/>
> +  </svg>
> +  <svg x="266">

This element is offscreen from the SVG, you can remove it.

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks! MarcoZ can you try the build in comment 12?

Comment 17

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

Comment on attachment 8900918 [details]
Bug 1387042 - Add a toast notification that will be shown when the Copy Link or Send to Device page actions are used.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172354/diff/2-3/

Comment 18

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

Comment on attachment 8900918 [details]
Bug 1387042 - Add a toast notification that will be shown when the Copy Link or Send to Device page actions are used.

https://reviewboard.mozilla.org/r/172354/#review178140

> Maybe use requestAnimationFrame inside the setTimeout callback here? See thread around https://bugzilla.mozilla.org/show_bug.cgi?id=1388183#c13

I read through the logs since that comment doesn't explain enough of the conversation. This doesn't seem related to what we're doing here which is just hiding the popup. The discussion that this was about was dealing with back to back animations, and making sure there is a style flush between them, which this doesn't really need to care about.

Comment 19

[Drew Willcoxon :adw]

Comment on attachment 8900918 [details]
Bug 1387042 - Add a toast notification that will be shown when the Copy Link or Send to Device page actions are used.

https://reviewboard.mozilla.org/r/172354/#review178262

Nice!  Do you think this could be extended to support other animated feedback, like we have on the star and Pocket buttons, kind of like we discussed in IRC the other day?  Just curious if you thought about that, no worries.

::: browser/base/content/browser-pageActions.js:701
(Diff revision 3)
> +  show(action, event) {
> +    this.feedbackLabel.textContent = this.panelNode.getAttribute(action + "Feedback");
> +    this.panelNode.hidden = false;
> +
> +    let anchor = BrowserPageActions.mainButtonNode;
> +    if (event.target.classList.contains("urlbar-icon")) {

Will this be OK for actions with wrappers?  I'm sure you've thought of that, just want to double check.

Comment 20

[Sam Foster [:sfoster] (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> > Maybe use requestAnimationFrame inside the setTimeout callback here? See thread around https://bugzilla.mozilla.org/show_bug.cgi?id=1388183#c13
> 
> I read through the logs since that comment doesn't explain enough of the
> conversation. This doesn't seem related to what we're doing here which is
> just hiding the popup. The discussion that this was about was dealing with
> back to back animations, and making sure there is a style flush between
> them, which this doesn't really need to care about.

My understanding is that the point of the requestAnimationFrame is that setTimeout is a blunt tool for scheduling operations which will touch the DOM, cause reflow and paint etc. rAF allows this to be scheduled for a more optimal time. I'm not sure how applicable this is here though, and its a easy fix if it turns out to be a good idea

Comment 21

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8900918 [details]
Bug 1387042 - Add a toast notification that will be shown when the Copy Link or Send to Device page actions are used.

https://reviewboard.mozilla.org/r/172354/#review178388

Looks good!

::: browser/themes/linux/customizableui/panelUI.css:96
(Diff revision 3)
>  
> +/*
> + * #pageActionFeedbackAnimatableImage is wider than the panel, and due to a
> + * bug in panels on Linux, a box-shadow appears where the image would be if
> + * overflow:hidden wasn't applied. Disabling the box-shadow for this panel on
> + * Linux works around this issue. This bug is on file as XXXXXX.

Nit: update comment with the bug number

Comment 22

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

Comment on attachment 8900918 [details]
Bug 1387042 - Add a toast notification that will be shown when the Copy Link or Send to Device page actions are used.

https://reviewboard.mozilla.org/r/172354/#review178262

> Will this be OK for actions with wrappers?  I'm sure you've thought of that, just want to double check.

Yes, I tested this with the bookmark star and the event target is the `.urlbar-icon`. However if the star is already filled in then the event.target is the #star-button-animatable-image and so it doesn't have the `urlbar-icon` class. Right now we only want to support this for the Copy URL and Send Page to Device, but we could extend this to other page action buttons easily without too much extra work.

Comment 23

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

Comment on attachment 8900918 [details]
Bug 1387042 - Add a toast notification that will be shown when the Copy Link or Send to Device page actions are used.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172354/diff/3-4/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3417ead48c7f
Add a toast notification that will be shown when the Copy Link or Send to Device page actions are used. r=adw,sfoster

Comment 25

[Marco Zehe (:MarcoZ)]

Will test this in Nightly when it lands there, and file a followup if necessary. The try builds didn't appear for me for Windows for some reason. Role="alert" should normally do the trick just fine. It's an implicit aria-live="assertive" and is normally picked up by screen readers, like with doorhangers. Clearing NI.

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3417ead48c7f

Comment 27

[Azmina]

I have reproduced this bug with Nightly 57.0a1 (2017-08-03) on Windows 8.1, 64 Bit!

This bug's fix is verified on Latest Nightly 57.0a1

Build ID : 20170830100230
User Agent : Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 28

[Marco Zehe (:MarcoZ)]

I can also verify that screen readers speak the message "Copied" when the URL has been copied.