WordPress.org

Make WordPress Core

Opened 21 months ago

Last modified 4 weeks ago

#44588 accepted enhancement

Denote the Copy action is complete by updating the Copy button to state 'Copied'

Reported by: garrett-eclipse Owned by: garrett-eclipse
Milestone: 5.5 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch has-screenshots commit
Focuses: administration Cc:

Description

Hello,

With the Suggested Privacy content introduced in 4.9.6 the sections provide a 'Copy' button;
https://i.imgur.com/WP8JmEw.png

It would be nice if once the action is complete that the button would update to state 'Copied' briefly before switching back to 'Copy' for more copying. By switching to 'Copied' temporarily the users gets direct feedback that the copy action was completed so they don't keep clicking the button unnecessarily.

Chers

Attachments (16)

Screen Shot 2018-07-16 at 11.19.30 AM.png (9.8 KB) - added by garrett-eclipse 21 months ago.
Copy Button
Screen Shot 2019-11-14 at 10.36.46 PM.png (126.1 KB) - added by garrett-eclipse 4 months ago.
Copy to TextEdit
Screen Shot 2019-11-14 at 10.37.07 PM.png (850.4 KB) - added by garrett-eclipse 4 months ago.
Copy to Microsoft Word (Mac)
44588.diff (5.6 KB) - added by nickylimjj 3 months ago.
copied-to-clipboard.png (130.8 KB) - added by nickylimjj 3 months ago.
'Copied!' feedback when button is clicked.
clipboard-data.png (208.3 KB) - added by nickylimjj 3 months ago.
Data copied to clipboard.
44588.2.diff (5.7 KB) - added by nickylimjj 3 months ago.
Alternate patch with 'Copied' text float right
copied-to-clipboard-right.png (128.4 KB) - added by nickylimjj 3 months ago.
Screenshot of 'Copied!' closer to button.
44588.3.diff (5.5 KB) - added by garrett-eclipse 3 months ago.
Updated patch with minor improvements
44588.4.diff (6.0 KB) - added by xkon 3 months ago.
44588.5.diff (1.7 KB) - added by nickylimjj 2 months ago.
without clipboardJs. just 'Copied!' text.
44588.6.diff (3.3 KB) - added by xkon 2 months ago.
44588.7.diff (6.4 KB) - added by garrett-eclipse 2 months ago.
Refresh for a few minor items and made the Copied text dismissable after 3 seconds so additional clicks will receive the indication their text was copied.
eefc573b39676dddd47856849123463d.gif (24.1 KB) - added by garrett-eclipse 2 months ago.
GIF to illustrate the vanishing Copied text. This is so additional clicks get the same feedback that something occurred.
44588.8.diff (6.1 KB) - added by garrett-eclipse 4 weeks ago.
Refreshed patch
44588.9.diff (7.1 KB) - added by garrett-eclipse 3 weeks ago.
Refreshed patch after #49540 was committed to 5.4. As well added comments to the copy action to explain the process that occurs

Download all attachments as: .zip

Change History (48)

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


20 months ago

#2 @desrosj
20 months ago

  • Focuses administration added; privacy removed
  • Keywords ui-feedback added
  • Milestone changed from Awaiting Review to Future Release

Good find, @garrett-eclipse. This should definitely get improved. I think adding user feedback to indicate the copy happened successfully is a good idea, but I also don't know that there is enough information here. What should the user do after copying? What does "Copy" actually do? Maybe the button could be "Copy to Clipboard".

I am marking this as ui-feedback to explore this a bit more.

#3 @desrosj
20 months ago

It's also worth noting that there is one copy button for the entire suggested text provided with core. Each plugin that suggests text will receive their own copy button. Maybe "Copy Section" after each section, or a way to bulk select the sections to copy would be useful.

#4 @garrett-eclipse
20 months ago

Thanks @desrosj, good thoughts

I've created #44677 to address the verbiage suggestion of 'Copy to Clipboard'.

And created #44678 to address the enhancement idea for bulk copy of sections.

Cheers

#5 @birgire
19 months ago

The "Email Data" button, on the privacy export data page, comes to mind, as it gives an immediate feedback when it's clicked on.

But it can only be clicked once, for each page load.

I think we should be able to click many times on the Copy button, without reloading the page.

I wonder about the following flow, with a javascript timer:

  • Copy button is clicked.
  • Button text changes immediately to "Copied".
  • After n milliseconds, the button text is back to "Copy".

