WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 9 days ago

Last modified 5 days ago

#48463 closed defect (bug) (fixed)

Copy link button in media

Reported by: theolg Owned by: afercia
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.2
Component: Media Keywords: has-screenshots has-patch commit
Focuses: ui, accessibility, javascript Cc:

Description

Hello,

I can't select all media link for a media with the button "Copy link".

https://i.ibb.co/1JsVLQN/media.jpg

It's only appears on Chrome but not Firefox.

I can reproduce on a new install with Chrome 77 on windows 10.

I have no javascript errors on console.

Thanks,

Attachments (10)

48463.diff (2.2 KB) - added by vabrashev 8 months ago.
48463.patch (3.8 KB) - added by sajjad67 8 months ago.
Feature to auto copy Media Link to clipboard when clicked 'Copy Link' label
48463-2.patch (8.7 KB) - added by sajjad67 8 months ago.
Patch with lot's of improvement of click event and event message plus button design
48463-2_preview.png (8.0 KB) - added by xkon 5 months ago.
48463.3.diff (8.8 KB) - added by audrasjb 3 months ago.
Media: Introduces copy link button
48463 site health example.png (16.7 KB) - added by afercia 5 weeks ago.
The copy button in the Site Health > Info page
48463.2.diff (10.5 KB) - added by afercia 4 weeks ago.
48463.4.diff (11.7 KB) - added by afercia 4 weeks ago.
48463.4.png (108.5 KB) - added by afercia 4 weeks ago.
48463.5.diff (11.7 KB) - added by afercia 9 days ago.

Download all attachments as: .zip

Change History (64)

#1 @audrasjb
8 months ago

  • Focuses ui accessibility added
  • Keywords needs-patch good-first-bug added
  • Version 5.2.4 deleted

#2 @audrasjb
8 months ago

Hi @theolg, welcome to WordPress Trac and thank you for opening this ticket!

Adding good-first-bug as I guess it could be a nice first contribution for new contributors.
I'll try to reproduce the issue on my side.

#3 @afercia
8 months ago

the button "Copy link"

Worth noting it's not a button. It's just a <label> element.

Related: #41612. Previously, the label text was just "URL", which was pretty confusing. Then changed to "Copy Link". Then it was agreed the text change doesn't actually solve the usability issue and it is worth exploring a different pattern with an actual button to copy the URL value. See ticket:41612#comment:48

If I'm not mistaken, no follow up ticket has been created yet so this ticket is more than welcome :) Not sure it's a good first bug, as anything related to the media views isn't that simple.

#4 @theolg
8 months ago

I try to find in /wp-includes/js/media-views.js how this focus and select is generated but I think it's over my capacitites

#5 @markdubois
8 months ago

I confirm this is a problem for me as well. I have sites with many images. In the past (several versions ago), one could copy the link to the specific image. Now, when one cursors over the words (Copy Link), the cursor changes and you "think" you can click and copy the link. However, when you do so, nothing happens (no information is actually copied to the clipboard). My solution is to click into the actual URL, then use CMD+a (Ctrl+a on Windows) to select the entire URL and CMD+c (Ctrl+c on Windows) to copy the URL to the clipboard. On sites with many images, this adds time to my workflow (which could easily be resolved by making this a button). For example, the text could indicate the field is a link and there could be a button on the right of the link which would allow one to copy the content/ URL.

I have just confirmed this on a MacBook AIR running MacOC 10.15, Brave browser Version 0.69.132 Chromium: 77.0.3865.90 (Official Build) (64-bit).

#6 @afercia
8 months ago

Besides the wording, which should be improved, worth also considering that core uses this CSS:

cursor: pointer;

for all <label> elements. Much like for buttons, see #47171, this may be confusing as it alters the default browsers style for labels and buttons.

@vabrashev
8 months ago

#7 @sajjad67
8 months ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Version set to 5.2

