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

support shadow DOM when reading element text #4230

Merged
merged 3 commits into from
Jul 19, 2017
Merged

support shadow DOM when reading element text #4230

merged 3 commits into from
Jul 19, 2017

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Jun 27, 2017

This adds support to getTextContent_ for reading text from shadow DOM nodes.

The following steps were added to the logic here:

  • If the node has a shadow root, return early with the result of retrieving the text for the root rather than the element
  • If the node is a document fragment, treat it like an element with the existing logic (as a fragment has child nodes too)
  • If the node is a <slot> (v1 spec) or <content> (v0 spec), iterate through the associated light DOM nodes and append their text content

Regarding the first point, we ignore the light DOM (the element's childnodes) and skip straight to the shadow root because any light children will only be rendered if there is a related slot/content (either a default or a named one). So any which do get slotted/distributed will be picked up further down when their insertion point is processed.

If I need to add this somewhere else or did my tests wrong, do let me know.

@43081j
Copy link
Contributor Author

43081j commented Jun 27, 2017

could anyone point me in the right direction to updating closure library?

what you have in third_party doesn't look anything like any distribution of the closure library. how have we updated it in the past?

can throw away the last 2 commits if there is a better way

@43081j
Copy link
Contributor Author

43081j commented Jul 1, 2017

not quite sure whats going on with these tests...

it seems the isSelected js method we have sometimes fails assertions in firefox tests.

@shs96c shs96c requested a review from juangj July 7, 2017 08:09
@juangj
Copy link
Contributor

juangj commented Jul 7, 2017

I haven't a clue how Closure gets updated in this repository. @jleyba, can you help out?

I can have a go at the rest of the code, though.

@43081j
Copy link
Contributor Author

43081j commented Jul 9, 2017

I think I updated closure correctly (both the lib and the compiler).

However, some part of the tests seems to fail under firefox. I can't seem to reproduce locally but it happens every time in CI. It appears to be related to closure in some way but i had trouble tracking it down.

@juangj
Copy link
Contributor

juangj commented Jul 11, 2017

Could I trouble you to split the Closure library updates into a separate PR, for the sake of making things easier to review and keeping functional changes separate from "routine" upgrades?

if ((element.nodeType == goog.dom.NodeType.ELEMENT ||
element.nodeType == goog.dom.NodeType.DOCUMENT_FRAGMENT) &&
element.nodeName != 'SCRIPT' &&
element.nodeName != 'STYLE') {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for adding 'STYLE' here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

some web components have style tags in their shadow trees. Polymer is a good example of this.

So, from what I remember, we would end up with CSS in the text content result.

@43081j
Copy link
Contributor Author

43081j commented Jul 16, 2017

At this point it'll be a little difficult splitting the two.

I would need to go back through the commits to see what is update-related and what is not, also making the 2nd PR dependant on the 1st PR.

would it be possible at all to keep it as one? I give it a go if not but will take a little longer.

@juangj
Copy link
Contributor

juangj commented Jul 17, 2017

Sorry for the inconvenience, but I would prefer that the Closure change be separate. It's for the sake of isolating the "meaningful" changes in this PR from the more routine, mechanical changes, and so that if something goes wrong with one of the changes, it's more clear which one.

@43081j 43081j mentioned this pull request Jul 17, 2017
1 task
@43081j
Copy link
Contributor Author

43081j commented Jul 17, 2017

@juangj this has been rebased and the closure update removed.

there is a new PR at #4325 which this will depend on, so it'll fail build until that is merged.

@juangj juangj merged commit 9cf58f6 into SeleniumHQ:master Jul 19, 2017
@juangj
Copy link
Contributor

juangj commented Jul 19, 2017

Thanks for the contribution, and sorry for the hassle!

@43081j 43081j deleted the shadow-text branch July 19, 2017 21:15
@43081j
Copy link
Contributor Author

43081j commented Jul 19, 2017

@juangj looks like some CI builds failed due to some executable issue... hopefully all is well though

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

Successfully merging this pull request may close these issues.

3 participants