Page MenuHomePhorge

Add a Copy-to-Clipboard Button to code blocks
Needs ReviewPublic

Authored by mturdus on Wed, Apr 16, 12:40.

Details

Summary

adds a small button to every code block which allows you to copy the code
block content to the clipboard.

Closes T16036

Test Plan
  1. Create/edit wiki page and add multiple code blocks
    • verify button appears on each code block
    • verify each button copies the correct code block content to the clipboard
    • verify in different page sizes: small screen hides table of content, while large screen shows table of content
  2. Create/edit maniphest task and add multiple code blocks
    • verify button appears on each code block
    • verify each button copies the correct code block content to the clipboard
  3. Create/edit phame blog and add multiple code blocks
    • verify button appears on each code block
    • verify each button copies the correct code block content to the clipboard
  4. Verify Remarkup Reference help guide

Diff Detail

Repository
rP Phorge
Branch
feature-add-copy-to-codeblock
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningwebroot/rsrc/js/core/RemarkupCodeblockCopy.js:1JAVELIN5`javelinsymbols` Not In Path
Unit
Test Failures
Build Status
Buildable 1877
Build 1877: arc lint + arc unit

Unit TestsFailed

TimeTest
0 msPhabricatorAnchorTestCase::testAnchors
EXCEPTION (RuntimeException): Code coverage needs to be enabled in php.ini by setting 'xdebug.mode' to 'coverage' #0 [internal function]: PhutilErrorHandler::handleError(2, '...', '...', 703) #1 /var/www/htdocs/phorge/arcanist/src/unit/engine/phutil/PhutilTestCase.php(703): xdebug_start_code_coverage(3)
0 msPhabricatorCelerityTestCase::testCelerityMaps
EXCEPTION (RuntimeException): Code coverage needs to be enabled in php.ini by setting 'xdebug.mode' to 'coverage' #0 [internal function]: PhutilErrorHandler::handleError(2, '...', '...', 703) #1 /var/www/htdocs/phorge/arcanist/src/unit/engine/phutil/PhutilTestCase.php(703): xdebug_start_code_coverage(3)
0 msPhabricatorConduitTestCase::testConduitMethods
EXCEPTION (RuntimeException): Code coverage needs to be enabled in php.ini by setting 'xdebug.mode' to 'coverage' #0 [internal function]: PhutilErrorHandler::handleError(2, '...', '...', 703) #1 /var/www/htdocs/phorge/arcanist/src/unit/engine/phutil/PhutilTestCase.php(703): xdebug_start_code_coverage(3)
0 msPhabricatorInfrastructureTestCase::testApplicationsInstalled
EXCEPTION (RuntimeException): Code coverage needs to be enabled in php.ini by setting 'xdebug.mode' to 'coverage' #0 [internal function]: PhutilErrorHandler::handleError(2, '...', '...', 703) #1 /var/www/htdocs/phorge/arcanist/src/unit/engine/phutil/PhutilTestCase.php(703): xdebug_start_code_coverage(3)
0 msPhabricatorInfrastructureTestCase::testRejectMySQLNonUTF8Queries
EXCEPTION (RuntimeException): Code coverage needs to be enabled in php.ini by setting 'xdebug.mode' to 'coverage' #0 [internal function]: PhutilErrorHandler::handleError(2, '...', '...', 703) #1 /var/www/htdocs/phorge/arcanist/src/unit/engine/phutil/PhutilTestCase.php(703): xdebug_start_code_coverage(3)
View Full Test Results (23 Failed)

Event Timeline

mturdus requested review of this revision.Wed, Apr 16, 12:40
aklapper added inline comments.
webroot/rsrc/js/core/RemarkupCodeblockCopy.js
3

The common style across the codebase seems to be require_celerity_resource('remarkup-codeblock-copy') (no CamelCase). Also I'm a bit surprised to see require_celerity_resource used to load a JS resource, seems like all other existing require_celerity_resource calls across the codebase load CSS but I assume you tested that this is needed.

ES5 compatibility issues fixed

Comments from z1 chat:

mturdus
I already tried the existing one: it doesn't work with cached objects (e.g. when you press F5) as the code block is on a cached wiki page. The behavior metadata isn't initialized anymore in the cached page.
bekay
15:11
bekay
That is true... but you can still use this behavior. I had the same problem but have solved it with setting the data with JS. Here my code - of course, you have to adapt this:

/**
 * @provides copy-network-path
 */

