Exclude unselectable text from being copied #138

bicarlsen · 2021-09-20T16:11:53Z

In the sphinx_rtd_theme I was using the :linenos: option for the literalinclude directive to display line numbers for my code blocks. However, when using the copybutton extenstion for users to easily copy the code, the line numbers were being included in the copied text, even though they aren't selectable by the user.

I added functionality to exclude this unselectabel text from the copied text, along with an option copybutton_exclude_unselectable to determine its behavior. This option is by default true.

…electable text should be excluded from copy and do so if needed. [_static/copybutton_funcs.js] Added functionality to get unselectable elements and remove their text.

welcome · 2021-09-20T16:11:55Z

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

…t. Fixed typo in Development section.

bicarlsen · 2021-09-20T19:28:29Z

When running pre-commint run --all and npm test on my local machine all the tests pass, so I am not sure why it is failing here.

theletterf · 2021-10-11T09:58:17Z

This should be merged, @choldgraf. Could you give it a look?

docs/index.rst

choldgraf

hey @bicarlsen - thanks for this PR! I have a few comments and requests in there, mostly around making sure that this addition is documented clearly so that others can understand it and maintain it in the future.

One other quick question - can we imagine any situation where an author would want a non-selectable text to be copied? If not then I'd be fine just making this happen and not exposing another config key

choldgraf · 2021-10-11T14:30:25Z

sphinx_copybutton/_static/copybutton.js_t

