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

navigator.clipboard is ONLY available in secure contexts (HTTPS) #9013

Closed
jkilzi opened this issue Apr 30, 2023 · 12 comments · Fixed by #10186
Closed

navigator.clipboard is ONLY available in secure contexts (HTTPS) #9013

jkilzi opened this issue Apr 30, 2023 · 12 comments · Fixed by #10186
Assignees
Labels
Milestone

Comments

@jkilzi
Copy link

jkilzi commented Apr 30, 2023

Ref: https://github.com/patternfly/patternfly-react/blob/ce1afc78845fba2fd75a62f3ed440b289b4d5b56/packages/react-core/src/components/ClipboardCopy/ClipboardCopy.tsx#LL14C1-L14C1

Therefore the clipboardCopyFunc can only be used in secured contexts. The code above doesn't work for pages served under HTTP for instance. (Bear in mind that this works properly for pages served locally under HTTP, e.g. http://localhost/...).

Please check out this article for more info.

@stale
Copy link

stale bot commented Sep 10, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the wontfix label Sep 10, 2023
@jkilzi
Copy link
Author

jkilzi commented Sep 11, 2023

Any news on this?

@stale stale bot removed the wontfix label Sep 11, 2023
Copy link

stale bot commented Nov 10, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the wontfix label Nov 10, 2023
@jkilzi
Copy link
Author

jkilzi commented Nov 12, 2023

Don't you dare stale-bot!

@stale stale bot removed the wontfix label Nov 12, 2023
Copy link

stale bot commented Jan 11, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the wontfix label Jan 11, 2024
@stale stale bot closed this as completed Jan 26, 2024
@tlabaj tlabaj added Pinned and removed wontfix labels Jan 30, 2024
@tlabaj tlabaj reopened this Jan 30, 2024
@kmcfaul kmcfaul modified the milestones: Prioritized Backlog, 2024.Q1 Mar 12, 2024
@wise-king-sullyman
Copy link
Contributor

Hello @jkilzi, since there are some downsides with document.execCommand I think many of our consumers wouldn't want us to just move to it over the clipboard API.

We could potentially add a fallback or optional prop to use that older API, but since execCommand is officially deprecated I would honestly be a good bit hesitant to do so.

If I'm understanding things correctly you can specify an alternative copy function like that using our onCopy prop if you need it, am I incorrect in that?

@jkilzi
Copy link
Author

jkilzi commented Mar 19, 2024

Hello @jkilzi, since there are some downsides with document.execCommand I think many of our consumers wouldn't want us to just move to it over the clipboard API.

We could potentially add a fallback or optional prop to use that older API, but since execCommand is officially deprecated I would honestly be a good bit hesitant to do so.

If I'm understanding things correctly you can specify an alternative copy function like that using our onCopy prop if you need it, am I incorrect in that?

IMHO you still need a polyfill, unless PatternFly can only be used with the same browsers that support the Clipboard API (if that's the case stop reading and close the issue 😉).
Unfortunately you don't really have a choice if you still want to offer this feature. The MUI folks have implemented that polyfill here.
After all the minimum expectation one can have is to to have this documented as a known limitation and add pointer to MDN docs explaining the situation for consumers of this component, otherwise you send the poor developer into an annoying research journey every time somebody complains about issues related to the clipboard button.
Delegating the implementation to the users through the onCopy prop won't help because it is supposed to be triggered after the Clipboard API has been called (what if you cannot call it at all?). In this context I believe that if the design system is offering a clipboard-copy functionality then taking care of making the feature cross-browser compatible is not a task that should be delegated to the users of the DS.

@wise-king-sullyman
Copy link
Contributor

IMHO you still need a polyfill, unless PatternFly can only be used with the same browsers that support the Clipboard API

The clipboard API has been supported in all of our supported browsers since 2020.

That doesn't mean we won't at least consider adding a polyfill since, as you've pointed out, non secure contexts could still be an issue. To be totally honest though I would be quite surprised if teams are deploying new production sites without HTTPS.

I would say I'm still leaning away from including it in PF unless there's an approach which doesn't require using deprecated browser behavior, but I'll bring all of this to the team at our standup today and let you know where we land.

After all the minimum expectation one can have is to to have this documented as a known limitation and add pointer to MDN docs explaining the situation for consumers of this component, otherwise you send the poor developer into an annoying research journey every time somebody complains about issues related to the clipboard button.

If we elect to not add a polyfill I would definitely be on board with us adding some documentation (potentially a warning in the console?) or something to help consumers know how to proceed.

Delegating the implementation to the users through the onCopy prop won't help because it is supposed to be triggered after the Clipboard API has been called

Also I believe that the onCopy prop totally replaces the functionality of our internal copy function rather than triggering after the clipboard call, is that not the behavior you see? If that isn't the case I think a change there would make sense.

@jkilzi
Copy link
Author

jkilzi commented Mar 19, 2024

My apologies, I double checked the onCopy, and you are right, users can override it entirely.

I must say, why would PF allow the dev/user to implement the "low-level" copy to clipboard function themselves? It is like allowing the driver to mess with the engine when all they want is just to drive... Anyway, IMO I wouldn't expose the onCopy at all, instead I would offer an "onBeforeCopy" and "onAfterCopy" that trigger before anything is copied (or after that) in this way PF remains in control of how the copy-to-clipboard should be performed under the hood in a way that guarantees it will work in different browsers. Makes sense?

@wise-king-sullyman
Copy link
Contributor

The team feels that since the polyfill requires deprecated browser functionality it would be best to leave implementations of it consumer side with the onCopy prop. I'll be adding a console warning in the event that navigator.console is undefined though, and trying to clarify what the onCopy prop does in our documentation.

I must say, why would PF allow the dev/user to implement the "low-level" copy to clipboard function themselves? It is like allowing the driver to mess with the engine when all they want is just to drive.

For situations just like this actually, if a consumer needs to support browsers outside of the PF support window, or just generally handle the clipboard copying in a way that we don't, the prop allows that flexibility.

Anyway, IMO I wouldn't expose the onCopy at all, instead I would offer an "onBeforeCopy" and "onAfterCopy" that trigger before anything is copied (or after that) in this way PF remains in control of how the copy-to-clipboard should be performed under the hood in a way that guarantees it will work in different browsers.

I think replacing the onCopy prop is something that would need to happen in a breaking change, would you mind opening a separate issue for that so that it can be discussed on its own and properly tracked/prioritized if it's something the team does decide to do?

@jkilzi
Copy link
Author

jkilzi commented Mar 20, 2024

I'll be adding a console warning in the event that navigator.console is undefined though, and trying to clarify what the onCopy prop does in our documentation.

I believe you meant navigator.clipboard 😉. But in general I agree with you.
Despite that IMHO (although this is a different topic...) if PF only supports only latest browsers versions it is not reasonable to expect that users will always be able to upgrade their browsers (think about stations running in disconnected environments, or firewalled... apps using PF will just won't work if the browser is "old", perhaps supporting the latest -1 or -2 version of each major browser would make more sense...)

would you mind opening a separate issue for that so that it can be discussed on its own

Sure, I'll do with pleasure, if time allows me... 😉

@wise-king-sullyman
Copy link
Contributor

Yes, navigator.clipboard is 100% what I meant 🙂

While we guarantee support for the latest browsers that of course doesn't explicitly mean that we won't consider changes to support non-latest browsers, but we do have to weigh the pros and cons of such changes.

If the clipboard API wasn't available on Chrome last month it would've been a very different decision than it being available for the last 4-6 years.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants