-
Notifications
You must be signed in to change notification settings - Fork 255
feat(clipboard): support input elements in shadow DOM #1033
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
Conversation
|
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:
|
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.
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 Report
@@ 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
... and 12 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@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 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 is there something I can do to help get this, #1038, #1039, #1040 merged?
@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.
@ph-fritsche Sorry, I got somewhat swallowed. I would like to help. I will try to rebase this to get started.
Co-authored-by: Philipp Fritsche <ph.fritsche@gmail.com>
...DOM in setup(). Removed old custom elements test
Co-authored-by: Philipp Fritsche <ph.fritsche@gmail.com>
Co-authored-by: Philipp Fritsche <ph.fritsche@gmail.com>
Co-authored-by: Philipp Fritsche <ph.fritsche@gmail.com>
cbe1391 to
7f803bf
Compare
Uh oh!
There was an error while loading. Please reload this page.
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?) (削除ここまで)