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

Test failure on iOS 8: child and adjacent -> p#firstp + p #290

Closed
mgol opened this Issue Oct 11, 2014 · 32 comments

Comments

Projects
None yet
6 participants
@mgol
Member

mgol commented Oct 11, 2014

We have one test failure on iOS8:

Mobile Safari 8.0.0 (iOS 10.10) selector child and adjacent FAILED
    Adjacent (p#firstp + p)
    Expected: [object HTMLParagraphElement]
    Actual: 
    t@/Users/mgol/Documents/projects/public/jquery/sizzle/test/data/testinit.js:51:11
    /Users/mgol/Documents/projects/public/jquery/sizzle/test/unit/selector.js:361:3
    run@/Users/mgol/Documents/projects/public/jquery/sizzle/node_modules/qunitjs/qunit/qunit.js:1303:22
    /Users/mgol/Documents/projects/public/jquery/sizzle/node_modules/qunitjs/qunit/qunit.js:1463:13
    process@/Users/mgol/Documents/projects/public/jquery/sizzle/node_modules/qunitjs/qunit/qunit.js:1016:24
    /Users/mgol/Documents/projects/public/jquery/sizzle/node_modules/qunitjs/qunit/qunit.js:186:12
@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 11, 2014

Member

This is blocking PR #289.

Member

mgol commented Oct 11, 2014

This is blocking PR #289.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Oct 11, 2014

Member

Have you investigated manually? p#firstp + p is a really basic selector that I would not expect to fail, especially by itself.

Member

gibson042 commented Oct 11, 2014

Have you investigated manually? p#firstp + p is a really basic selector that I would not expect to fail, especially by itself.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 11, 2014

Member

I didn't have time to look at it more closely yet, I'll try soon.

This is reproducible both in emulators and on real devices FWIW.

Member

mgol commented Oct 11, 2014

I didn't have time to look at it more closely yet, I'll try soon.

This is reproducible both in emulators and on real devices FWIW.

@BenjaminPoulain

This comment has been minimized.

Show comment
Hide comment
@BenjaminPoulain

BenjaminPoulain Oct 14, 2014

It is probably the bug https://bugs.webkit.org/show_bug.cgi?id=136851 fixed in http://trac.webkit.org/changeset/173688

Compound selectors fail to use the right origin in some cases.

BenjaminPoulain commented Oct 14, 2014

It is probably the bug https://bugs.webkit.org/show_bug.cgi?id=136851 fixed in http://trac.webkit.org/changeset/173688

Compound selectors fail to use the right origin in some cases.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 14, 2014

Member

@BenjaminPoulain Thanks for the links. This really should be fixed in a patch release, it's a serious bug.

@gibson042 I've confirmed on a raw example, withouth jQuery. document.querySelectorAll('p#id + p') returns an empty NodeList even if there are elements that should be matched. This is on a real iPhone 5s with iOS 8.0.2.

Member

mgol commented Oct 14, 2014

@BenjaminPoulain Thanks for the links. This really should be fixed in a patch release, it's a serious bug.

@gibson042 I've confirmed on a raw example, withouth jQuery. document.querySelectorAll('p#id + p') returns an empty NodeList even if there are elements that should be matched. This is on a real iPhone 5s with iOS 8.0.2.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Oct 14, 2014

Member

I'd like to wait and see on this one, then... code to fix a bug present only in iOS 8.0.2–8.0.4 (for example) doesn't thrill me.

Member

gibson042 commented Oct 14, 2014

I'd like to wait and see on this one, then... code to fix a bug present only in iOS 8.0.2–8.0.4 (for example) doesn't thrill me.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 14, 2014

Member

It's unlikely there will be any further 8.0.x releases; 8.1.0 might be released soon, though. Let's see.

Maybe they'll also manage to fix the user agent in 8.1 simulator, that makes it impossible for us to test iOS8 in jQuery.

Member

mgol commented Oct 14, 2014

It's unlikely there will be any further 8.0.x releases; 8.1.0 might be released soon, though. Let's see.

Maybe they'll also manage to fix the user agent in 8.1 simulator, that makes it impossible for us to test iOS8 in jQuery.

@BenjaminPoulain

This comment has been minimized.

Show comment
Hide comment
@BenjaminPoulain

BenjaminPoulain Oct 14, 2014

What's the bug with the simulator's user-agent? Did you file a report on bugzilla and/or radar?

BenjaminPoulain commented Oct 14, 2014

What's the bug with the simulator's user-agent? Did you file a report on bugzilla and/or radar?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 14, 2014

Member

@BenjaminPoulain I filed a bug report via the Feedback Assistant from Yosemite public beta. See tobie/ua-parser#450 for more info.

Member

mgol commented Oct 14, 2014

@BenjaminPoulain I filed a bug report via the Feedback Assistant from Yosemite public beta. See tobie/ua-parser#450 for more info.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 20, 2014

Member

The issue is not fixed in iOS 8.1.

The user agent of a real iOS is correct; the one from iOS 8.1 simulator in the final XCode 6.1 is still wrong.

Member

mgol commented Oct 20, 2014

The issue is not fixed in iOS 8.1.

The user agent of a real iOS is correct; the one from iOS 8.1 simulator in the final XCode 6.1 is still wrong.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 30, 2014

Member

I've reported the issue once again via https://bugreport.apple.com

Member

mgol commented Oct 30, 2014

I've reported the issue once again via https://bugreport.apple.com

@BenjaminPoulain

This comment has been minimized.

Show comment
Hide comment
@BenjaminPoulain

BenjaminPoulain Oct 30, 2014

There is no need, the issue is fixed in WebKit.

The reason the patch does not make it to a minor update is the lack of user impact. The patch should come eventually with the next branching from WebKit.

BenjaminPoulain commented Oct 30, 2014

There is no need, the issue is fixed in WebKit.

The reason the patch does not make it to a minor update is the lack of user impact. The patch should come eventually with the next branching from WebKit.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Nov 4, 2014

Member

The reason the patch does not make it to a minor update is the lack of user impact.

It would be nice if Apple was friendlier towards developers.

Member

mgol commented Nov 4, 2014

The reason the patch does not make it to a minor update is the lack of user impact.

It would be nice if Apple was friendlier towards developers.

@BenjaminPoulain

This comment has been minimized.

Show comment
Hide comment
@BenjaminPoulain

BenjaminPoulain Nov 4, 2014

Sorry for the misunderstanding, I did not mean to split "users" and "developers" as different categories. WebKit is a framework, its "users" are people making website: developers. What I meant is nobody reported this as an important problem affecting a website.

The bug was fixed in WebKit trunk. This area is better tested now to make sure such problem never happen again. It will eventually make it to the stable WebKit.

BenjaminPoulain commented Nov 4, 2014

Sorry for the misunderstanding, I did not mean to split "users" and "developers" as different categories. WebKit is a framework, its "users" are people making website: developers. What I meant is nobody reported this as an important problem affecting a website.

The bug was fixed in WebKit trunk. This area is better tested now to make sure such problem never happen again. It will eventually make it to the stable WebKit.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Nov 4, 2014

Member

I understand it; however, this problem makes it impossible for us to test
jQuery on iOS8 automatically so it's in fact quite important to us. And
jQuery is included on a lot of pages so I hope Apple cares that we can test
it properly and not break on iOS8 by accident.

Member

mgol commented Nov 4, 2014

I understand it; however, this problem makes it impossible for us to test
jQuery on iOS8 automatically so it's in fact quite important to us. And
jQuery is included on a lot of pages so I hope Apple cares that we can test
it properly and not break on iOS8 by accident.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Nov 8, 2014

Member

@gibson042 Since Apple doesn't seem to consider this bug as important enough to backport the fix from current WebKit, should we just blacklist the test on iOS8? Or do you think we should workaround it?

Member

mgol commented Nov 8, 2014

@gibson042 Since Apple doesn't seem to consider this bug as important enough to backport the fix from current WebKit, should we just blacklist the test on iOS8? Or do you think we should workaround it?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Nov 17, 2014

Member

Things that don't work:

p#a + p
p#a ~ p
.b#a ~ [id]
[style]#a ~ p

Things that do work:

#a + p
#a.b ~ [id]
.b#a ~ #c
#a[style] ~ p

It seems that anything#id ~/+ not-an-id is broken.

Update: iOS 8.1.1 went out, this bug is not fixed there.

Member

mgol commented Nov 17, 2014

Things that don't work:

p#a + p
p#a ~ p
.b#a ~ [id]
[style]#a ~ p

Things that do work:

#a + p
#a.b ~ [id]
.b#a ~ #c
#a[style] ~ p

It seems that anything#id ~/+ not-an-id is broken.

Update: iOS 8.1.1 went out, this bug is not fixed there.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Nov 29, 2014

Member

Thanks @mzgol; Webkit source confirmed your anything#id ~/+ not-an-id hypothesis, with the added caveat of in-page selection. So fixing it takes more code than I'd like: #297

Member

gibson042 commented Nov 29, 2014

Thanks @mzgol; Webkit source confirmed your anything#id ~/+ not-an-id hypothesis, with the added caveat of in-page selection. So fixing it takes more code than I'd like: #297

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 5, 2014

Member

I'm still 👎 on landing a fix like this. If Apple doesn't think it's serious, who are we to argue?

Member

dmethvin commented Dec 5, 2014

I'm still 👎 on landing a fix like this. If Apple doesn't think it's serious, who are we to argue?

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Dec 8, 2014

Member

This problem also present in desktop Safari 8.0, that's worrisome since this is a major version whereas we usually wait pretty good amount of time for Apple to release even a minor release for desktop.

This also prevents us to add desktop and mobile Safari to Sizzle browser launchers and we constantly see this problem with integration tests in Core - http://swarm.jquery.org/project/jquery

Member

markelog commented Dec 8, 2014

This problem also present in desktop Safari 8.0, that's worrisome since this is a major version whereas we usually wait pretty good amount of time for Apple to release even a minor release for desktop.

This also prevents us to add desktop and mobile Safari to Sizzle browser launchers and we constantly see this problem with integration tests in Core - http://swarm.jquery.org/project/jquery

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Dec 8, 2014

Member

This problem also present in desktop Safari 8.0, that's worrisome since this is a major version whereas we usually wait pretty good amount of time for Apple to release even a minor release for desktop.

Well, for Apple a minor update is just the current Safari with the engine from the next major version so it's not surprising we wait for those (as they release major updates once a year). Also, because this is effectively a new browser version we will still need to test on Safari 8.0.x even when 8.1.0 is released.

So unless Apple fixes it in a patch release which seems unlikely we either have to workaround it in Sizzle or blacklist the test for Safari 8. The latter seems ugly; we usually don't treat "modern" browsers in this way if we can fix a specific bug.

Member

mgol commented Dec 8, 2014

This problem also present in desktop Safari 8.0, that's worrisome since this is a major version whereas we usually wait pretty good amount of time for Apple to release even a minor release for desktop.

Well, for Apple a minor update is just the current Safari with the engine from the next major version so it's not surprising we wait for those (as they release major updates once a year). Also, because this is effectively a new browser version we will still need to test on Safari 8.0.x even when 8.1.0 is released.

So unless Apple fixes it in a patch release which seems unlikely we either have to workaround it in Sizzle or blacklist the test for Safari 8. The latter seems ugly; we usually don't treat "modern" browsers in this way if we can fix a specific bug.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Dec 8, 2014

Member

If it were Chrome, I'd be less inclined to land the fix. Since Apple has a much slower release cycle, I'm leaning towards landing.

Member

timmywil commented Dec 8, 2014

If it were Chrome, I'd be less inclined to land the fix. Since Apple has a much slower release cycle, I'm leaning towards landing.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Dec 8, 2014

Member

That said, we can land the fix in the same version that removes legacy code. It won't be a lot, but it will likely make room for the fix.

Member

timmywil commented Dec 8, 2014

That said, we can land the fix in the same version that removes legacy code. It won't be a lot, but it will likely make room for the fix.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Dec 8, 2014

Member

@timmywil

That said, we can land the fix in the same version that removes legacy code.

I assume it may take some time? We should then blacklist this test in Safari 8/iOS 8 for the time before the fix lands so that we can actually test on these browsers and care about other failures.

Member

mgol commented Dec 8, 2014

@timmywil

That said, we can land the fix in the same version that removes legacy code.

I assume it may take some time? We should then blacklist this test in Safari 8/iOS 8 for the time before the fix lands so that we can actually test on these browsers and care about other failures.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Dec 8, 2014

Member

@mzgol We can go ahead and land now. We just wouldn't release the next version of Sizzle until the other changes were made.

Member

timmywil commented Dec 8, 2014

@mzgol We can go ahead and land now. We just wouldn't release the next version of Sizzle until the other changes were made.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Dec 8, 2014

Member

@timmywil Landing in Sizzle won't prevent Core from getting the build failure until Core can get the updated Sizzle:
http://swarm.jquery.org/result/2193039

Member

mgol commented Dec 8, 2014

@timmywil Landing in Sizzle won't prevent Core from getting the build failure until Core can get the updated Sizzle:
http://swarm.jquery.org/result/2193039

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Dec 8, 2014

Member

Right, I was only thinking of the Sizzle repo. For Core, it might be a good case for QUnit.skip().

Member

timmywil commented Dec 8, 2014

Right, I was only thinking of the Sizzle repo. For Core, it might be a good case for QUnit.skip().

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 8, 2014

Member

Ok then, if most of the team thinks it should be fixed, let's fix it. How about if I also do a blog post, since this is exactly the kind of thing that jQuery can protect you from vs using bare qSA.

Member

dmethvin commented Dec 8, 2014

Ok then, if most of the team thinks it should be fixed, let's fix it. How about if I also do a blog post, since this is exactly the kind of thing that jQuery can protect you from vs using bare qSA.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 8, 2014

Member

Per our meeting today, we're putting out an urgent patch for 1.11.2/2.1.2 to address this, since 3.0 is a month or more away and a higher risk upgrade. @BenjaminPoulain if you have any idea about when Safari will pull in the WebKit fix, now would be a good time to let us know.

Member

dmethvin commented Dec 8, 2014

Per our meeting today, we're putting out an urgent patch for 1.11.2/2.1.2 to address this, since 3.0 is a month or more away and a higher risk upgrade. @BenjaminPoulain if you have any idea about when Safari will pull in the WebKit fix, now would be a good time to let us know.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Dec 8, 2014

Member

@BenjaminPoulain if you have any idea about when Safari will pull in the WebKit fix, now would be a good time to let us know.

Or if, as I suspect, you're not allowed to share such info even if you know it, it'd be great if you could raise it internally at Apple that we're planning a release just because of this bug. It would help if we didn't have to do it.

Member

mgol commented Dec 8, 2014

@BenjaminPoulain if you have any idea about when Safari will pull in the WebKit fix, now would be a good time to let us know.

Or if, as I suspect, you're not allowed to share such info even if you know it, it'd be great if you could raise it internally at Apple that we're planning a release just because of this bug. It would help if we didn't have to do it.

@BenjaminPoulain

This comment has been minimized.

Show comment
Hide comment
@BenjaminPoulain

BenjaminPoulain Dec 9, 2014

I am sorry, I cannot really say more.

BenjaminPoulain commented Dec 9, 2014

I am sorry, I cannot really say more.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 9, 2014

Member

@BenjaminPoulain That says a lot doesn't it? 😼 The link to the WebKit bug was very helpful so we could understand the scope of the bug. Please do comment when you can, we appreciate it.

Member

dmethvin commented Dec 9, 2014

@BenjaminPoulain That says a lot doesn't it? 😼 The link to the WebKit bug was very helpful so we could understand the scope of the bug. Please do comment when you can, we appreciate it.

gibson042 added a commit that referenced this issue Dec 12, 2014

@gibson042 gibson042 closed this in 24d41a5 Dec 12, 2014

rikschennink added a commit to rikschennink/caniuse that referenced this issue Dec 19, 2014

Update queryselector.json
I'm not sure if this should be in, but I guess it could save people a lot of headache knowing about this bug.

jquery/sizzle#290,

rikschennink added a commit to rikschennink/caniuse that referenced this issue Dec 19, 2014

Update queryselector.json
Letting people know about this bug could save some serious headaches.

jquery/sizzle#290

@gibson042 gibson042 added this to the 2.2.0 milestone Feb 14, 2016

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