Gh2172 Add `ShadowRoot()` as a chainable Selector method #5560

mostlyfabulous · 2020-09-20T04:25:49Z

Purpose

Extend the Selector API to have a shadowRoot function that returns the elements inside the shadowRoot when the shadow DOM is open. These elements are only otherwise accessible with the shadowRoot property when using statements like document.querySelector('css-selector').shadowRoot.querySelectorAll('css-selector'); in a Selector, but this creates Selector chaining problems if there are child/nested elements with a shadowRoot on them as well.

e.g.

Selector('elem-with-shadowRoot').child('p').shadowRoot.withText('text'); will not work.

This PR should let developers chain existing Selector statements that target elements in the shadowRoot.

Approach

Add a new API shadowRoot that can be applied to a Selector which returns the contents of an element's shadowRoot if it exists and null otherwise.

References

#2172

https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_shadow_DOM

Pre-Merge TODO

Write tests for your proposed changes
Add API tests
Add TS definition
Make sure that existing tests do not fail

testcafe-build-bot · 2020-09-20T05:07:50Z

❌ Tests for the commit 28d84e4 have failed. See details:

AlexKamaev · 2020-09-22T09:57:51Z

@mostlyfabulous
Thank you for your contribution.
As I understand at this moment the PR is not ready yet. I tried the following test and it does not work as expected:

import { Selector } from 'testcafe';

fixture `Selector`
    .page `../pages/index.html`;

test('shadowRoot selector API tests', async t => {
    await t.click(Selector('#hasShadowDOM').shadowRoot().child('p').withText('Text should be found!'));
});

I debugged your code and found a couple of issues to fix.
I added some modifications to your code to make the test work. Please take a look at them: 0e9e4cf

We would appreciate it if you review the recommendations from the following comment: #2172 (comment)

testcafe-build-bot · 2020-09-22T10:54:59Z

❌ Tests for the commit 0e9e4cf have failed. See details:

testcafe-build-bot · 2020-09-26T08:28:36Z

✅ Tests for the commit 7272508 have passed. See details:

testcafe-build-bot · 2020-09-27T12:03:56Z

❌ Tests for the commit 3900fe5 have failed. See details:

mostlyfabulous · 2020-09-27T17:00:02Z

IE11 does not support `attachShadow` and must be polyfilled. Would prefer to skip IE11 tests for `shadowRoot` API method:

Unrelated PR build errors:

testcafe-build-bot · 2020-09-27T17:59:20Z

❌ Tests for the commit f2d84de have failed. See details:

AlexKamaev · 2020-09-28T08:40:41Z

@mostlyfabulous
I agree that we need just to skip tests in IE11, since it does not support the Shadow API. You can skip the test using the skip property instead of using the only property as described here:

testcafe/test/functional/fixtures/regression/gh-1366/test.js

Line 3 in d7918a1

    
           return runTests('testcafe-fixtures/index-test.js', 'choose value', { skip: 'ie,edge' });

Then, remove all IE related code in your test as follows:

const isIEFunction = ClientFunction(() => {

In addition, there is no need to create/copy tests for all selector properties. A test with one property is enough.

mostlyfabulous · 2020-09-29T03:52:02Z

Then, remove all IE related code in your test as follows:

Do you mean to say, wrap any code specific to IE in the function you provided?

testcafe-build-bot · 2020-09-29T04:38:02Z

❌ Tests for the commit c36f8bb have failed. See details:

testcafe-build-bot · 2020-09-29T07:04:40Z

❌ Tests for the commit 6c161aa have failed. See details:

AlexKamaev · 2020-09-29T07:46:51Z

Then, remove all IE related code in your test as follows:

Do you mean to say, wrap any code specific to IE in the function you provided?

No. I saw this function in your code. Probably you accidentally copied it from another test file. I mean, that we do not need any additional code for IE, since IE does not support shadow DOM.
The only thing that we need to do with IE for Shadow DOM tests - is only skipping IE in our test suites. Thus, this isIEFunction should be deleted.

mostlyfabulous · 2020-10-02T18:33:35Z

@testcafe-build-bot restart

AlexKamaev · 2020-10-05T08:06:46Z

@mostlyfabulous
I see the failed tests. This happens because the /fixtures/api/es-next/selector/pages/index.html page is used for Chrome, Firefox, and IE. So, I don't recommend adding your changes to the /fixtures/api/es-next/selector/pages/index.html page. Please create a separate page for your test.

Please also rebase your commit to the DevExpress/master branch since some npm packages need to be refreshed.

AlexKamaev · 2020-10-14T11:07:01Z

@mostlyfabulous
Hello. The PR is almost ready. Do you need any assistance to complete it? If you have any difficulties, feel free to ask questions here.

mostlyfabulous · 2020-10-19T17:20:43Z

I should be able to get round to the changes you suggested this week but feel free to finish the outstanding changes so this can be released!

AlexKamaev · 2020-10-20T07:21:36Z

Ok,
If I complete my current tasks before you're able to continue working this PR, I'll finish it.

testcafe-build-bot · 2020-10-26T22:12:15Z

❌ Tests for the commit 0a0c260 have failed. See details:

AlexKamaev · 2020-10-27T07:08:17Z

@testcafe-build-bot retest

testcafe-build-bot · 2020-10-27T08:00:51Z

✅ Tests for the commit 0a0c260 have passed. See details:

AndreyBelym

An excellent PR, thank you. Just some moments to discuss :)

AndreyBelym · 2020-10-28T13:31:34Z

src/client-functions/selectors/add-api.js

-        // NOTE: we can search for elements only in document or element.
-        if (querySelectorRoot.nodeType !== 1 && querySelectorRoot.nodeType !== 9)
+        // NOTE: we can search for elements only in document/element/shadow root.
+        if (querySelectorRoot.nodeType !== 1 && querySelectorRoot.nodeType !== 9 && querySelectorRoot.toString() !== '[object ShadowRoot]')


I guess we can check here for nodeType === 11 (document fragments' node type) since shadow roots have this node type and document fragments have the querySelectorAll methods:
https://developer.mozilla.org/en-US/docs/Web/API/DocumentFragment

That was my initial approach, but then I saw the hammerhead implementation =)

AndreyBelym · 2020-10-28T13:38:04Z

src/client-functions/selectors/add-api.js

@@ -762,6 +762,28 @@ function addHierarchicalSelectors (options) {

        return createDerivativeSelectorWithFilter(args);
    };
+
+    // ShadowRoot
+    obj.shadowRoot = (filter, dependencies) => {


filter can be used when a method can produce multiple results from a single element (e.g. the parent method that matches all parent nodes from the current element up to the document element). dependencies can be used when filter is a function or a ClientFunction to pass some context variables. Since an element can have only a single shadowRoot, I think we do not need filter and dependencies here.

Makes sense to me now

AndreyBelym · 2020-10-28T14:14:58Z

src/client/driver/command-executors/client-functions/selector-executor/index.js

@@ -63,6 +64,9 @@ export default class SelectorExecutor extends ClientFunctionExecutor {
                const isElementVisible   = !this.command.visibilityCheck || visible(el);
                const isTimeout          = new DateCtor() - startTime >= this.timeout;

+                if (domUtils.isShadowRoot(el))
+                    throw new ActionElementIsShadowRootElementError();


I don't feel that here is the right place for this check. By doing it here, we prevent using expressions like t.expect(element.shadowRoot()).contains('text') that are perfectly valid in my opinion. I think it's better to do it here:
https://github.com/DevExpress/testcafe/blob/master/src/client/driver/command-executors/execute-action.js#L157

testcafe-build-bot · 2020-10-29T20:39:07Z

❌ Tests for the commit db511da have failed. See details:

AlexKamaev · 2020-10-30T07:08:59Z

@testcafe-build-bot retest

AlexKamaev · 2020-10-30T07:18:31Z

@testcafe-build-bot retest

testcafe-build-bot · 2020-10-30T07:47:34Z

✅ Tests for the commit db511da have passed. See details:

AlexKamaev · 2020-10-30T08:22:25Z

@AndreyBelym FPR

[AlexanderMoiseev](https://github.com/AlexanderMoiseev) [augustomezencio-hotmart](https://github.com/augustomezencio-hotmart) - [Fix tree-kill vulnerability from old version.](#5690) [bdwain](https://github.com/bdwain) - [Fixes header types for request and response mocks (#5529)](#5530) [mostlyfabulous](https://github.com/mostlyfabulous) - [Gh2172 Add `ShadowRoot()` as a chainable Selector method](#5560) [danielroe](https://github.com/danielroe) - [Use polyfill if `location.origin` doesn't exist (in IE11)](DevExpress/testcafe-hammerhead#2441)

[AlexanderMoiseev](https://github.com/AlexanderMoiseev) [augustomezencio-hotmart](https://github.com/augustomezencio-hotmart) - [Fix tree-kill vulnerability from old version.](DevExpress#5690) [bdwain](https://github.com/bdwain) - [Fixes header types for request and response mocks (DevExpress#5529)](DevExpress#5530) [mostlyfabulous](https://github.com/mostlyfabulous) - [Gh2172 Add `ShadowRoot()` as a chainable Selector method](DevExpress#5560) [danielroe](https://github.com/danielroe) - [Use polyfill if `location.origin` doesn't exist (in IE11)](DevExpress/testcafe-hammerhead#2441)

[AlexanderMoiseev](https://github.com/AlexanderMoiseev) [augustomezencio-hotmart](https://github.com/augustomezencio-hotmart) - [Fix tree-kill vulnerability from old version.](#5690) [bdwain](https://github.com/bdwain) - [Fixes header types for request and response mocks (#5529)](#5530) [mostlyfabulous](https://github.com/mostlyfabulous) - [Gh2172 Add `ShadowRoot()` as a chainable Selector method](#5560) [danielroe](https://github.com/danielroe) - [Use polyfill if `location.origin` doesn't exist (in IE11)](DevExpress/testcafe-hammerhead#2441)

need-response-app bot added the STATE: Need response label Sep 20, 2020

mostlyfabulous mentioned this pull request Sep 20, 2020

Extend the Selector API to simplify testing ShadowDOM elements #2172

Closed

need-response-app bot removed the STATE: Need response label Sep 22, 2020

need-response-app bot added the STATE: Need response label Sep 27, 2020

need-response-app bot removed the STATE: Need response label Sep 28, 2020

need-response-app bot added the STATE: Need response label Sep 29, 2020

mostlyfabulous force-pushed the gh2172 branch from c36f8bb to 6c161aa Compare September 29, 2020 05:59

need-response-app bot added STATE: Need response and removed STATE: Need response labels Sep 29, 2020

need-response-app bot removed the STATE: Need response label Oct 5, 2020

need-response-app bot added the STATE: Need response label Oct 19, 2020

need-response-app bot removed the STATE: Need response label Oct 20, 2020

AlexKamaev force-pushed the gh2172 branch from 6c161aa to 219604d Compare October 21, 2020 16:00

mostlyfabulous and others added 8 commits October 26, 2020 18:19

Add tests for Selector API: shadowRoot()

c41e49e

fixes

a2e31d9

Add TypeScript definition for shadowRoot()

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

14f5f5f

Add HTML & JS for Selector tests

c2ccc81

Add initial shadowRoot() tests

459c2b5

Draft: re-use tests for shadowRoot()

cf7f119

Add SKIP_IE11_RUN_OPTIONS for Selector API shadowRoot()

8ff37ca

Remove unnecessary tests, use skip

f7157e6

AlexKamaev force-pushed the gh2172 branch from 2e4df98 to 0a0c260 Compare October 26, 2020 15:20

mostlyfabulous marked this pull request as ready for review October 27, 2020 18:20

AndreyBelym reviewed Oct 28, 2020

View reviewed changes

review

db511da

AlexKamaev force-pushed the gh2172 branch from 0a0c260 to db511da Compare October 29, 2020 16:20

AndreyBelym approved these changes Oct 30, 2020

View reviewed changes

AlexKamaev mentioned this pull request Nov 2, 2020

Describe Selector().shadowRoot() method #5675

Closed

AndreyBelym merged commit 7a38865 into DevExpress:master Nov 2, 2020

mostlyfabulous changed the title ~~Gh2172~~ Gh2172 Add ShadowRoot() as a chainable Selector method Nov 5, 2020

AlexSkorkin mentioned this pull request Dec 9, 2020

[docs] Add New Contributors to the README.MD file #5789

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gh2172 Add `ShadowRoot()` as a chainable Selector method #5560

Gh2172 Add `ShadowRoot()` as a chainable Selector method #5560

mostlyfabulous commented Sep 20, 2020 •

edited

Loading

testcafe-build-bot commented Sep 20, 2020

AlexKamaev commented Sep 22, 2020

testcafe-build-bot commented Sep 22, 2020

testcafe-build-bot commented Sep 26, 2020

testcafe-build-bot commented Sep 27, 2020

mostlyfabulous commented Sep 27, 2020

testcafe-build-bot commented Sep 27, 2020

AlexKamaev commented Sep 28, 2020 •

edited

Loading

mostlyfabulous commented Sep 29, 2020 •

edited

Loading

testcafe-build-bot commented Sep 29, 2020

testcafe-build-bot commented Sep 29, 2020

AlexKamaev commented Sep 29, 2020

mostlyfabulous commented Oct 2, 2020

AlexKamaev commented Oct 5, 2020

AlexKamaev commented Oct 14, 2020

mostlyfabulous commented Oct 19, 2020

AlexKamaev commented Oct 20, 2020

testcafe-build-bot commented Oct 26, 2020

AlexKamaev commented Oct 27, 2020

testcafe-build-bot commented Oct 27, 2020

AndreyBelym left a comment

AndreyBelym Oct 28, 2020

AlexKamaev Oct 28, 2020

AndreyBelym Oct 28, 2020

AlexKamaev Oct 28, 2020

mostlyfabulous Oct 28, 2020

AndreyBelym Oct 28, 2020

testcafe-build-bot commented Oct 29, 2020

AlexKamaev commented Oct 30, 2020

AlexKamaev commented Oct 30, 2020

testcafe-build-bot commented Oct 30, 2020

AlexKamaev commented Oct 30, 2020

Gh2172 Add ShadowRoot() as a chainable Selector method #5560

Gh2172 Add ShadowRoot() as a chainable Selector method #5560

Conversation

mostlyfabulous commented Sep 20, 2020 • edited Loading

Purpose

Approach

References

Pre-Merge TODO

testcafe-build-bot commented Sep 20, 2020

AlexKamaev commented Sep 22, 2020

testcafe-build-bot commented Sep 22, 2020

testcafe-build-bot commented Sep 26, 2020

testcafe-build-bot commented Sep 27, 2020

mostlyfabulous commented Sep 27, 2020

IE11 does not support attachShadow and must be polyfilled. Would prefer to skip IE11 tests for shadowRoot API method:

Unrelated PR build errors:

testcafe-build-bot commented Sep 27, 2020

AlexKamaev commented Sep 28, 2020 • edited Loading

mostlyfabulous commented Sep 29, 2020 • edited Loading

testcafe-build-bot commented Sep 29, 2020

testcafe-build-bot commented Sep 29, 2020

AlexKamaev commented Sep 29, 2020

mostlyfabulous commented Oct 2, 2020

AlexKamaev commented Oct 5, 2020

AlexKamaev commented Oct 14, 2020

mostlyfabulous commented Oct 19, 2020

AlexKamaev commented Oct 20, 2020

testcafe-build-bot commented Oct 26, 2020

AlexKamaev commented Oct 27, 2020

testcafe-build-bot commented Oct 27, 2020

AndreyBelym left a comment

Choose a reason for hiding this comment

AndreyBelym Oct 28, 2020

Choose a reason for hiding this comment

AlexKamaev Oct 28, 2020

Choose a reason for hiding this comment

AndreyBelym Oct 28, 2020

Choose a reason for hiding this comment

AlexKamaev Oct 28, 2020

Choose a reason for hiding this comment

mostlyfabulous Oct 28, 2020

Choose a reason for hiding this comment

AndreyBelym Oct 28, 2020

Choose a reason for hiding this comment

testcafe-build-bot commented Oct 29, 2020

AlexKamaev commented Oct 30, 2020

AlexKamaev commented Oct 30, 2020

testcafe-build-bot commented Oct 30, 2020

AlexKamaev commented Oct 30, 2020

Gh2172 Add `ShadowRoot()` as a chainable Selector method #5560

Gh2172 Add `ShadowRoot()` as a chainable Selector method #5560

mostlyfabulous commented Sep 20, 2020 •

edited

Loading

IE11 does not support `attachShadow` and must be polyfilled. Would prefer to skip IE11 tests for `shadowRoot` API method:

AlexKamaev commented Sep 28, 2020 •

edited

Loading

mostlyfabulous commented Sep 29, 2020 •

edited

Loading