48463.patch is a tested patch with feature of Select Entire Input Text & Copy it to the clipboard. Browser support for Chrome 43+, Firefox 42+, Safari 10+, Edge and Internet Explorer 10+

@sajjad67
8 months ago

Feature to auto copy Media Link to clipboard when clicked 'Copy Link' label

This ticket was mentioned in Slack in #core by sajjad67. View the logs.


8 months ago

#9 @SergeyBiryukov
8 months ago

  • Milestone changed from Awaiting Review to 5.4

This ticket was mentioned in Slack in #core by sajjad67. View the logs.


8 months ago

#11 @afercia
8 months ago

  • Focuses javascript added
  • Keywords needs-patch added; good-first-bug has-patch needs-testing removed

@vabrashev @sajjad67 thanks for the patches! A few important considerations:

  • the copy link needs to work in all the media views, for example also in the "Add Media" modal and everywhere the URL input field is displayed
  • I'd suggest to explore building a new "copy button" view: it would also allow to avoid too many elements targeted with JS, which is sort of an anti-pattern in backbone.js
  • most importantly: the new view would be reusable in other places
  • I'd recommend to not make the <label> element clickable, as that's not the expected behavior
  • in #41612 it was mentioned a better approach would be: entirely remove the input field and just use a "Copy link" button
  • at that point the attachment URL could be displayed in plain text as there's no need for a readonly input field
  • the strings need to be translatable: see Sorry, unable to copy, same for the CSS generated content: Link Copied! (so I'm afraid we can't use the CSS approach)

This ticket was mentioned in Slack in #core by sajjad67. View the logs.


8 months ago

#13 @afercia
8 months ago

  • Keywords needs-design added

This ticket was mentioned in Slack in #design by sajjad67. View the logs.


8 months ago

This ticket was mentioned in Slack in #design by sajjad67. View the logs.


8 months ago

@sajjad67
8 months ago

Patch with lot's of improvement of click event and event message plus button design

This ticket was mentioned in Slack in #core by sajjad67. View the logs.


8 months ago

This ticket was mentioned in Slack in #design by sajjad67. View the logs.


8 months ago

#18 @sajjad67
8 months ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


5 months ago

@xkon
5 months ago

#20 @nrqsnchz
5 months ago

While @xkon's screenshot looks good, I'm not seeing the same results if I test the patch locally:

https://cldup.com/hHByjZx5AE.png

Not sure if the patch has an issue or if there's something wrong with my dev environment. Can anyone else test and confirm?

#21 follow-up: @nrqsnchz
5 months ago

Thanks @xkon for the troubleshooting help! :D

I'm able to properly test the patch now, and this is my feedback:

  1. The button needs to have a focus state, like any other button.
  2. When the button is clicked, focus is moved back to the URL field, this causes screen readers to not read the "Copied!"label. I think focus should stay on the button instead. What do you think @audrasjb?

#22 @audrasjb
5 months ago

Yes, I think the focus has to stay on the button.

I believe we should also use wp.a11y.speak to help screen reader users to understand the action.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


5 months ago

#24 @audrasjb
5 months ago

  • Keywords needs-refresh added
  • Milestone changed from 5.4 to 5.5

Moving to 5.5 as this ticket still needs decision, and then, a proper patch.

#25 @sabernhardt
5 months ago

#49434 was marked as a duplicate.

This ticket was mentioned in Slack in #design by sajjad67. View the logs.


3 months ago

This ticket was mentioned in Slack in #design by sajjad67. View the logs.


3 months ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


3 months ago

#29 @afercia
3 months ago

  • Owner set to audrasjb
  • Status changed from new to assigned

#30 in reply to: ↑ 21 @melchoyce
3 months ago

Replying to nrqsnchz:

  1. When the button is clicked, focus is moved back to the URL field, this causes screen readers to not read the "Copied!"label. I think focus should stay on the button instead. What do you think @audrasjb?

+1

What else is needed for this to move forward?

@audrasjb
3 months ago

Media: Introduces copy link button

