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

feat(clipboard): support input elements in shadow DOM #1033

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

Christian24
Copy link

@Christian24 Christian24 commented Aug 12, 2022

What:
This allows support for copy, cut & paste in inputs in open shadow dom custom elements.

Why:
Because I filled #1025 and #1026. This doesn't completely resolve #1026 though, however it should ease the situation when using native inputs in custom elements.

How:
Changed the implementation of getting the activeElement.

Checklist:

- [ ] Documentation (I am uncertain about this, maybe the state of #1026 should be at some point be part of the documentation?)

  • Tests
  • Ready to be merged

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 12, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a342415:

Sandbox Source
userEvent-dom Configuration
userEvent-react Configuration

src/clipboard/copy.ts Outdated Show resolved Hide resolved
tests/dom/customElement.ts Outdated Show resolved Hide resolved
tests/dom/customElement.ts Outdated Show resolved Hide resolved
tests/dom/customElement.ts Outdated Show resolved Hide resolved
@ph-fritsche
Copy link
Member

ph-fritsche commented Aug 13, 2022

First of all: Thanks for contributing to this library. This is much appreciated ❤️

I think we should add tests to tests/clipboard/*.ts testing just the interaction that happens on the .copy()/.cut()/.paste().
If we feel like .click() and .keyboard() should have additional tests too, we should add them there.
Testing the features separately makes it much easier for future developers to identify if a test breakage is expected on a change. It's really bad if you're working on something, 50 tests break and you have to spend extra time to figure out which of those 50 tests have false assumptions that you're just about to correct and which actually warn you that you're in the process of breaking something else by mistake.

Also the thing we just imply here is the internal abstractions working correctly. So this is probably where the real testing should take place to pave the way for possible other changes required by #1026.
So we need to test util/focus/getActiveElement and the document/copySelection and event/input to properly interact with shadow DOM.
If we know about a limitation of the current change and we don't want to wait for all aspects to be resolved, this can also mean adding some tests with comments that outline what is missing.

@Christian24
Copy link
Author

Christian24 commented Aug 14, 2022

I added some very simple tests for getActiveElement. So far I have only dealt with input related stuff. This is the area most relevant to me.

Does this look good to you?

@codecov
Copy link

codecov bot commented Aug 14, 2022

Codecov Report

Merging #1033 (cbe1391) into main (d7483f0) will increase coverage by 0.53%.
The diff coverage is 100.00%.

❗ Current head cbe1391 differs from pull request most recent head a342415. Consider uploading reports for the commit a342415 to get more accurate results

@@             Coverage Diff             @@
##             main     #1033      +/-   ##
===========================================
+ Coverage   99.46%   100.00%   +0.53%     
===========================================
  Files          89        88       -1     
  Lines        2067      2061       -6     
  Branches      701       698       -3     
===========================================
+ Hits         2056      2061       +5     
+ Misses         11         0      -11     
Files Changed Coverage Δ
src/clipboard/copy.ts 100.00% <100.00%> (ø)
src/clipboard/cut.ts 100.00% <100.00%> (ø)
src/clipboard/paste.ts 100.00% <100.00%> (ø)
src/event/focus.ts 100.00% <100.00%> (ø)
src/utils/focus/getActiveElement.ts 100.00% <100.00%> (ø)

... and 12 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/clipboard/copy.ts Outdated Show resolved Hide resolved
src/clipboard/copy.ts Outdated Show resolved Hide resolved
tests/_helpers/setup.ts Outdated Show resolved Hide resolved
tests/_helpers/shadow-input.ts Outdated Show resolved Hide resolved
tests/clipboard/copy.ts Outdated Show resolved Hide resolved
tests/utility/type.ts Outdated Show resolved Hide resolved
tests/utils/focus/getActiveElement.ts Outdated Show resolved Hide resolved
tests/utils/focus/getActiveElement.ts Outdated Show resolved Hide resolved
tests/utils/focus/getActiveElement.ts Outdated Show resolved Hide resolved
tests/utils/focus/getActiveElement.ts Outdated Show resolved Hide resolved
src/clipboard/copy.ts Outdated Show resolved Hide resolved
tests/_helpers/setup.ts Outdated Show resolved Hide resolved
tests/_helpers/shadow-input.ts Outdated Show resolved Hide resolved
tests/utility/type.ts Outdated Show resolved Hide resolved
tests/utils/focus/getActiveElement.ts Outdated Show resolved Hide resolved
tests/utils/focus/getActiveElement.ts Outdated Show resolved Hide resolved
tests/utility/type.ts Outdated Show resolved Hide resolved
@ph-fritsche
Copy link
Member

@Christian24 Thanks for your work on this. I'll review again with fresh eyes and try to figure out if - based on what we learned here - there are some easy improvements towards full shadow DOM support that we could add to that release as well.

@Christian24
Copy link
Author

@Christian24 Thanks for your work on this. I'll review again with fresh eyes and try to figure out if - based on what we learned here - there are some easy improvements towards full shadow DOM support that we could add to that release as well.

Thanks for taking the time to review it!

@ph-fritsche ph-fritsche changed the title Support cut, copy and paste on open shadow DOM inputs feat(clipboard): support input elements in shadow DOM Aug 18, 2022
@Christian24
Copy link
Author

@ph-fritsche is there something I can do to help get this, #1038, #1039, #1040 merged?

@ph-fritsche
Copy link
Member

@Christian24 I'd like to resolve #1019 before merging the pending PRs so that we have proper testing utils in place to verify our changes and any future bug reports. Also the CI is broken - as it seems due to conflicts with more recent versions of Typescript and Eslint. We need to fix these before we can move forward.
I opened a draft for updating the test environment in #1081 . Any help on this would be highly welcome.

@Christian24
Copy link
Author

@ph-fritsche Sorry, I got somewhat swallowed. I would like to help. I will try to rebase this to get started.

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

Successfully merging this pull request may close these issues.

Support open shadow DOM components
2 participants