Closed Bug 1387042 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Status
firefox57 --- fixed

People

(Reporter: ntim, Assigned: jaws)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-animation])

Attachments

(3 files, 1 obsolete file)

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
Blocks: 1352697
No longer blocks: photon-structure
Summary: When adding "Copy link" to the address bar, "Copy link" doesn't have any feedback when the link has been copied → [UX] When adding "Copy link" to the address bar, "Copy link" doesn't have any feedback when the link has been copied
Whiteboard: [photon-structure][triage][photon-animation][triage]
Bryan / Stephen, what do you think we should do here?
Flags: needinfo?(shorlander)
Flags: needinfo?(bbell)
I think it would help if the icons had a hover/active state like described in the spec. Right now, the button looks disabled.
Aaron had some ideas for this.
Flags: needinfo?(shorlander) → needinfo?(abenson)
Flags: needinfo?(bbell)
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.
Whiteboard: [photon-structure][triage][photon-animation][triage] → [photon-animation][triage]
Flags: qe-verify-
Priority: -- → P3
Whiteboard: [photon-animation][triage] → [reserve-photon-animation]
Assignee: nobody → amlee
Status: NEW → ASSIGNED
Iteration: --- → 57.1 - Aug 15
Priority: P3 → P1
Whiteboard: [reserve-photon-animation] → [reserve-photon-animation] [ux]
Iteration: 57.1 - Aug 15 → 57.2 - Aug 29
Link to spec for Copy URL confirmation message 

https://mozilla.invisionapp.com/share/CFD5CY9KW
Flags: needinfo?(amlee)
Assignee: amlee → jaws
Attached video Copy URL .mp4 (obsolete) —
Flags: qe-verify- → qe-verify?
Summary: [UX] When adding "Copy link" to the address bar, "Copy link" doesn't have any feedback when the link has been copied → When adding "Copy link" to the address bar, "Copy link" doesn't have any feedback when the link has been copied
Whiteboard: [reserve-photon-animation] [ux] → [reserve-photon-animation]
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Flags: qe-verify? → qe-verify+
Priority: P1 → P3
QA Contact: stefan.georgiev
Iteration: 57.2 - Aug 29 → ---
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attached image checkmark.svg
Attachment #8899910 - Attachment is obsolete: true
Iteration: --- → 57.2 - Aug 29
Priority: P3 → P1
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.
Flags: needinfo?(dbolter)
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?
Attachment #8900918 - Flags: review?(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/#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
Attachment #8900918 - Flags: review?(sfoster)
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.
Thanks! MarcoZ can you try the build in comment 12?
Flags: needinfo?(dbolter) → needinfo?(mzehe)
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 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.
Attachment #8900918 - Flags: review?(adw) → review+
(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 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
Attachment #8900918 - Flags: review?(sfoster) → review+
Depends on: 1394575
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.
Pushed by jwein@mozilla.com:
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
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
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.
Flags: needinfo?(mzehe)
https://hg.mozilla.org/mozilla-central/rev/3417ead48c7f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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
QA Whiteboard: [bugday-20170830]
Blocks: 1395154
Status: RESOLVED → VERIFIED
Flags: qe-verify+
I can also verify that screen readers speak the message "Copied" when the URL has been copied.
No longer depends on: 1395178
Depends on: 1395398
Depends on: 1395410
Depends on: 1395194
See Also: → 1398681
See Also: 1398681
Depends on: 1411978
Depends on: 1471558
No longer depends on: 1471558
You need to log in before you can comment on or make changes to this bug.