-
Notifications
You must be signed in to change notification settings - Fork 2k
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
document.querySelectorAll sluggish on Firefox (v30) for large DOM #629
Comments
This is an unfortunate consequence of how we do the ShadowDOM polyfill. What is worse is that we cannot do live node lists so getElementsByClassName, getElementsByTagName and getElementsByName traverse the whole document instead of exiting when first element is found. |
Thank you for the swift response @arv . Admittedly, my understanding of all of the inner-workings of platform.js is naive at best, but any chance you (or someone) can elaborate on why the wrapped NodeLists are built from the bottom-up (by javascript tree traversal), instead of from the top-down (by filtering native NodeLists generated by calls to the native querying methods)? I have an example that seems to work well in Firefox here -- https://gist.github.com/jcmoore/69a49f887e03c1d2bfa1 specifically, please see: and likewise changes to the other overridden query functions: From platform.js' perspective, do the overridden query methods have to do anything beyond keeping "ShadowDOM" elements out of the resulting (wrapped) NodeLists? Is there something else I'm missing? Thanks in advance, |
That is a great approach. I don't see any problems with it and it should add a much needed boost. At one point we didn't have the treeScope_ so a full walk up the tree would have been required. We have open bugs on supporting :shadow and other new selectors here but we can special case them. Do you mind sending a PR? |
Sure thing, but it may take a little while (as I need to get authorization from my employer to agree on the CLA...) |
I thought of a problem with using this approach. <a>
<b></b>
</a> var a = ...
var sr = a.createShadowRoot();
sr.innerHTML = 'no content element';
assert.equal(a.firstElementChild, a.querySelector('b')); The problem is that We might still want to do this even though it breaks a few edge cases. The current solution breaks a few other cases (like |
@jcmoore You can disable the tests but we do not want to commit breaking tests. |
This is awesome @jcmoore. It would be killer to have some before and after numbers to point people to when all of this lands. |
@ebidel This has now been integrated (yay!) and it sounds like we can close the issue. Leaving it open for you to review since you asked for some data. |
Anyone have decent numbers to share? If not we can close. I just thought it would be great to show perf progress. |
On Firefox v30, the platform.js "wrapped" versions of querySelectorAll, getElementsByClassName, getElementsByTagName, and getElementsByName take much longer to execute than their "unwrapped" counterparts.
Example page can be found here -- https://gist.github.com/jcmoore/44b117ce6cd6ab4630f3
The text was updated successfully, but these errors were encountered: