[BUG] Share > Copy link broken in Safari

[wkruse]

Describe the bug

Share > Copy link is broken in Safari. When you try to copy a link, you hear a sound and see a popup which says Copied, but nothing is copied into the clipboard.

To Reproduce
Steps to reproduce the behavior:

1. Go to Discover
2. Click on Share > Copy link

Expected behavior
Copy link should work in Safari.

OpenSearch Version
1.3.2
2.0.0

Dashboards Version
1.3.2
2.0.0

Plugins
Default from the corresponding official Docker images

Host/Environment (please complete the following information):

• OS: macOS Catalina 10.15.7
• Safari 15.5 (15613.2.7.1.9, 15613)

Additional context

In Google Chrome 102.0.5005.61 it works without issues.
In Firefox 101.0 it works without issues.

No logs in the Safari Developer Tools > Console, besides the two probably unrelated, which appear on page reload

[Error] Refused to execute a script because its hash, its nonce, or 'unsafe-inline' does not appear in the script-src directive of the Content Security Policy. (discover, line 352)
[Log] ^ A single error about an inline script not firing due to content security policy is expected! (bootstrap.js, line 43)

Related to #689.

[joshuarrrr]

Great find! From a quick look at the source code, I suspect this bug may actually be in the EUI component (<EuiCopy>), rather than its implementation here:

OpenSearch-Dashboards/src/plugins/share/public/components/url_panel_content.tsx

Lines 128 to 152 in b2cfb6e

	<EuiCopy textToCopy={this.state.url || ''} anchorClassName="eui-displayBlock">
	{(copy: () => void) => (
	<EuiButton
	fill
	fullWidth
	onClick={copy}
	disabled={this.state.isCreatingShortUrl || this.state.url === ''}
	data-share-url={this.state.url}
	data-test-subj="copyShareUrlButton"
	size="s"
	>
	{this.props.isEmbedded ? (
	<FormattedMessage
	id="share.urlPanel.copyIframeCodeButtonLabel"
	defaultMessage="Copy iFrame code"
	/>
	) : (
	<FormattedMessage
	id="share.urlPanel.copyLinkButtonLabel"
	defaultMessage="Copy link"
	/>
	)}
	</EuiButton>
	)}
	</EuiCopy>

[kavilla]

Hello @wkruse!

Thanks for opening!

@joshuarrrr is correct. EuiCopy eventually calls copy.js which calls copy_to_clipboard.js this utilizes document.execCommand('copy') to copy the text which is deprecated.

So Safari must have removed support for this and eventually other browsers might as well. Unfortunately this is in the design system library. So should be fixed when the official fork is produced.

Tagging @opensearch-project/opensearch-ux for visibility.

[kgcreative]

+1 to make this as a design system priority. There's no visual/UX changes, but we want to make sure we're doing the right thing from a functionality perspective. @joshuarrrr + @kavilla - in addition to the deprecation of using document.execCommand, there's a new Navigator.clipboard api (https://developer.mozilla.org/en-US/docs/Web/API/Clipboard) -- we may need to serve alternative methods depending on browser version as it's not yet fully compatible everywhere

[joshuarrrr]

Yep, for browser compatibility we'd need to support both: https://stackoverflow.com/a/60239236

[AMoo-Miki]

To me, this sounds like a CSP issue and not a compatibility problem since according to caniuse, all browsers that matter continue to support this as of now.

[kgcreative]

Thanks for the update @AMoo-Miki. We should prioritize fixing this in OUI. What are the next steps?

[KrooshalUX]

Closing this issue in favor of a fix by OUI - issue here: opensearch-project/oui#314

[AMoo-Miki]

This is not an issue of OUI. OuiCopy is correctly triggering document.execCommand('copy') but securityDashboards interferes with it.

1. OuiCopy creates an invisible span, throws the URL being shared into it, selects it, and triggers "copy".
2. securityDashboards listens to the copy event to inject tenants into the URL being copied and it does this by attempting to change the text inside the invisible span using element.textContent.

The problem is that Firefox, Chrome, and Safari, each react differently. If some text in a node is "selected":

• Firefox: changing the node's content using textContent does nothing; the original text remains in the node and it continues to be selected.
• Chrome: the node's content changes when setting textContent but also the result will be selected.
• Safari: the node's content changes when setting textContent but after that, nothing will be selected.

As a result, when the button to copy link is hit:

• Firefox: security doesn't get to update the URL and the original is copied.
• Chrome: security gets to change the URL and the new one is copied.
• Safari: since nothing is selected, nothing is copied.

This issue should be raised on security plugin. The appropriate solution would be for securityDashboards to change their listener to:

1. find the parent node of the selection,
2. deselect the text (so Firefox can change the node's content),
3. update the node's content, and
4. select the node's content

When the listener is done, the copy would occur.

[joshuarrrr]

@opensearch-project/triage Can you please transfer to https://github.com/opensearch-project/security-dashboards-plugin

[cwperks]

Can an admin transfer this to the security-dashboards-plugin repo?

[leanneeliatra]

I will pick this up @davidlago, please assign me.
I will focus on closing my in review items & then pick this up in between.

[leanneeliatra]

This ticket has been picked back up.
P.S Can an admin transfer this to the security-dashboards-plugin repo?

[leanneeliatra]

Note: In recent opensearch versions this issue is fixed.
Determining when fix happened / Reproducing in version specified - this is incorrect. I am currently implementing a fix in the security dashboard plugin for this issue.

[kgcreative]

@leanneeliatra + @AMoo-Miki -- Do we know which PR addressed this issue? i'd love to link it here.

[leanneeliatra]

@leanneeliatra + @AMoo-Miki -- Do we know which PR addressed this issue? i'd love to link it here.

After further investigation I was mistaken! I'm still working on a fix.

[leanneeliatra]

The solution for this issue is in progress.

[leanneeliatra]

I'm attempting to solve this issue and have read the comments and ticket.
I tried to use the new Navigator.clipboard api (https://developer.mozilla.org/en-US/docs/Web/API/Clipboard) (this was mentioned above but may not be the root of the issue.). When I attempt to copy using this method I get the error

NotAllowedError: The request is not allowed by the user agent or the platform in the current context, possibly because the user denied permission.

This approach may be a no go, logging for recording purposes.

[AMoo-Miki]

The appropriate solution would be for securityDashboards to change their listener to:

1. find the parent node of the selection,
2. deselect the text (so Firefox can change the node's content),
3. update the node's content, and
4. select the node's content

Have you by chance tried my suggestion above? I believe with that specific sequence, the problem would be worked around in all the browsers:

The problem is that Firefox, Chrome, and Safari, each react differently. If some text in a node is "selected":

1. Firefox: changing the node's content using textContent does nothing; the original text remains in the node and it continues to be selected.
2. Chrome: the node's content changes when setting textContent but also the result will be selected.
3. Safari: the node's content changes when setting textContent but after that, nothing will be selected.

[leanneeliatra]

The appropriate solution would be for securityDashboards to change their listener to:

1. find the parent node of the selection,
2. deselect the text (so Firefox can change the node's content),
3. update the node's content, and
4. select the node's content

Have you by chance tried my suggestion above? I believe with that specific sequence, the problem would be worked around in all the browsers:

The problem is that Firefox, Chrome, and Safari, each react differently. If some text in a node is "selected":

1. Firefox: changing the node's content using textContent does nothing; the original text remains in the node and it continues to be selected.
2. Chrome: the node's content changes when setting textContent but also the result will be selected.
3. Safari: the node's content changes when setting textContent but after that, nothing will be selected.

Hi @AMoo-Miki I am in the process of looking into your solution at the moment. Can I check please, do you mean for me to update the selection range of the text, something like this: https://javascript.info/selection-range?
Thank you for the in depth explanation it's very helpful.

[AMoo-Miki]

Yes Leanne. The change I was proposing would go into security-dashboards-plugin where they manipulate the URL of the shareable.

1. Before the manipulation happens, we get the parentNode and
2. using Selection.removeAllRanges()[ref] we deselect it.
3. Then the content is allowed to change and
4. we select it back by creating a range, doing selectNode[ref], and then using addRange[ref] to select it.

[leanneeliatra]

Thank you very much for the recommendation, I have the fix added here: https://github.com/leanneeliatra/security-dashboards-plugin-fork/tree/safari-copy-link-fix

[leanneeliatra]

Update:

1. PR for bug fix generated Fix copy link issue in Safari security-dashboards-plugin#1633
2. Tests in progress

• Tests also needed for CI completion for bugfix (point 1)

[leanneeliatra]

Can I confirm both cypress tests and unit tests are required here please?
The change was: to fix a broken copy link button. In Safari only, when clicked, the button did not copy.
Some tickets have needed both, or one or the other so best to check. cc @peternied @cwperks @davidlago
Currently, there does not seem to be a unit test in place for this file 'public/services/shared-link.ts'.
I have started on the cypress test.
Also, just to confirm is the opensearch-dashboards-functional-test repository the correct place to add the cypress tests or should I add them to a different repository keeping in mind you are merging the tests and code at the moment? Thanks.

[leanneeliatra]

Hi, can someone assist in making sure I am creating the tests in the right repository and that both cypress, and unit tests are required for this ticket please? (I presume yes, but for my last ticket unit tests were not required, so best to check).

[peternied]

Thanks for the question - it depends 😆

Generally speaking every change should have a test associated with it. Sometimes a unit test is more than enough.

Another way you could approach this issue is by building a test case that fails and then making a fix that has the test passing.

[peternied]

I'd recommend creating a pull request and then you can get guidance based on the changes you're making.

[leanneeliatra]

Thanks Peter, yes good approach to create a test that fails, then fix until it is passing.

The tests are going well locally but if I try to push to my fork, I get many linting errors so I was just continuing with the test as it's still not fully complete yet.

This is what I get when attempting to push to my fork.

So yes the update is still the same:
Update:

1. PR for bug fix generated Fix copy link issue in Safari security-dashboards-plugin#1633
2. Tests in progress locally, blocked by linting errors when attempting push.

[leanneeliatra]

Update:

1. Bugfix complete Fix copy link issue in Safari security-dashboards-plugin#1633
2. integration tests in progress Integration tests for broken safari link opensearch-dashboards-functional-test#940

[leanneeliatra]

Update

1. Bugfix complete Fix copy link issue in Safari security-dashboards-plugin#1633
2. integration tests complete for testing the functionality but small linting errors in progress. Integration tests for broken safari link opensearch-dashboards-functional-test#940
3. Jest unit test in progress Fix copy link issue in Safari security-dashboards-plugin#1633

[leanneeliatra]

PRs currently in review for merge.

[derek-ho]

Not able to replicate this issue on my local or in playground

[leanneeliatra]

Not able to replicate this issue on my local or in playground

Okay, some things that got me when I first tried to reproduce this, it must be done in Safari, safari is the only browser with this issue. Also, don't click the shortURL slider button. Just immediately click 'copy link'. Last thing, when I first did this I'm just so used to chrome, I went there first. Then when I realised and switched to Safari, the link was in the clipboard from Chrome, not safari, so when I copy and pasted I thought it worked! Copy something irrelevant (a word on the page, not a URL) then click copy link in Safari and the link should not copy. Just some small things that I was made aware of when first trying to reproduce in case they may be catching you, perhaps not.

Update: it seems @derek-ho was not able to reproduce this either.