#31 @audrasjb
3 months ago

  • Keywords needs-testing has-screenshots needs-dev-note added; needs-design needs-refresh removed

In 48463.3.diff:

  • focus repositionning on the button once the link is copied.
  • wp.a11y.speak added to inform screen readers
  • many coding standards fixes on the previous proposal

Works fine on my side.
Please help testing!

Cheers,
Jb

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


2 months ago

#33 @afercia
2 months ago

  • Keywords needs-refresh added

Regarding technical implementation details, please see discussion on latest accessibility bug-scrub on Slack, starting from https://wordpress.slack.com/archives/C02RP4X03/p1588344508354900

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


2 months ago

#35 @sajjad67
7 weeks ago

  • Keywords reporter-feedback added

Confirm! Works fine on my side too...

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


6 weeks ago

#37 @afercia
5 weeks ago

  • Keywords reporter-feedback removed

#38 @afercia
5 weeks ago

  • Keywords needs-patch added; has-patch needs-testing needs-dev-note needs-refresh removed

Thanks for the patches and the work done here! Looking at 48463.3.diff I'm not sure that's the best approach.

  • I would recommend to not use window.clipboardData and document.execCommand().
  • Instead, I'd recommend to use ClipboardJS which is already used in the Site Health > Info page: it has all it's needed and allows for a much simpler implementation. For an example, see src/js/_enqueues/admin/site-health.js
  • The same code shouldn't be replicated in two different places: the patch introduces two new functions in the attachment details.js and in post.js that do the same thing. Not sure there's the need to have this code in post.js in the first place, unless I'm missing something.
  • Translatable strings for JS can be provided via wp.i18n.
  • I don't think the copy button should be displayed within the input field.
  • The state message should be separated from the button and the button text should not change, following the accessibility team feedback and also for consistency with the implementation in Site Health (see attached screenshot).

I will try to make a new patch soon.

@afercia
5 weeks ago

The copy button in the Site Health > Info page

#39 @afercia
5 weeks ago

Related: #50322 for an example of a ClipboardJS improved implementation.

#40 @afercia
4 weeks ago

  • Owner changed from audrasjb to afercia

Not sure there's the need to have this code in post.js in the first place, unless I'm missing something.

I do understand now it's meant to be used in the "Edit Media" page :)

Going to try an implementation based on ClipboardJS.

@afercia
4 weeks ago

#41 @afercia
4 weeks ago

  • Keywords has-patch added; needs-patch removed

48463.2.diff is a first pass of a ClipboardJS implementation.

Some testing would be nice :) A few places to check:

  • Media Library "list mode" edit media page
  • Media Library "grid mode" attachment details modal
  • "Add Media" modal in the Classic Editor
  • The media modals in the block editor where the attachment details are displayed e.g.: the Image block

@afercia
4 weeks ago

#42 @afercia
4 weeks ago

48463.4.diff improvs the styling for the repsonsive view.

See below a few example screenshots.

@afercia
4 weeks ago

#43 @afercia
3 weeks ago

Some testing would be nice :) Pinging @azaozz as the 5.5 release squad Media Tech.
See also: #50322 and #50335

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


2 weeks ago

#45 @afercia
2 weeks ago

  • Keywords commit added

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


2 weeks ago

@afercia
9 days ago

#47 @afercia
9 days ago

48463.5.diff refreshes the patch.

#48 @afercia
9 days ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 48232:

Accessibility: Media: Add a "Copy URL" button to the attachment File URL fields.

For a number of years, various screens in the WordPress admin provided users with a readonly input field to copy the attachment file URL. Manually copying from a readonly field is an annoying task at best even for mouser users. It's a usability and accessibility issue at the same time.
These fields now have a new "Copy URL" button that is easy to use and accessible to everyone.

Props theolg, markdubois, vabrashev, sajjad67, xkon, nrqsnchz, melchoyce, audrasjb, afercia.
See #41612, #50322, #50335.
Fixes #48463.

#49 @afercia
9 days ago

In 48233:

Accessibility: Site Health: Improve the "Copy site info" button accessibility.

  • avoids a focus loss when clicking the "Copy site info" button
  • uses setTimeout() and clearTimeout() to properly handle the "Copied!" text
  • minor JavaScript coding standards

Props audrasjb, Clorith, afercia.
See #48463, #50335.
Fixes #50322.

#50 @afercia
9 days ago

In 48234:

Accessibility: Privacy: Accessibility improvements for the Privacy Policy Guide page.

Improves accessibility of the "Copy this section" button and "Return to Top" link:

  • uses setTimeout() and clearTimeout() to properly handle the "Copied!" text
  • simplifies the button text by removing the redundant visually hidden text
  • fixes the mismatching visual and DOM order of the Copy button and the "Return to Top" link
  • improves the "Return to Top" links by providing real page fragment identifiers, when possible
  • hides the "Return to Top" up arrow from assistive technologies
  • minor coding standards

Props afercia, garrett-eclipse.
See #48463, #50322.
Fixes #50335.

#51 @afercia
8 days ago

In 48268:

Media: Enable JavaScript translations for the media-views script after [48232].

See #48463.

#52 @afercia
8 days ago

Note: Typed a wrong commit message for [48268]: it should have been "for the post script".

#53 @azaozz
6 days ago

Sorry for the late review. Frankly I think adding of the "Copy URL" button borders on being a UI bloat in the media popup. Why would you need a third way to copy a pretty much useless URL? But it's already committed, so.....

Before this patch the URL could be copied easily by Ctrl + A followed by Ctrl + C (Cmd + A, Cmd + C on Mac). For people that prefer to use a mouse, this is triple click + right-click and copy.

At the same time the File URL field is an anachronism left over from the old old media library popup from WP 2.1. It used to be somewhat useful at the time, but currently solicits "not best behavior" for images as using the full size is generally not recommended.

Would be really interesting to know how many people that have commented on this ticket have ever copied an URL from there, and where they've pasted it? :) I personally have copied an URL from there around 10 times, ever (that's less than once per year), and only when testing, never in production.

There is also a small visual glitch: when translated the "Copy URL" string may get quite longer than the English version. Then the popup "Copied!" is pushed underneath but seems stuck to the border/outline.

#54 @afercia
5 days ago

Thanks @azaozz. Most of the design / UX discussion happened on #41612, where it started around the name of the input field. Originally, the name was "URL" which was reported to be confusing for users:

When testing the image widget recently, I found that many of the folks I tested with confused the image URL field with a place to add a link to their image. It says "URL," so people expect they can select and paste a URL into it

Then, we changed the name to "Copy Link" (see screenshot) and the input field was moved to the bottom. In a way, this made things even worse :)

Finally, there was consensus to add a Button to copy the URL and use a clarer name for the field.

Before this patch the URL could be copied easily by Ctrl + A followed by Ctrl + C (Cmd + A, Cmd + C on Mac). For people that prefer to use a mouse, this is triple click + right-click and copy.

Copying the URL on a mobile device isn't that easy, just to mention one case where the new button greatly helps. For mouse users, it is now just one click instead of a mix of various mouse/keyboard actions.

At the same time the File URL field is an anachronism left over from the old old media library popup from WP 2.1. It used to be somewhat useful at the time, but currently solicits "not best behavior" for images as using the full size is generally not recommended.

Interesting point. In this regard, at some point on #41612 we did consider to remove the input field. See:

what about to entirely remove the input field and just use a "Copy link" button?

Re: ths visual glitch:

There is also a small visual glitch: when translated the "Copy URL" string may get quite longer than the English version. Then the popup "Copied!" is pushed underneath but seems stuck to the border/outline.

I did notice that and can be remediated if really necessary. It's an edge case and I did want to keep the patch as simple as possible.

One more thing in my to-do list is to create a new ticket to propose a wp utility function that wraps ClipboardJS to:

Note: See TracTickets for help on using tickets.