JX.onload(function()
{
    // Remarkup is cached and can't hold metadata
    JX.Stratcom.listen(
        'mousedown',
        ['network-path'],
        function(evt)
        {
            const node = evt.getNode('network-path');
            const text = node.parentNode.getAttribute('title');
            JX.Stratcom.addData(node, {
                text: text,
                successMessage: 'Path copied.',
                errorMessage: 'Copy of path failed.'
            })
        }
    );
});

mousedown because it will be fired before the click of the copy behavior.


This way your behavior will be slim because you use already codes core components.

@bekay
I tried to use JX.Stratcom.listen but I ran into the following issues:

  • I couldn't use mouseenter and mouseleave events for some reason
  • when using mousedown in combination with an a-tag with href=#, I don't know how to convert e.preventDefault. e.prevent() didn't work the same

@bekay

  • when using mousedown in combination with an a-tag with href=#, I don't know how to convert e.preventDefault. e.prevent() didn't work the same

That is something like e.kill() in Javelin

https://we.phorge.it/source/phorge/browse/master/webroot/rsrc/externals/javelin/core/Event.js

First of all: you should create the copy button server side. Ideally inside the remark rule for creating a code block. There you can attach the ressources too. And you can add sigils to the button. Javelin uses sigils to attach behaviors and listen to events.

aklapper requested changes to this revision.Wed, Apr 16, 20:01
This revision now requires changes to proceed.Wed, Apr 16, 20:01

javascript replaced with javelin stuff

This comment was removed by mturdus.
webroot/rsrc/js/core/RemarkupCodeblockCopy.js
3

This statement actually adds a script tag like this:

<script type="text/javascript" src="http://phorge/res/defaultX/phabricator/e29e01e7/rsrc/js/core/RemarkupCodeblockCopy.js">

I used this camelcase naming because of the naming of the files in rsrc/js/core
There seems to be 2 type of files in this directory: "behavior ones" and "non-behavior-ones".
The "non-behavior ones" are camelcase named and the others ones are "dash-formatted-named".

In D25966#25739, @bekay wrote:

First of all: you should create the copy button server side. Ideally inside the remark rule for creating a code block. There you can attach the ressources too. And you can add sigils to the button. Javelin uses sigils to attach behaviors and listen to events.

I'm still creating the button via javascript because if I do if on server side I may break some remarkup unit tests like the ones that use src/infrastructure/markup/remarkup/__tests__/remarkup/code-block-guess-from-name.txt.
I'm not sure how these are triggered or how these exactly work.

In D25966#25739, @bekay wrote:

First of all: you should create the copy button server side. Ideally inside the remark rule for creating a code block. There you can attach the ressources too. And you can add sigils to the button. Javelin uses sigils to attach behaviors and listen to events.

I'm still creating the button via javascript because if I do if on server side I may break some remarkup unit tests ...

That's not a really good reason for that - the test should be updated...

At least the button element should be added from PHP, to allow styling/layout; The implementation can be added in JS (I think that's done for hovercards)

You can trigger the tests using arc unit src/infrastructure/markup/remarkup/; Each test file is

input 
~~~~~
output-for-web
~~~~~
output-for-plain-text

You should have the javelinesymbols binary available for this change.

src/applications/phame/controller/post/PhamePostViewController.php
90

Why do you need this block explicitly again in this file?

webroot/rsrc/js/core/RemarkupCodeblockCopy.js
8–13

is this how we normally do these things?

@avivey is right... you should add the button where the remarkup code block is created. The remarkup tests should be edited and the markup of the button added in every txt file testing the code block!

src\infrastructure\markup\remarkup\__tests__\remarkup\backticks-whitespace.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\block-then-list.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\code-block-guess-from-name.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\code-block-whitespace.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\diff.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\just-backticks.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\newline-then-block.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\quoted-code-block.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\quoted-indent-block.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\tick-block-flavored.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\tick-block-multi-flavored-comment.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\tick-block-multi-flavored-empty.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\tick-block-multi-flavored.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\tick-block-multi.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\tick-block.txt
src\infrastructure\markup\remarkup\__tests__\remarkup\trailing-whitespace-codeblock.txt

Seperation of concerns: Markup is created server side, behavior is added with JS!

webroot/rsrc/js/core/RemarkupCodeblockCopy.js
34

No! Under no circumstances add listeners to a sigil from another behavior. Use your own sigil specific to this behavior like "codeblock-copy", add it to the button and listen to it here.

38

Maybe we should comment why we add data: "Because Remarkup is cached, it will forget data added server side. We have to add the data for the copy behavior here directly before every click."

41

Is seems not logical to add data for the tooltip on a mousedown - because then is will not show before the button is clicked. Maybe ditch the tooltip behavior altogether and just use a good oldfashioned title attribute. Should be enough...