Another one is to have a label near the button showing the state, but it should be cleared when the user copies other sections or makes other manual copies of text on the page. For a very long text (many plugin sections), this might be slower than the first option.

Third option could be to not show any feedback at all (status quo).

ps:

Here's an interesting discussion I found on the topic:

https://ux.stackexchange.com/questions/40950/what-is-the-best-way-to-confirm-something-has-been-copied

#6 @garrett-eclipse
19 months ago

  • Keywords ux-feedback added

Thanks @birgire, I do like the idea of providing feedback directly in the button there so maybe the timer is the best idea.
There was an idea in your link I thought interesting of checking the clipboard to see if the contents is in there and while it's there keep the button as 'Copied' until the clipboard changes.

#7 @garrett-eclipse
12 months ago

  • Keywords needs-patch added; ui-feedback ux-feedback removed
  • Milestone changed from Future Release to 5.3
  • Owner set to garrett-eclipse
  • Status changed from new to assigned

With r45044 from #46647 the clipboard.js library was introduced to core. We should follow the convention presented for the Site Health 'Copy to Clipboard' button on the Privacy Policy guide's Copy button.

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


8 months ago

#9 @birgire
7 months ago

#47998 was marked as a duplicate.

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


6 months ago

#12 @davidbaumwald
6 months ago

  • Milestone changed from 5.3 to 5.4

@garrett-eclipse This ticket still needs a patch. With version 5.3 Beta 1 landing tomorrow, this enhancement is being moved for consideration in 5.4. If this can be patched for 5.3, feel free to update the ticket.

#13 follow-up: @garrett-eclipse
4 months ago

Rather than open a new ticket wanted to flag the copy to TextEdit & Microsoft Word should be tested once clipboard.js is adopted here to ensure the unnecessary formatting is cleaned up.
[Edit] In the attached screens below the main unnecessary formatting that comes over is the background and empty space.

Last edited 4 months ago by garrett-eclipse (previous) (diff)

@garrett-eclipse
4 months ago

Copy to Microsoft Word (Mac)

This ticket was mentioned in Slack in #core-privacy by nickylimjj. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-php by nickylimjj. View the logs.


3 months ago

@nickylimjj
3 months ago

@nickylimjj
3 months ago

'Copied!' feedback when button is clicked.

@nickylimjj
3 months ago

Data copied to clipboard.

#16 @nickylimjj
3 months ago

  • Keywords has-patch has-screenshots added; needs-patch removed

A patch and 2 pictures to demonstrate the patch. Perhaps to review

  1. style design for the 'Copied!' feedback. Where should it be?
  2. Having a challenge to get the formatting right for the copied data since I accessed the data through internal variables (that is in html) because it lets me
    • parse out the description
    • not leave a highlighted side-effect on the text on button click

@nickylimjj
3 months ago

Alternate patch with 'Copied' text float right

@nickylimjj
3 months ago

Screenshot of 'Copied!' closer to button.

@garrett-eclipse
3 months ago

Updated patch with minor improvements

#17 @garrett-eclipse
3 months ago

  • Keywords needs-testing dev-feedback 2nd-opinion added

Thanks for the patch @nickylimjj this is a great start here.

Reviewing I made a few minor tweaks in 44588.3.diff, they are;

  • Cleanup JS variable creation.
  • Updated JS a11y.speak text to use 'Privacy Policy Guide section' instead of 'Privacy Content' to be more specific.
  • Reverted the css class for .privacy-text-copy-button back to .privacy-text-copy in case plugins have styled or targetted it. This is to avoid back-compat issues.
  • Cleanup PHP for coding standards on indentation.
  • In the PHP removed unneeded variable instantiations when they are stings only being used a single time.

I'm adding dev-feedback / 2nd-opinion to get thoughts on the copy formatting change as you noted it now copies HTML instead of formatted text. From my testing this works ok in the block editor as it gets converted to blocks, but in the classic editor and classic block it's pasted as HTML so you'd need to use the Text tab to paste. As well pasting to rich text editors like Word, Pages, etc no longer holds the formatting. Personally I'm on the fence as I wasn't a huge fan of the original formatted copy but now it's less transferrable.
*Possibly we can place the headings and paragraphs on new lines so it's less a jumble and easier to read.

#18 @garrett-eclipse
3 months ago

P.S. I forgot to address your placement question, I agree having the Copied text close to the button makes the most sense and avoids it being overlooked on large screens. My patch is an enhancement to that version.