@@ -118,7 +118,11 @@ const addCopyButtonToCodeCells = () => {

 var copyTargetText = (trigger) => {
  var target = document.querySelector(trigger.attributes['data-clipboard-target'].value);
-  return formatCopyText(target.innerText, {{ "{!r}".format(copybutton_prompt_text) }}, {{ copybutton_prompt_is_regexp | lower }}, {{ copybutton_only_copy_prompt_lines | lower }}, {{ copybutton_remove_prompts | lower }}, {{ copybutton_copy_empty_lines | lower }}, {{ "{!r}".format(copybutton_line_continuation_character) }}, {{ "{!r}".format(copybutton_here_doc_delimiter) }})
+  let text = (


Could you provide a comment about what this is doing? This uses some JS syntax that I haven't seen before (with ? and :) and we want this theme to be maintainable for non-JS developers so it'd help to have extra explanation

The p ? t : f syntax is the ternary operator that returns t if p is true and f otherwise.

choldgraf · 2021-10-11T14:30:53Z

sphinx_copybutton/_static/copybutton_funcs.js

@@ -2,10 +2,54 @@ function escapeRegExp(string) {
    return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); // $& means the whole matched string
 }

+function getUnselectable(target) {


Please provide a high-level docstring that describes what this function does

choldgraf · 2021-10-11T14:31:21Z

sphinx_copybutton/_static/copybutton_funcs.js

+    return unselectable;
+}
+
+function getFilteredText(target, exclude) {


same here - please provide a docstring

choldgraf · 2021-10-11T14:33:08Z

docs/index.rst

+Exclude unselectable text from copied text
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+By default, ``sphinx-copybutton`` will exclude text that is not selectable by the user. Unselectable text is determined with the CSS style


Suggested change

By default, ``sphinx-copybutton`` will exclude text that is not selectable by the user. Unselectable text is determined with the CSS style

By default, ``sphinx-copybutton`` will exclude text that is not selectable by the user.

Unselectable text is determined with the CSS style

Suggested change

By default, ``sphinx-copybutton`` will exclude text that is not selectable by the user. Unselectable text is determined with the CSS style

By default, ``sphinx-copybutton`` will exclude text that is not selectable by the user. Unselectable text is determined with the CSS style

choldgraf · 2021-10-11T14:33:34Z

docs/index.rst

+Exclude unselectable text from copied text
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+By default, ``sphinx-copybutton`` will exclude text that is not selectable by the user. Unselectable text is determined with the CSS style


can you provide an example of what un-selectable text generally is, so that users understand when they'll likely use this?

choldgraf · 2021-10-11T15:52:08Z

One more quick thought - another option would be to let users specify one or more classes that should be removed from the text before copying. That way I think we could simplify the copying logic considerably. For example, if the default class to be excluded was .linenos, then the code could simply be:

// Clone our target so we don't modify the live DOM
clone = target.cloneNode(true)

// Select the classes we want to remove, and then remove each one
clone.querySelectorAll(".linenos").forEach((node) => (node.remove()))

// This should not have any elements we don't want
textToCopy = clone.innerText;

I've just merged a PR to add a couple minimal examples to the docs that should demonstrate this behavior not working. We can use this to confirm that this PR does, in fact, fix the problem :-)

#140

bicarlsen · 2021-10-11T20:27:14Z

can we imagine any situation where an author would want a non-selectable text to be copied? If not then I'd be fine just making this happen and not exposing another config key

I can not, but figured I would leave it as an option just in case. I'm happy to remove it though.

bicarlsen · 2021-10-11T20:32:30Z

One more quick thought - another option would be to let users specify one or more classes that should be removed from the text before copying. That way I think we could simplify the copying logic considerably.

I agree and I mentioned this in regard to Issue #9 as I think it is all tied in together. I'm happy to add that functionality, but wanted to get commetns on it first.

choldgraf · 2021-10-13T23:17:19Z

I do like the simplicity of the implementation in #138 (comment) so unless we can see any downsides to that approach, I'd be fine going that direction

bicarlsen · 2021-10-18T09:30:42Z

@choldgraf , I agree about your comment regarding #138 (comment). There is a reason I did it recursively, but can't remember the exact reason now. I'll reimplmenet using the Node.clone method, and keep the recursive method off to the side incase it's needed later.

bicarlsen · 2021-10-18T19:54:12Z

I remember why I did it recursively. It's because I exclude Node's who have the userSelect style set to none. However, this property is only available when using getComputedStyle, which must be used on each Node individually.

@choldgraf , what is your opinion on excluding unselectable Nodes via th .linenos class, or by checking directly the userSelect property?

choldgraf · 2021-10-25T02:37:36Z

@bicarlsen sorry for the slow response! My general tendency is to go with the simpler possible where we can, unless there's a strong reason to use the more complex approach. Here, it seems like excluding the .linenos class will get us roughly the same functionality with a lot less JS, so I'd be +1 on taking that approach

bicarlsen · 2021-10-25T07:57:23Z

@choldgraf Great, we should be good to go. I haven't written any tests for it yet, but will do so as soon as I have time.

bicarlsen · 2021-11-30T18:17:56Z

@choldgraf just wanted to give this a bump, since it's been a while.

choldgraf

just a quick comment about the config, but this looks pretty good to me.

you mentioned that you wanted to write tests, I think that still needs to happen right?

choldgraf · 2021-11-30T19:18:22Z

docs/index.rst

+
+.. code-block:: python
+
+   copybutton_exclude = '.exclude_me'


how about instead of hard-coding .linenos, we just set this value to be a list that defaults to ['.linenos']. Then if somebody wants to define their own exclusion list, they can provide it and include .linenos as well on their own. This seems a bit simpler

choldgraf · 2021-11-30T19:20:18Z

docs/index.rst

@@ -395,6 +404,7 @@ For example, to make the copy button visible by default (not just when a code ce
      html_css_files = ["custom.css"]

 See the `Sphinx documentation on custom CSS for more information <https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-html_static_path>`_.
+>>>>>>> 6ee971ab8f9c3741d380240c180b1ff47f1d2ddc


I think you have some git merge resolution cruft in here

…ith funcitons using .innerText. [package.json] Added browser-env module for DOM support in testing. [docs/index] Updated documentation. [copybutton, copybutton_funcs] Fixed minor errors. Updated default exclude text. [__init__] Updated default exclude text.

bicarlsen · 2021-12-01T12:50:26Z

you mentioned that you wanted to write tests, I think that still needs to happen right?

I tried to implement testing but apparently it is diffuclt to test functions that use .innerText because it requires a layout engine, which jsdom, which most of the testing utilities use, doesn't include.

I've included the tests in the test.js file, but commented out their actual running.

for more information, see https://pre-commit.ci

rkdarst · 2022-03-22T10:17:52Z

I just found a reason why "unselectable text" is better than .linenos:

When rendering shell prompts, the $ shell prompt before a command is un-selectable, so that copying does the right thing when copying. Using the copybutton currently includes $. Who knows what other highlighters would be this smart.

Example: text boxes found in this section: https://coderefinery.org/git-intro/basics/#recording-a-snapshot-with-git

So, my vote for the "unselectable text" method (not that it counts for much).

rkdarst · 2022-06-10T07:37:32Z

This became an issue in a workshop, so I did more investigation.
The previous version of this PR (minimum working copy: https://github.com/rkdarst/sphinx-copybutton/tree/exclude-unselectable-new ) did work to exclude unselectable text, including the $ in shell-session highlighted blocks.

But, in my live site, even normal un-selection didn't work. It turns out that not selecting $ comes from sphinx_rtd_theme, which is interesting, since the parsing comes from pygments (and the reason it doesn't work on my live site on readthedocs is that it uses sphinx_rtd_theme that is too old):
https://github.com/readthedocs/sphinx_rtd_theme/blob/0da22b885be387b79f7552c92be00fd14d11228a/src/sass/_theme_rst.sass#L103-L106

  div.highlight
    span.linenos, .gp
      user-select: none
      pointer-events: none

If you search pygments, I find that gp stands for the Generic.Prompt token, so there is already some sort of abstraction layer leakage between the themes and parsers.

So my proposal:

decide if we go with unselectable text or configurable.
- Now I am more on the side of the current strategy (explicitly define the excluded classes), but the .lineno .gp information will start being copied around more and more places which doesn't seem ideal, but anyway it's still going to be copied to the themes...
add .gp to the list of default excludes if we stay with this strategy
add .gp as user-select: none to sphinx-book-theme? a quick grep doesn't show that string in the relevant context. I don't even know if it uses pygments as a highlighter so that this is relevant, though. (I don't yet use the theme myself)

rkdarst

Trying to address one of the requests from choldgraf.

And proposal of adding .gp to the default exclude list, but I'm not yet sure if that is a good idea.

rkdarst · 2022-06-16T10:28:14Z

docs/index.rst

+Exclude text
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+You may exclude elements matching CSS selectors from being copied by specifying the `copybutton_exclude` option in ``conf.py``.


Suggested change

You may exclude elements matching CSS selectors from being copied by specifying the `copybutton_exclude` option in ``conf.py``.

You may exclude elements matching CSS selectors from being copied by specifying the `copybutton_exclude` option in ``conf.py``.

For example, a viewer usually doesn't want to copy the line numbers, and CSS provides a way to exclude this. This option

implements that option for sphinx-copybutton as well.

rkdarst · 2022-06-16T10:28:41Z

docs/index.rst

@@ -372,6 +372,17 @@ In this case, all elements that match ``your.selector`` will have a copy button
 added to them.


+Exclude text


Suggested change

Exclude text

Exclude text from being copied

rkdarst · 2022-06-16T10:31:01Z

docs/index.rst

+
+   copybutton_exclude = '.exclude_me'
+
+By default `.linenos` is excluded. If you specify the `copybutton_exclude` option it will replace the default.


Suggested change

By default `.linenos` is excluded. If you specify the `copybutton_exclude` option it will replace the default.

By default `.linenos, .gp` are excluded. If you specify the `copybutton_exclude` option it will replace the default.

`.linenos` is the Sphinx default for line numbers, and `.gp` is Pygments (the highlighter of Sphinx) for "prompt", e.g. `$` in a shell-session.

I am unsure if we want .gp by default, but since sphinx_rtd_theme (and probably others should as well) makes it unselectable by default, it seems reasonable that we mirror that behavior.

But, if we add .gp by default, then themes which don't un-select .gp will confuse users. If we do add it, users in themes that understand pygments have to change things. Hm, seeming more like like interpreting the CSS has some sort of advantages but I'm not sure if it's worth the extra work yet...

rkdarst · 2022-06-16T10:33:24Z

sphinx_copybutton/__init__.py

@@ -68,6 +69,7 @@ def setup(app):
    app.add_config_value("copybutton_here_doc_delimiter", "", "html")
    app.add_config_value("copybutton_image_path", "copy-button.svg", "html")
    app.add_config_value("copybutton_selector", "div.highlight pre", "html")
+    app.add_config_value("copybutton_exclude", ".linenos", "html")


Suggested change

app.add_config_value("copybutton_exclude", ".linenos", "html")

app.add_config_value("copybutton_exclude", ".linenos, .gp", "html")

Wait, does this work to select multiple? In the end it gets passed to the filterText function.

rkdarst · 2022-06-16T10:57:19Z

For reference, I just found this:

Apparently .gp (GenericPrompt) is also now excluded in the basic Sphinx CSS:
https://github.com/sphinx-doc/sphinx/pull/9120/files (a bit more commentary on this)

Sphinx basic.css_t excludes the following things: table.highlighttable td.linenos, span.linenos, div.highlight span.gp.

Upstream pygments doesn't seem to do any user-select: none in any CSS (though I'm not sure if it is designed to do this, there aren't many *.css* files in there)

- `.linenos` is the line numbers, and `.gp` is the tag from the pygments highlighter for things like the `$` prompt. Upstream sphinx CSS excludes `.gp` from being copied, see the analysis here: executablebooks#138 (comment) - If upstream sphinx excludes `.gp`, we probably should as well.

- Re-add documentation from PR executablebooks#138 - that PR got conflicted with a docs rearrangement, so I add the docs back in one chunk now. I include my own suggestions in that PR.

rkdarst · 2022-09-13T08:53:25Z

#178 is a rebase of this that resolves conflicts with the docs and hopefully includes the suggestions from above.

bicarlsen added 2 commits September 20, 2021 17:56

[__init__] Added config value. [_static/copybutton.js_t] Check if uns…

3307827

…electable text should be excluded from copy and do so if needed. [_static/copybutton_funcs.js] Added functionality to get unselectable elements and remove their text.

[test.js] Removed unused function from import.

ce16954

[docs/index.rst] Updated documentation for excluding unselectable tex…

42355fc

…t. Fixed typo in Development section.

This was referenced Sep 21, 2021

Sphinx "inline" line numbers? #85

Open

[Improvement suggestion] Use Class Tag to enable any element copy to clipboard #9

Open

choldgraf reviewed Oct 11, 2021

View reviewed changes

docs/index.rst Outdated Show resolved Hide resolved

Update docs/index.rst

e35b070

choldgraf requested changes Oct 11, 2021

View reviewed changes

bicarlsen added 3 commits October 11, 2021 22:20

Added documentation.

e683c26

Resolved merge conflict.

03e46d4

Resolved merge conflicts.

ce20709

[__init__] Removed config value. Cleaned up rest of code.

acc63cd

Always remove Nodes. Give user option to exclude other Nodes.

e78f41e

choldgraf mentioned this pull request Oct 25, 2021

How can I remove linenos from the copied text? #144

Closed

bicarlsen added 3 commits October 25, 2021 09:44

Fixed bugs.

0941ef2

Changed formatting.

d1e4962

Resolved merge conflicts.

5e37a68

choldgraf reviewed Nov 30, 2021

View reviewed changes

[pre-commit.ci] auto fixes from pre-commit.com hooks

665fe0a

for more information, see https://pre-commit.ci

effigies mentioned this pull request May 6, 2022

remove lineos in example to make easier to copy-paste bids-standard/stats-models#21

Merged

rkdarst reviewed Jun 16, 2022

View reviewed changes

2bndy5 mentioned this pull request Jun 26, 2022

Why deprecate the html_codeblock_linenos_style option? sphinx-doc/sphinx#10265

Closed

nmstoker mentioned this pull request Jun 30, 2022

Copy button on doc examples copies line numbers as well as code pyscript/pyscript#565

Closed

3 tasks

rkdarst mentioned this pull request Sep 13, 2022

ENH: Exclude unselectable text from being copied (update) #178

Merged

choldgraf closed this in #178 Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude unselectable text from being copied #138

Exclude unselectable text from being copied #138

bicarlsen commented Sep 20, 2021

welcome bot commented Sep 20, 2021

bicarlsen commented Sep 20, 2021

theletterf commented Oct 11, 2021

choldgraf left a comment •

edited

Loading

choldgraf Oct 11, 2021

bicarlsen Oct 11, 2021

choldgraf Oct 11, 2021

choldgraf Oct 11, 2021

choldgraf Oct 11, 2021

choldgraf Oct 11, 2021

choldgraf commented Oct 11, 2021 •

edited

Loading

bicarlsen commented Oct 11, 2021

bicarlsen commented Oct 11, 2021

choldgraf commented Oct 13, 2021

bicarlsen commented Oct 18, 2021

bicarlsen commented Oct 18, 2021

choldgraf commented Oct 25, 2021

bicarlsen commented Oct 25, 2021 •

edited

Loading

bicarlsen commented Nov 30, 2021

choldgraf left a comment

choldgraf Nov 30, 2021

choldgraf Nov 30, 2021

bicarlsen commented Dec 1, 2021

rkdarst commented Mar 22, 2022

rkdarst commented Jun 10, 2022

rkdarst left a comment

rkdarst Jun 16, 2022

rkdarst Jun 16, 2022

rkdarst Jun 16, 2022

rkdarst Jun 16, 2022

rkdarst commented Jun 16, 2022

rkdarst commented Sep 13, 2022

	By default, ``sphinx-copybutton`` will exclude text that is not selectable by the user. Unselectable text is determined with the CSS style
	By default, ``sphinx-copybutton`` will exclude text that is not selectable by the user.
	Unselectable text is determined with the CSS style

		@@ -372,6 +372,17 @@ In this case, all elements that match ``your.selector`` will have a copy button
		added to them.


		Exclude text


		copybutton_exclude = '.exclude_me'

		By default `.linenos` is excluded. If you specify the `copybutton_exclude` option it will replace the default.

	By default `.linenos` is excluded. If you specify the `copybutton_exclude` option it will replace the default.
	By default `.linenos, .gp` are excluded. If you specify the `copybutton_exclude` option it will replace the default.
	`.linenos` is the Sphinx default for line numbers, and `.gp` is Pygments (the highlighter of Sphinx) for "prompt", e.g. `$` in a shell-session.

	app.add_config_value("copybutton_exclude", ".linenos", "html")
	app.add_config_value("copybutton_exclude", ".linenos, .gp", "html")

Exclude unselectable text from being copied #138

Exclude unselectable text from being copied #138

Conversation

bicarlsen commented Sep 20, 2021

welcome bot commented Sep 20, 2021

bicarlsen commented Sep 20, 2021

theletterf commented Oct 11, 2021

choldgraf left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choldgraf commented Oct 11, 2021 • edited Loading

bicarlsen commented Oct 11, 2021

bicarlsen commented Oct 11, 2021

choldgraf commented Oct 13, 2021

bicarlsen commented Oct 18, 2021

bicarlsen commented Oct 18, 2021

choldgraf commented Oct 25, 2021

bicarlsen commented Oct 25, 2021 • edited Loading

bicarlsen commented Nov 30, 2021

choldgraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bicarlsen commented Dec 1, 2021

rkdarst commented Mar 22, 2022

rkdarst commented Jun 10, 2022

rkdarst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkdarst commented Jun 16, 2022

rkdarst commented Sep 13, 2022

choldgraf left a comment •

edited

Loading

choldgraf commented Oct 11, 2021 •

edited

Loading

bicarlsen commented Oct 25, 2021 •

edited

Loading