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

IE tests #49

Merged
merged 5 commits into from Oct 21, 2011
Merged

IE tests #49

merged 5 commits into from Oct 21, 2011

Conversation

rvagg
Copy link
Collaborator

@rvagg rvagg commented Oct 10, 2011

2 changes:

  1. latest pull request screwed up IE support even more by double using the global i so I've simply redefined it locally where it's causing the problem.
  2. I finally got my head around it and figured out why the IE<9 tests weren't all passing: the context-node collection wasn't working because if you don't have an ID selector anywhere in your query then you default to document which you can't collect siblings of and you really have to do a brute-force through getElementsByTagName('*').

Now all tests are passing in IE6+ et. al.

@ded
Copy link
Owner

ded commented Oct 11, 2011

i believe you bundled this with the last, no?

@rvagg
Copy link
Collaborator Author

rvagg commented Oct 11, 2011

Nope, totally different set of issues, I figured a separate pull request was appropriate so you could review in isolation. Here's my compare with your latest master: https://github.com/rvagg/qwery/compare/master...ieieie

@rvagg
Copy link
Collaborator Author

rvagg commented Oct 11, 2011

no hurry from me btw, I believe you're on holiday so enjoy yourself!

@rvagg
Copy link
Collaborator Author

rvagg commented Oct 17, 2011

Merged latest master so the diff now shows only the 2 lines that have changes to fix IE; currently many selectors with IE throw an error due to the global i and not all tests pass because of the wonky context-node collection. This commit makes everything green and happy.

@mr-le
Copy link

mr-le commented Oct 20, 2011

qwery is very slow in IE8 compared to other selectors
http://jsperf.com/css-selector-libraries/5

@rvagg
Copy link
Collaborator Author

rvagg commented Oct 20, 2011

Qwery wins in my normal browser, FF8b on Linux 64bit, take a bow @ded.

I'm keen to get stuck into the non-native selector routines in Qwery some time and see if I can figure out why Qwery doesn't pass lots of common selector use cases in IE, I might see if I can get my head around the performance while I'm at it. Although, I'm not overly concerned because I'm not sure these kinds of benchmarks really reflect real-world selector-engine usage very well and I don't think it really serves Ender to wind back the clock to the days of the selector-engine wars. If you need raw speed then there's a few obvious alternatives.

@rvagg
Copy link
Collaborator Author

rvagg commented Oct 20, 2011

Oh, I see what you mean, in my IE8 VM I get 545 ops/sec for Qwery vs 16,621 ops/sec for Sizzle. That is quite a difference.

@rvagg
Copy link
Collaborator Author

rvagg commented Oct 21, 2011

OK, so the difference in speed is that Qwery doesn't use querySelectorAll when it's available if it isn't CSS3 compliant, which the IE8 version isn't. But for such a simple case as "tag.class" or just ".class" there's no reason not to IMO, we're already branching for this case anyway. So I've put in a shortcut for it in my latest commit to my ieieie branch. See diff.

See http://jsperf.com/css-selector-libraries/9 if you want reassurance re speed.

Tests still passing.

ded added a commit that referenced this pull request Oct 21, 2011
@ded ded merged commit 9b4c6cb into ded:master Oct 21, 2011
@ded
Copy link
Owner

ded commented Oct 21, 2011

i love how qwery is still faster than querySelectorAll because when it finds just .class it uses getElementsByClassName instead

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

Successfully merging this pull request may close these issues.

None yet

3 participants