Skip to content
This repository has been archived by the owner on Mar 13, 2018. It is now read-only.

Implement newly proposed DOM selection approach #474

Merged
merged 1 commit into from
Jul 28, 2014

Conversation

jcmoore
Copy link

@jcmoore jcmoore commented Jul 25, 2014

There may be some possible improvements for selecting light-DOM nodes in a shadow-rooted element with no contained content element -- I also tried querying the whole DOM natively and traversing it instead of the wrapped tree (https://gist.github.com/jcmoore/69a49f887e03c1d2bfa1#file-queryselector2-js) but it was no faster than the original method, and probably had significant memory implications.

@jcmoore
Copy link
Author

jcmoore commented Jul 25, 2014

Polymer/polymer#629

@@ -7,6 +7,41 @@

var HTMLCollection = scope.wrappers.HTMLCollection;
var NodeList = scope.wrappers.NodeList;
var wrap = scope.wrapIfNeeded;
Copy link
Contributor

Choose a reason for hiding this comment

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

keep this as wrapIfNeeded to avoid confusion

@ebidel
Copy link
Contributor

ebidel commented Jul 25, 2014

Before this PR can be merged, please sign the CLA. See the CONTRIBUTING guide.

@jcmoore jcmoore added cla: no and removed cla: yes labels Jul 25, 2014
@jcmoore
Copy link
Author

jcmoore commented Jul 25, 2014

I'd already received confirmation that the MediaMath's (corporate) CLA had been accepted ([email protected]), do I need to send a copy somewhere else?

@jcmoore jcmoore added cla: no and removed cla: yes labels Jul 25, 2014
@ebidel
Copy link
Contributor

ebidel commented Jul 25, 2014

Got it. Thanks for the heads up.

@ebidel
Copy link
Contributor

ebidel commented Jul 25, 2014

Thanks for signing the CLA!

@jcmoore jcmoore removed the cla: no label Jul 25, 2014
else if (target instanceof OriginalDocument)
list = originalDocumentQuerySelectorAll.call(target, selector);

if (!list) // occurs when when this/target -> ShadowRoot/DocumentFragment
Copy link
Contributor

Choose a reason for hiding this comment

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

How about?

// When we get a ShadowRoot the logical tree is going to be disconnected so we do a manual tree traversal

Any reason this is not just the else branch?

if (target instanceof OriginalElement)
  list = originalElementQuerySelectorAll.call(target, selector);
else if (target instanceof OriginalDocument)
  list = originalDocumentQuerySelectorAll.call(target, selector);
else
  return findElements(this, index, result, p, selector, null);

@arv
Copy link
Contributor

arv commented Jul 25, 2014

LGTM once you fix the curlies for the if/else. (You can use curlies for all if/else if you want too).

Can you squash these into a single commit?

We usually include "Fixes #N" in the commit message too.

@jcmoore
Copy link
Author

jcmoore commented Jul 25, 2014

Ah, to do a squash on the commits do I need a new pull-request (on a new branch?

(I'm unfamiliar with the process I'm afraid)

@arv
Copy link
Contributor

arv commented Jul 25, 2014

@jcmoore
Copy link
Author

jcmoore commented Jul 25, 2014

All squashed

arv added a commit that referenced this pull request Jul 28, 2014
Implement newly proposed DOM selection approach
@arv arv merged commit f2c05d3 into googlearchive:master Jul 28, 2014
@arv
Copy link
Contributor

arv commented Jul 28, 2014

Thank you so much. Lets hope this one does not cause any real world issues.

@sjmiles @sorvell Keep your eyes open.

@sjmiles
Copy link
Contributor

sjmiles commented Jul 28, 2014

Thanks you guys. :) We'll check it out.

@frankiefu
Copy link
Contributor

Filed an issue seems to be related to this change. https://github.com/Polymer/ShadowDOM/issues/476

@jcmoore
Copy link
Author

jcmoore commented Jul 28, 2014

(Very new to pull-request work-flow). In order to address this, must I do a brand new pull-request?

@arv
Copy link
Contributor

arv commented Jul 28, 2014

Yeah. A new pull request seems appropriate.

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

Successfully merging this pull request may close these issues.

5 participants