sphinx-copybutton

sphinx-copybutton copied to clipboard

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.

Thanks for submitting your first pull request! You are awesome! :hugs:
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! :tada:

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.

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

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 :-)

https://github.com/executablebooks/sphinx-copybutton/pull/140

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.

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.

I do like the simplicity of the implementation in https://github.com/executablebooks/sphinx-copybutton/pull/138#issuecomment-940149343 so unless we can see any downsides to that approach, I'd be fine going that direction

@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.

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?

@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

@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.

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

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.

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).

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)

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)

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