#19 @xkon
3 months ago

Hey @nickylimjj & @garrett-eclipse, thanks for the patches!

I've checked both 44588.2.diff and 44588.3.diff and they look promising but there are a couple of issues that need to be addressed to get this finalized.

The existing code is targetting the nearest "wp-suggested-text" and copies its inner code. Since the patches introduce the clipboard.js the copied information has to be added to the button.

The code on the patches currently is as:
$content .= sprintf( '<button type="button" class="privacy-text-copy button" data-clipboard-text="%1$s">', esc_attr( self::get_default_content( false, false ) ) );

This means that all clipboard buttons are forced to copy the default text due to self::get_default_content( false, false ). This should be changed to $section['policy_text'] instead so each button has it's own section text available to get copied.

Also, note that since "everything" will actually be copied now the clipboard text ends up having the parent <div class="wp-suggested-text"> wrapper which is not needed.

As far as I can see from various plugins that I've installed to test this we most likely need to strip the wrapper mentioned as well as all the class="privacy-policy-tutorial" as they are not needed for copying and they might interfere with any front-end styling. This should leave a clean text with just headings and paragraphs for copying.

I hope this makes sense & tell me if you need any help with bumping a .4 patch with these changes so I can take a closer look!

Last edited 3 months ago by xkon (previous) (diff)

@xkon
3 months ago

#20 @xkon
3 months ago

Following up after a brief DM over Slack and looking further into this.

I played around with copy/pasting on various mediums (offline/online document editors) & Gutenberg.

It seems that there are some issues when using the data-clipboard-text due to various tabs and all other special chars used in each section here.

I've tried converting the text striping out anything that's not needed like "\r\n" and tabs etc but then the copy/paste styling seemed to not be respected by all mediums. I couldn't pinpoint why but there wasn't any need to go further with this approach since it kinda broke from scratch. Note that the issue was for the policies that plugins might add, the default policy worked just fine :).

Also, the text data-clipboard parameter practically adds all of the text that we wanted to copy again in the HTML source and that didn't seem right to me since there's no difference in the HTML as we actually want to keep the formatting here. Essentially on this implementation, the data-clipboard-text was printing the same content a second time just for the usage of the copy method creating a bigger HTML source without any actual need.

In 44588.4.diff I've converted the patch to use data-clipboard-target instead of the data-clipboard-text.

Each section now has its own unique ID that is generated by the source name like id="policy-text-PLUGINNAME". This makes it easier to add targets to clipboard.js so each copy button can target the specific section it belongs to and copy it.

This way there's no need to output double HTML inside data placeholders and the styling/formatting seems to be working fine on various mediums / Gutenberg / Google Docs as expected.

#21 @xkon
3 months ago

After checking #44621 I realized (as I had forgotten about it, sorry!) that the current Copy action only grabs the headings + suggested texts from each section.

On the other hand, the patches introduced here ( including mine :) ) copy "everything" including notes from authors and explanatory texts that are not needed from the incoming texts from plugins.

I tried playing around a bit with clipboard.js trying to make it copy only the suggested texts as needed but to no avail.

If anyone else likes to give this a try feel free to do so, but if we can't make it work with clipboard.js I'd suggest leaving the functionality as is at the moment to only copy the "suggested text" as needed and simply add the "Copied!" notice.

@garrett-eclipse would you be ok with that instead?

#22 @nickylimjj
3 months ago

Hey @xkon!

The initial patches (up to .3) that uses data-clipboard-text manage to exclude the description (refer to clipboard-data.png) because the first parameter in self::get_default_content( false, false ) can control that. I considered data-clipboard-target but I had the same problem you encountered in your .4 patch + it left a side effect on my browser (the text was highlighted, as if you dragged your cursor across).

Perhaps we would want to redefine the$section['policy_text'] data structure for more effective targeting? Otherwise, another strategy could be to create a function that parses $section['policy_text'] since it's already expressive as HTML.

#23 @xkon
3 months ago

Hey @nickylimjj, thanks for the follow-up!

Unfortunately, the text that is added at the moment is a little bit "vague" in regards of the HTML as we didn't set any standard classes to somehow distinguish what is explanatory vs suggestion. This has led to plugins adding just paragraphs and heading without any major distinction though which is kind of making it "non-expressive".

