Skip to content

[Security Solution][Resolver] Replace copy-to-clipboard with native navigator.clipboard#80193

Merged
michaelolo24 merged 4 commits intoelastic:masterfrom
michaelolo24:use-native-copy
Oct 13, 2020
Merged

[Security Solution][Resolver] Replace copy-to-clipboard with native navigator.clipboard#80193
michaelolo24 merged 4 commits intoelastic:masterfrom
michaelolo24:use-native-copy

Conversation

@michaelolo24
Copy link
Copy Markdown
Contributor

@michaelolo24 michaelolo24 commented Oct 12, 2020 •
edited
Loading

Summary

This replaces the clipboard package with the native navigator.clipboard api. For reference on writeText() https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText and browser availability: https://caniuse.com/?search=writetext

@michaelolo24 michaelolo24 added Feature:Resolver Security Solution Resolver feature Team:Threat Hunting Security Solution Threat Hunting Team v7.11.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Oct 12, 2020
@michaelolo24 michaelolo24 changed the title [use native copy functionality [Security Solution][Resolver] Replace copy-to-clipboard with native navigator.clipboard Oct 12, 2020
@michaelolo24 michaelolo24 marked this pull request as ready for review October 12, 2020 17:26
@michaelolo24 michaelolo24 requested review from a team as code owners October 12, 2020 17:26
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 12, 2020

Pinging @elastic/endpoint-app-team (Feature:Resolver)

@michaelolo24 michaelolo24 marked this pull request as draft October 12, 2020 17:26
@michaelolo24 michaelolo24 marked this pull request as ready for review October 12, 2020 17:36
@michaelolo24
Copy link
Copy Markdown
Contributor Author

michaelolo24 commented Oct 12, 2020

@elasticmachine merge upstream

bkimmel
bkimmel reviewed Oct 12, 2020
View reviewed changes
x-pack/plugins/security_solution/public/resolver/view/panels/copyable_panel_field.tsx Outdated
Copy link
Copy Markdown
Contributor

@bkimmel bkimmel Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ I think you could either put the text that's going to be copied in that aria-label like Copy ${textToCopy} to clipboard or you could maybe add an aria-description="${texttoCopy}" .

Copy link
Copy Markdown
Contributor Author

@michaelolo24 michaelolo24 Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aria-description doesn't seem to be valid. I can change it to Copy ${textToCopy} to clipboard?

oatkiller
oatkiller reviewed Oct 12, 2020
View reviewed changes
x-pack/plugins/security_solution/public/resolver/view/panels/copyable_panel_field.tsx Outdated
Copy link
Copy Markdown
Contributor

@oatkiller oatkiller Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you just put this inline since its a string, or otherwise use a camel case variable name.

Copy link
Copy Markdown
Contributor Author

@michaelolo24 michaelolo24 Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep

oatkiller
oatkiller reviewed Oct 12, 2020
View reviewed changes
x-pack/plugins/security_solution/public/resolver/view/panels/copyable_panel_field.tsx Outdated
Copy link
Copy Markdown
Contributor

@oatkiller oatkiller Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this data-test-subj correct? In the old code it was namespaced under resolver:...

Copy link
Copy Markdown
Contributor Author

@michaelolo24 michaelolo24 Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yea, I fixed it.

oatkiller
oatkiller reviewed Oct 12, 2020
View reviewed changes
x-pack/plugins/security_solution/public/resolver/view/panels/copyable_panel_field.tsx Outdated
Copy link
Copy Markdown
Contributor

@oatkiller oatkiller Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose in preventing default and stopping propagation?

Copy link
Copy Markdown
Contributor Author

@michaelolo24 michaelolo24 Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copypasta. stopPropagation would be useful if we also had the drag and drop functionality, but alas we aren't there yet

oatkiller
oatkiller reviewed Oct 12, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change. I added a few comments. Let me know if they don't make sense.

x-pack/plugins/security_solution/public/resolver/view/panel.test.tsx Outdated
Copy link
Copy Markdown
Contributor

@oatkiller oatkiller Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing this, would you add writeText to the side effect simulator?

Copy link
Copy Markdown
Contributor Author

@michaelolo24 michaelolo24 Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Copy Markdown
Contributor Author

@michaelolo24 michaelolo24 Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

oatkiller
oatkiller approved these changes Oct 13, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for this

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Oct 13, 2020

💛 Build succeeded, but was flaky

Test Failures

Creates a timeline.Timelines Creates a timeline

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has failed 9 times on tracked branches: https://github.com/elastic/kibana/issues/79389

CypressError: `cy.wait()` could not find a registered alias for: `@timeline`.
You have not aliased anything yet.
    at aliasNotFoundFor (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:150289:22)
    at $Cy.getAlias (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:150252:7)
    at waitForXhr (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:159965:25)
    at http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:160054:14
    at tryCatcher (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:10325:23)
    at MappingPromiseArray._promiseFulfilled (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:7445:38)
    at MappingPromiseArray.PromiseArray._iterate (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:8647:31)
    at MappingPromiseArray.init (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:8611:10)
    at MappingPromiseArray._asyncInit (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:7414:10)
    at _drainQueueStep (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:5036:12)
    at _drainQueue (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:5025:9)
    at Async.../../node_modules/bluebird/js/release/async.js.Async._drainQueues (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:5041:5)
    at Async.drainQueues (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:4911:14)
From Your Spec Code:
    at Context.eval (http://localhost:6151/__cypress/tests?p=cypress/integration/timeline_creation.spec.ts:13497:35)

Metrics [docs]

async chunks size

id before after diff
securitySolution 10.6MB 10.6MB +3.2KB

History

  • 💚 Build #81159 succeeded 6d517ab20b3ab39c6bc17ec9d97a1e7f48992c1f
  • 💔 Build #81151 failed fadbe51525628114ca05ac2b6383e27313c43b7f
  • 💔 Build #81143 failed 8fdeef2ed4607554afe5d818f85a0282557cde10

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@michaelolo24 michaelolo24 merged commit 5bf7966 into elastic:master Oct 13, 2020
@michaelolo24 michaelolo24 deleted the use-native-copy branch October 13, 2020 15:41
@michaelolo24 michaelolo24 mentioned this pull request Oct 13, 2020
Merged
michaelolo24 added a commit to michaelolo24/kibana that referenced this pull request Oct 13, 2020
@michaelolo24
[Security Solution][Resolver] Replace copy-to-clipboard with native n…
e1442e2 
…avigator.clipboard (elastic#80193)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 13, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ilm/migrate-h…
200b6d4 
…otphase-to-formlib

* 'master' of github.com:elastic/kibana: (59 commits)
  [Security Solution][Resolver] Replace copy-to-clipboard with native navigator.clipboard (elastic#80193)
  [Security Solution] Reduce initial bundle size (elastic#78992)
  [Security Solution][Resolver] Fix Resize node box-shadow bug (elastic#80223)
  Move observability content (elastic#79978)
  skip flaky suite (elastic#79389)
  removing kibana_datatable` in favor of `datatable` (elastic#75184)
  [ML] Fixes for anomaly swim lane  (elastic#80299)
  [Lens] Smokescreen lens test unskip (elastic#80190)
  Improved AlertsClient tests structure by splitting a huge alerts_client.tests.ts file into a specific files defined by its responsibility. (elastic#80088)
  [APM] React key warning when opening popover with external resources (elastic#80328)
  [Step 1] use Observables on server search API (elastic#79874)
  Apply back pressure in Task Manager whenever Elasticsearch responds with a 429 (elastic#75666)
  [Lens] Leverage original http request error (elastic#79831)
  [Security Solution][Case] Improve ServiceConnectorCaseParams type (elastic#80109)
  [SECURITY_SOLUTION] Fix query on alert histogram (elastic#80219)
  [DOCS] Update ingest node pipelines doc (elastic#79187)
  [Ingest Manager] Split up OpenAPI spec file  (elastic#80107)
  [SECURITY_SOLUTION][ENDPOINT] Fix label on Trusted App create name field (elastic#80001)
  [Ingest Manager] Fix agent policy bump revision to create only one POLICY_CHANGE action (elastic#80081)
  Grid layout fixes (elastic#80305)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared/data_tier_allocation_field.tsx
#	x-pack/plugins/index_lifecycle_management/public/shared_imports.ts
michaelolo24 added a commit that referenced this pull request Oct 13, 2020
@michaelolo24
[7.x] [Security Solution][Resolver] Replace copy-to-clipboard with na…
b81bce9 
…tive navigator.clipboard (#80193) (#80369)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@oatkiller oatkiller oatkiller approved these changes

@bkimmel bkimmel bkimmel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:Resolver Security Solution Resolver feature release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team v7.11.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@michaelolo24 @elasticmachine @kibanamachine @oatkiller @bkimmel