Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code copy button should check for secure context and indicate to the user if it's not available #1202

Closed
rmoff opened this issue Mar 22, 2023 · 9 comments
Labels
enhancement status: ready to implement Issues that can be actively worked on, and need an implementation!

Comments

@rmoff
Copy link
Contributor

rmoff commented Mar 22, 2023

IIUC the writeText function is only available when the page is viewed in a secure context (src, src).

This means that it fails in other situations, e.g. over HTTP. The following is logged in the console:

just-the-docs.js:479 Uncaught TypeError: Cannot read properties of undefined (reading 'writeText')

It would be useful for the code to detect if it is a secure context and if so show the copy icon greyed out with a tooltip to indicate why (or make it configurable with the other option being to not render it if not available).

@max06
Copy link
Contributor

max06 commented Mar 22, 2023

I'm not sure if it's actually worth it: writeText is available in secure context (and there's no reason to not run it secure nowadays) and when running on localhost, which should cover all usual circumstances.

Do you have an environment what's not fitting in here?

(Edit: And for your configuration option: enable_copy_code_button: true already exists.)

@mattxwang
Copy link
Member

Thank you for submitting an issue @rmoff! I appreciate your detail in linking various resources.

I see two possible paths forward:

  1. (easy) document this somewhere in our theme docs, discuss how it might happen (noting that many of our users are non-technical), and instruct users how to amend this (sounds like closer to what @max06 is looking for)
  2. (harder, but not by much) implement a first-class fix, similar to the one that @rmoff has proposed in the issue

I personally am leaning towards the second option; I think many theme users may not be familiar with what "secure context" even is. I imagine this is a one or two-liner.

(or make it configurable with the other option being to not render it if not available)

My understanding here is that this suggestion is to do this at page load; at build-time, we wouldn't know what users will be using the site (and thus, what context they're viewing from). I think this is reasonable, but perhaps less-preferred than the tooltip option - the end user may be able to debug it themselves if they know what's going on.

@max06
Copy link
Contributor

max06 commented Mar 23, 2023

Apologies - I noticed I sounded way too blunt in my first reply.

I'm afraid a fix implemented at page-load might cause irritation, for example if the button works on one machine (because local context) but doesn't on another one (eg through a local network). An additional "disabled" style for the button would probably work, though it might still never get seen by the page maintainer (uh, grammar 😲 - thx chatgpt!). I personally would prefer a defined state in this case.

My first comment was intended to point at the effort required for such change. If I remember correctly, we had one case of insecure context so far, and that happend to the dev implementing this button. This would be the second case, hence my question about more details about this specific environment.

Apologies again, and have a great evening! 😴

@mattxwang
Copy link
Member

mattxwang commented Mar 28, 2023

No problem at all @max06, I didn't think you were too blunt - it was a good perspective to add!

I think better understanding @rmoff's use-case may also help, like you've stated. I'm assuming that you're serving Just the Docs over HTTP, and that's causing the error - are there any other instances that you've run into? This doesn't affect me personally, but I wonder if local development over http is the blocker here.

(given that many major browsers are pushing users away from HTTP, I'll have to think a little bit about how to best integrate this change gracefully)

@mattxwang
Copy link
Member

mattxwang commented Mar 28, 2023

And, just for completeness' sake - happy to review a PR for this. I'd imagine the minimum required change would be:

// just-the-docs.js
{%- if site.enable_copy_code_button != false %}

jtd.onReady(function(){
  if (!window.isSecureContext) return; // optionally, log something first
  // ...

In this case, the button is not rendered at all. But, this may not be the best user experience.

As I've done some more looking, other approaches that may work:

  • like mentioned, adding a disabled state + tooltip (though tooltips can be finicky)
  • or, when clicking the button, instead of showing the "successful copy" icon, display a message on the screen about the copy not working
  • potentially asking the browser for permissions first (it's unclear exactly how this would resolve the problem)

If anything, I'm leaning slightly towards the second option?

I would prefer to not use solutions that rely on deprecated APIs (ex document.execCommand) or "hacky" solutions that create offscreen elements (e.g. the off-screen <textarea> hack) or iframes, since those seem disingenuous to me.

@mattxwang mattxwang added the status: needs discussion Issues that need more discussion before they can be properly triaged. label Mar 29, 2023
@rmoff
Copy link
Contributor Author

rmoff commented Mar 29, 2023

Hi both, thanks for the discussion and apologies for the delay replying.

@max06 you didn't sound blunt, no worries :)

Do you have an environment what's not fitting in here?

Yes, my local machine. Running Jeckyll --serve with Docker it launches with this message:

[…]
 Auto-regeneration: enabled for '/srv/jekyll'
LiveReload address: http://0.0.0.0:35729
    Server address: http://0.0.0.0:4000
  Server running... press ctrl-c to stop.

Thus I click on http://0.0.0.0:4000 and there's no secure context. If I manually browse to localhost it works fine, sure.

  • I could get jekyll to bind to localhost instead of 0.0.0.0 but that'll have its own implications (e.g. portability), and seems overkill to fix this issue
  • I could remember to use localhost when browsing locally, but when am I going to remember that…

In practice I think there's several things to do in order of effort which would all be useful:

  1. Document this behaviour somewhere (I'm guessing Google will find this now, but properly in the docs would be useful)
  2. Do something page-side to address it.
    a. low effort: catch the condition and log a proper message for it
    b. more effort: do something on the page render as discussed e.g. disable it if not secure context (with a tooltip & log message to say why), or tooltip when clicked to say why it's not going to work, etc.

2b can be iterated on and is arguably overkill for something which is only going to be affecting local devs and not end-users of JtD (assuming best practice of serving over HTTPS is followed).

(1) I can do a PR for now and (2a) is beyond my skillset but would be useful to do if everyone agrees it :)

@mattxwang
Copy link
Member

I think 1 + 2a sounds great! If you'd like to submit a PR for 1, glad to review it; if not, I can add it to my backlog. I can take on 2a (unless someone else is interested - in that case, go for it!)

@rmoff
Copy link
Contributor Author

rmoff commented Apr 6, 2023

Sounds good, I'll do that.

@mattxwang mattxwang added status: ready to implement Issues that can be actively worked on, and need an implementation! and removed status: needs discussion Issues that need more discussion before they can be properly triaged. labels Apr 8, 2023
mattxwang pushed a commit that referenced this issue Apr 20, 2023
mattxwang pushed a commit that referenced this issue Apr 22, 2023
@mattxwang
Copy link
Member

Closed by #1225, #1226.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement status: ready to implement Issues that can be actively worked on, and need an implementation!
Projects
None yet
Development

No branches or pull requests

3 participants