The difference that was visible currently with the current italic Suggested text was that it was just wrapped in an extra <p> without any class. This is also under consideration at the moment on changing in #44621 that could potentially add a wrapper around the suggested text only for better visual indication so that would definitely make targetting way easier for the copying purposes.

What I propose for now to avoid losing the scope here also is to simply add the functionality needed for this ticket only which is the indication of "Copied!".

Moving to clipboard.js and changing the actual copy mechanism should be tracked on a different ticket than this, at least that's what I propose to not end up having tickets again waiting on each other that are changing functionality without it being obvious.

This ticket was mentioned in Slack in #core-privacy by xkon. View the logs.


2 months ago

@nickylimjj
2 months ago

without clipboardJs. just 'Copied!' text.

#25 @nickylimjj
2 months ago

created a patch (.5) without the clipboardJS dependency.

@xkon
2 months ago

#26 @xkon
2 months ago

Thanks @nickylimjj for .5 patch :) !

44588.6.diff goes 1 step further following 44588.5.diff and also adds the i18n as well as a11y.speak functionality.

@garrett-eclipse I think we're OK with this feel free to take another look if anything else might be needed else I'm +1 for commit tag.

@garrett-eclipse
2 months ago

Refresh for a few minor items and made the Copied text dismissable after 3 seconds so additional clicks will receive the indication their text was copied.

@garrett-eclipse
2 months ago

GIF to illustrate the vanishing Copied text. This is so additional clicks get the same feedback that something occurred.

#27 @garrett-eclipse
2 months ago

  • Keywords dev-feedback 2nd-opinion removed
  • Status changed from assigned to accepted

Thanks @nickylimjj & @xkon for the work here. Sorry my suggestion to adopt ClipboardJS didn't pan out. I appreciate you both delving into it fully before abandoning that approach. I agree after testing those patches the clipboard contents was never really up to par with what we had originally so am all onboard with the current direction.

In 44588.7.diff I've made a minor refresh to re-apply some changes from my comment:17 mainly;

  • Cleanup JS variable creation.
  • Cleanup PHP for coding standards on indentation.
  • In the PHP removed unneeded variable instantiations when they are strings only being used a single time.

I also updated the copy action to introduce a setTimeout in order to dismiss the Copied! text after 3 seconds. This is so when coming back to a used button and clicking copy again we get the feedback. When we didn't dismiss the text any additional clicks don't receive that visual feedback and so aren't sure if the action completed. Demo;
https://core.trac.wordpress.org/raw-attachment/ticket/44588/eefc573b39676dddd47856849123463d.gif

This feels good to go to me but I don't want to mark my own patch for commit, could either of you (@nickylimjj or @xkon) give my latest patch a once over and test and we'll put our stamp of approval on this. My tests, CS checks and PHPUnit tests all ran green.

Sidenote: While testing with some plugins such as WooCommerce I noticed they were still using the old .wp-policy-help class which resulted in their help/tutorial text being included in the copy content. To address this I opened 49282

#28 in reply to: ↑ 13 @garrett-eclipse
2 months ago

Replying to garrett-eclipse:

Rather than open a new ticket wanted to flag the copy to TextEdit & Microsoft Word should be tested once clipboard.js is adopted here to ensure the unnecessary formatting is cleaned up.
[Edit] In the attached screens below the main unnecessary formatting that comes over is the background and empty space.

Revisiting this comment I didn't want to overlook this so in order to keep this ticket moving forward have created #49283 to address the empty lines where the tutorial text was hidden during the copy action.

#29 @xkon
7 weeks ago

  • Keywords commit added; needs-testing removed

I've retested the final patch here and works as expected on my end as well, marking this for commit. Thanks for the refresh!

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


7 weeks ago

#31 @xkon
7 weeks ago

  • Keywords needs-refresh added; commit removed
  • Milestone changed from 5.4 to Future Release

This needs a refresh as it seems after other various recent commits.

I'm marking this for a refresh & future release since we're so close on 5.4 Beta at the moment.

@garrett-eclipse
4 weeks ago

Refreshed patch

#32 @garrett-eclipse
4 weeks ago

  • Keywords commit added; needs-refresh removed
  • Milestone changed from Future Release to 5.5

I've refreshed the patch in 44588.8.diff, let's get this in for 5.5.

@garrett-eclipse
3 weeks ago

Refreshed patch after #49540 was committed to 5.4. As well added comments to the copy action to explain the process that occurs

Note: See TracTickets for help on using tickets.