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

1.12.0 vs 1.11.2 selectors with # broken #2824

Closed
cosmicnet opened this Issue Jan 14, 2016 · 48 comments

Comments

Projects
None yet
@cosmicnet

cosmicnet commented Jan 14, 2016

The following is a perfectly valid selector under 1.11.2:
$('.class a[href=#anchor]');

But fails under 1.12.0, and requires quotes:
$('.class a[href="#anchor"]');

Likely related to:
41f522a

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 14, 2016

Member

Yes, but it was never a valid standard CSS selector, anything with special characters or spaces has to be quoted in the selector. Accepting it in previous versions was a bug.

Attribute values must be CSS identifiers or strings. -- https://www.w3.org/TR/css3-selectors/#attribute-selectors

Member

dmethvin commented Jan 14, 2016

Yes, but it was never a valid standard CSS selector, anything with special characters or spaces has to be quoted in the selector. Accepting it in previous versions was a bug.

Attribute values must be CSS identifiers or strings. -- https://www.w3.org/TR/css3-selectors/#attribute-selectors

@cosmicnet

This comment has been minimized.

Show comment
Hide comment
@cosmicnet

cosmicnet Jan 18, 2016

Whether you regard this as a previous bug or not, it is a breaking change.

cosmicnet commented Jan 18, 2016

Whether you regard this as a previous bug or not, it is a breaking change.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jan 18, 2016

Member

Fixing a bug almost always breaks something.

Member

timmywil commented Jan 18, 2016

Fixing a bug almost always breaks something.

@cosmicnet

This comment has been minimized.

Show comment
Hide comment
@cosmicnet

cosmicnet Jan 18, 2016

The release notes specify this is a non-breaking change. But you are changing the way the selector syntax works and has worked for a long time. There will be a LOT of code in the wild that uses this construct.

Your own test suite previous proved the accepted # syntax:
41f522a
You had to update it to work with the new syntax. Yet nowhere do you indicate that the selector syntax has now changed. Where is the issue for the bug? Where is the release note stating the change in behaviour?

cosmicnet commented Jan 18, 2016

The release notes specify this is a non-breaking change. But you are changing the way the selector syntax works and has worked for a long time. There will be a LOT of code in the wild that uses this construct.

Your own test suite previous proved the accepted # syntax:
41f522a
You had to update it to work with the new syntax. Yet nowhere do you indicate that the selector syntax has now changed. Where is the issue for the bug? Where is the release note stating the change in behaviour?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jan 18, 2016

Member

@cosmicnet Every behavior change may break code that relies on unspecified behavior; if we treated all of them as breaking changes we couldn't have non-major releases at all. Semver only applies to what's documented, i.e. what's on https://api.jquery.com/. We've never promised there that such a selector works, hence by definition changing that is not a breaking change. We did think that it might break some incorrect code, though and that's why we didn't introduce this change in a patch release but in a minor one, even though semver permits us to do it in a patch release.

Member

mgol commented Jan 18, 2016

@cosmicnet Every behavior change may break code that relies on unspecified behavior; if we treated all of them as breaking changes we couldn't have non-major releases at all. Semver only applies to what's documented, i.e. what's on https://api.jquery.com/. We've never promised there that such a selector works, hence by definition changing that is not a breaking change. We did think that it might break some incorrect code, though and that's why we didn't introduce this change in a patch release but in a minor one, even though semver permits us to do it in a patch release.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 18, 2016

Member

@cosmicnet specifically what do you think we need to do here?

Are you advocating that we revert this change? I don't think we can do that. We are trying to parse valid CSS selectors per the spec and allowing these kind of exceptions (even undocumented) makes that difficult if not impossible. It also causes the selector to be rejected by the querySelectorAll path which results in performance degradation.

If you're just saying you're frustrated that this was a breaking change to your code, we're sorry for that. There were several other undocumented features that we did mention in the release notes, it's just hard to know which ones the world may be depending upon. We're always glad to take feedback on our WIP builds, note that this was committed long ago and using the WIP build would have revealed the compatibility issue.

Member

dmethvin commented Jan 18, 2016

@cosmicnet specifically what do you think we need to do here?

Are you advocating that we revert this change? I don't think we can do that. We are trying to parse valid CSS selectors per the spec and allowing these kind of exceptions (even undocumented) makes that difficult if not impossible. It also causes the selector to be rejected by the querySelectorAll path which results in performance degradation.

If you're just saying you're frustrated that this was a breaking change to your code, we're sorry for that. There were several other undocumented features that we did mention in the release notes, it's just hard to know which ones the world may be depending upon. We're always glad to take feedback on our WIP builds, note that this was committed long ago and using the WIP build would have revealed the compatibility issue.

@cosmicnet

This comment has been minimized.

Show comment
Hide comment
@cosmicnet

cosmicnet Jan 18, 2016

@dmethvin The fact that your own test suite was proving the "unspecified behaviour" indicates to me just how common this usage will be, I also see that as documenting the feature. After all, the test suite should be the definitive definition of what is valid and what is not. If it's proving "undocumented features" that are erroneous then that indicates a serious problem with the tests.

I fail to see how not mentioning it at all in the release notes is anything other than an omission. Instead, at the very least a note with the explanation you have given would be far more appropriate.

cosmicnet commented Jan 18, 2016

@dmethvin The fact that your own test suite was proving the "unspecified behaviour" indicates to me just how common this usage will be, I also see that as documenting the feature. After all, the test suite should be the definitive definition of what is valid and what is not. If it's proving "undocumented features" that are erroneous then that indicates a serious problem with the tests.

I fail to see how not mentioning it at all in the release notes is anything other than an omission. Instead, at the very least a note with the explanation you have given would be far more appropriate.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 18, 2016

Member

@cosmicnet specifically what do you think we need to do here?

Member

dmethvin commented Jan 18, 2016

@cosmicnet specifically what do you think we need to do here?

@cosmicnet

This comment has been minimized.

Show comment
Hide comment
@cosmicnet

cosmicnet Jan 18, 2016

@dmethvin I fail to see how not mentioning it at all in the release notes is anything other than an omission. Instead, at the very least a note with the explanation you have given would be far more appropriate.

cosmicnet commented Jan 18, 2016

@dmethvin I fail to see how not mentioning it at all in the release notes is anything other than an omission. Instead, at the very least a note with the explanation you have given would be far more appropriate.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 18, 2016

Member

Thanks. I agree. All we can do is try to track these better in the future.

Member

dmethvin commented Jan 18, 2016

Thanks. I agree. All we can do is try to track these better in the future.

drbyte added a commit to drbyte/zencart that referenced this issue Feb 8, 2016

aaronjorbin added a commit to aaronjorbin/api.jquery.com that referenced this issue Apr 14, 2016

Link to W3C for unquoted single word
Unquoted single word has a specific definition in this case that is not
succinct. A link to the spec helps developers understand what is meant.

fixes jquerygh-910
ref jquery/jquery#2824
@danieliser

This comment has been minimized.

Show comment
Hide comment
@danieliser

danieliser Apr 18, 2016

@dmethvin I think this needs a serious revisit. Its too late now, but in the future this is really relevant to how you make decisions.

WordPress included this in v4.5 and broke dozens of themes if not hundreds of really popular themes which means this bug is now in the wild on a very large scale.

Problem is that my products are unrelated and have no issues at all but are reliant on our users theme not throwing stupid errors.

So no my entire support queue is full due to problems I can't do anything about. Seems like the simple solution would have been to simply make it still recognize the older ones and throw a silent error while still doing what was expected originally. Deprecate it, document it, revist it later to remove it entirely.

Progress in the name of progress, is anything but.

danieliser commented Apr 18, 2016

@dmethvin I think this needs a serious revisit. Its too late now, but in the future this is really relevant to how you make decisions.

WordPress included this in v4.5 and broke dozens of themes if not hundreds of really popular themes which means this bug is now in the wild on a very large scale.

Problem is that my products are unrelated and have no issues at all but are reliant on our users theme not throwing stupid errors.

So no my entire support queue is full due to problems I can't do anything about. Seems like the simple solution would have been to simply make it still recognize the older ones and throw a silent error while still doing what was expected originally. Deprecate it, document it, revist it later to remove it entirely.

Progress in the name of progress, is anything but.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 18, 2016

Member

When WordPress updates a jQuery version it may always break themes,
especially if it's a major upgrade of jQuery. This wasn't a major upgrade
but previously we weren't following semver and minor upgrades did have
breaking changes from time to time, e.g. in 1.9 there were many of them. I'm
sure it broke a lot of code back then as well. This is unavoidable,
otherwise we couldn't make any changes in the library.

This is completely normal if after a new jQuery version is released some
plugins/themes don't work with it. They should adjust to the changes over
time.

Michał Gołębiowski

Member

mgol commented Apr 18, 2016

When WordPress updates a jQuery version it may always break themes,
especially if it's a major upgrade of jQuery. This wasn't a major upgrade
but previously we weren't following semver and minor upgrades did have
breaking changes from time to time, e.g. in 1.9 there were many of them. I'm
sure it broke a lot of code back then as well. This is unavoidable,
otherwise we couldn't make any changes in the library.

This is completely normal if after a new jQuery version is released some
plugins/themes don't work with it. They should adjust to the changes over
time.

Michał Gołębiowski

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Apr 18, 2016

Member

Totally agree with @mgol, but i think we should discuss this on today meeting anyway.

Member

markelog commented Apr 18, 2016

Totally agree with @mgol, but i think we should discuss this on today meeting anyway.

@markelog markelog reopened this Apr 18, 2016

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Apr 18, 2016

Member

Okay, so we discuss this in length at the meeting, we decide to do the following -

  1. We not gonna revert
  2. We will try to mitigate this in jquery-migrate, since wordpress includes it, it should help
  3. We changed our policy guideline to prevent such changes happening in patch/minor versions on case-by-case bases
  4. We will discuss this with wordpress team

Thank you for your thoughts everyone!

Member

markelog commented Apr 18, 2016

Okay, so we discuss this in length at the meeting, we decide to do the following -

  1. We not gonna revert
  2. We will try to mitigate this in jquery-migrate, since wordpress includes it, it should help
  3. We changed our policy guideline to prevent such changes happening in patch/minor versions on case-by-case bases
  4. We will discuss this with wordpress team

Thank you for your thoughts everyone!

@markelog markelog closed this Apr 18, 2016

@markelog markelog removed the Needs review label Apr 18, 2016

@danieliser

This comment has been minimized.

Show comment
Hide comment
@danieliser

danieliser Apr 19, 2016

@markelog - Thanks guys, I think that will go a long way. I am going to discuss with some fellow WP devs the possibility of both automated compatibility checks for plugins and themes as well as the possibility to do version-ed loading of jQuery in the future, allowing plugins & themes to load a select version if required with its own unique namespace. I think with those 2 their could be a path to jQuery 3 over a few years.

danieliser commented Apr 19, 2016

@markelog - Thanks guys, I think that will go a long way. I am going to discuss with some fellow WP devs the possibility of both automated compatibility checks for plugins and themes as well as the possibility to do version-ed loading of jQuery in the future, allowing plugins & themes to load a select version if required with its own unique namespace. I think with those 2 their could be a path to jQuery 3 over a few years.

AurelioDeRosa added a commit to jquery/api.jquery.com that referenced this issue Apr 19, 2016

Link to W3C for unquoted single word
Unquoted single word has a specific definition in this case that is not
succinct. A link to the spec helps developers understand what is meant.

Fixes gh-910
Ref jquery/jquery#2824
Closes gh-911
@aaronjorbin

This comment has been minimized.

Show comment
Hide comment
@aaronjorbin

aaronjorbin Apr 20, 2016

RE: WordPress jQuery communication.
I'm one of the WordPress core committers. Happy to help encourage communication in any way I can. I'm now idling in both the jquery and jquery-dev rooms as jorbin. Feel free to contact me if you ever need anything from WordPress. Y'all are also welcome to come hang out in our slack.

aaronjorbin commented Apr 20, 2016

RE: WordPress jQuery communication.
I'm one of the WordPress core committers. Happy to help encourage communication in any way I can. I'm now idling in both the jquery and jquery-dev rooms as jorbin. Feel free to contact me if you ever need anything from WordPress. Y'all are also welcome to come hang out in our slack.

@danieliser

This comment has been minimized.

Show comment
Hide comment
@danieliser

danieliser Apr 20, 2016

@aaronjorbin - Thanks for jumping in here. I think this is a strategic partnership for both WP & jQuery.

danieliser commented Apr 20, 2016

@aaronjorbin - Thanks for jumping in here. I think this is a strategic partnership for both WP & jQuery.

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Apr 21, 2016

Member

FWIW, I've been in the WordPress Slack channel since it was first created. There are multiple WordPress committers who ping me when things come up related to jQuery projects. I'm not sure how this helps with a change like this though, unless @timmywil is going to inform WordPress devs of every change.

One part of the solution is being available. This is easy. And @aaronjorbin just did that for the WP -> jQuery side.

Another part is making your presence known. This is not so easy. The people involved in this thread now know that @aaronjorbin is available to ping in our channels if needed. However, others do not know. And over time, some people in this thread will forget.

The bigger part is being active. As @aaronjorbin said, "I'm now idling in both the jquery and jquery-dev rooms as jorbin." But idling is not the same as paying attention to everything that's going on. I'm not saying that this is bad, it's exactly what I've been doing for the jQuery -> WP side for years (going back to before Slack). But without the active part of this, I'm not sure how issues like this will be avoided.

I'm all for increasing communication even more. Certainly the more people we have on each side that are idling in various channels, the more people will know to ping them at the right times. But I'm interested to hear what the plans are for how this type of problem will be avoided in the future.

I used to take a much more active role in this across several projects, but it's been a few years since I switched to a passive role. This page is now very out of date, but we used to track this information on the jQuery UI wiki: http://wiki.jqueryui.com/w/page/54364943/Project-Collaboration though we never published individual names for the people we worked with.

Member

scottgonzalez commented Apr 21, 2016

FWIW, I've been in the WordPress Slack channel since it was first created. There are multiple WordPress committers who ping me when things come up related to jQuery projects. I'm not sure how this helps with a change like this though, unless @timmywil is going to inform WordPress devs of every change.

One part of the solution is being available. This is easy. And @aaronjorbin just did that for the WP -> jQuery side.

Another part is making your presence known. This is not so easy. The people involved in this thread now know that @aaronjorbin is available to ping in our channels if needed. However, others do not know. And over time, some people in this thread will forget.

The bigger part is being active. As @aaronjorbin said, "I'm now idling in both the jquery and jquery-dev rooms as jorbin." But idling is not the same as paying attention to everything that's going on. I'm not saying that this is bad, it's exactly what I've been doing for the jQuery -> WP side for years (going back to before Slack). But without the active part of this, I'm not sure how issues like this will be avoided.

I'm all for increasing communication even more. Certainly the more people we have on each side that are idling in various channels, the more people will know to ping them at the right times. But I'm interested to hear what the plans are for how this type of problem will be avoided in the future.

I used to take a much more active role in this across several projects, but it's been a few years since I switched to a passive role. This page is now very out of date, but we used to track this information on the jQuery UI wiki: http://wiki.jqueryui.com/w/page/54364943/Project-Collaboration though we never published individual names for the people we worked with.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Apr 21, 2016

Member

@scottgonzalez

But I'm interested to hear what the plans are for how this type of problem will be avoided in the future

Improving communication between projects would definitely help (personally, i'm aware of our connection with the wordpress team), but wordpress wasn't exclusively affected by this problem, we received issues about this before.

Scale of it, however, was revealed only in this ticket, that is why it was re-open and added back to the agenda of the weekly meeting.

In monday, we had extensive discussion on not letting these kind of issues rise again in patch and minor versions, which resulted in guideline changes.

So from now on, we will be more careful with these kind of changes and even though they will still be evaluated on case by case basis (as guideline says), likelihood of such event should be very low. Not only for the wordpress themes though but for all users.

Maybe @dmethvin or @timmywil have more info about this.

Member

markelog commented Apr 21, 2016

@scottgonzalez

But I'm interested to hear what the plans are for how this type of problem will be avoided in the future

Improving communication between projects would definitely help (personally, i'm aware of our connection with the wordpress team), but wordpress wasn't exclusively affected by this problem, we received issues about this before.

Scale of it, however, was revealed only in this ticket, that is why it was re-open and added back to the agenda of the weekly meeting.

In monday, we had extensive discussion on not letting these kind of issues rise again in patch and minor versions, which resulted in guideline changes.

So from now on, we will be more careful with these kind of changes and even though they will still be evaluated on case by case basis (as guideline says), likelihood of such event should be very low. Not only for the wordpress themes though but for all users.

Maybe @dmethvin or @timmywil have more info about this.

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Apr 21, 2016

Member

That sounds great @markelog. Does this also mean that changes like this, even though the feature was undocumented, will be listed in the change log for major releases?

Member

scottgonzalez commented Apr 21, 2016

That sounds great @markelog. Does this also mean that changes like this, even though the feature was undocumented, will be listed in the change log for major releases?

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Apr 21, 2016

Member

Yeah, see definition on https://github.com/jquery/jquery/wiki/API-design-guidelines under "Undocumented inputs result in unpredictable output."

Member

markelog commented Apr 21, 2016

Yeah, see definition on https://github.com/jquery/jquery/wiki/API-design-guidelines under "Undocumented inputs result in unpredictable output."

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Apr 21, 2016

Member

Perfect. Thanks.

Member

scottgonzalez commented Apr 21, 2016

Perfect. Thanks.

@danieliser

This comment has been minimized.

Show comment
Hide comment
@danieliser

danieliser Apr 21, 2016

@markelog Thanks for the special attention you guys have shown here. I think even a subtle guideline change like that could make all the difference in terms of keeping scale of issues down.

Next step would be to get the WP Make: Meta team involved to update the automated scanners to look for these types of things and notify plugin & theme authors automatically.

danieliser commented Apr 21, 2016

@markelog Thanks for the special attention you guys have shown here. I think even a subtle guideline change like that could make all the difference in terms of keeping scale of issues down.

Next step would be to get the WP Make: Meta team involved to update the automated scanners to look for these types of things and notify plugin & theme authors automatically.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 2, 2016

Member

There is a patch going into the next jQuery Migrate (both 1.x and 3.x) to detect and fix most of these cases. There are still situations where the selector is too complex for the regexp to fix it, but it seems to handle the cases that have been reported in the past months. jquery/jquery-migrate#184

Member

dmethvin commented May 2, 2016

There is a patch going into the next jQuery Migrate (both 1.x and 3.x) to detect and fix most of these cases. There are still situations where the selector is too complex for the regexp to fix it, but it seems to handle the cases that have been reported in the past months. jquery/jquery-migrate#184

@dmethvin dmethvin referenced this issue May 6, 2016

Closed

Event: evaluate selector at add time #3097

3 of 4 tasks complete

dougbeal added a commit to foolscapcon/affix-sidebar-bootstrap-sass that referenced this issue May 9, 2016

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 20, 2016

Member

jQuery Migrate 1.4.1 has just been released, it fixes the problem with unquoted selectors in the cases that have been reported.

http://blog.jquery.com/2016/05/19/jquery-migrate-1-4-1-released-and-the-path-to-jquery-3-0/

Member

dmethvin commented May 20, 2016

jQuery Migrate 1.4.1 has just been released, it fixes the problem with unquoted selectors in the cases that have been reported.

http://blog.jquery.com/2016/05/19/jquery-migrate-1-4-1-released-and-the-path-to-jquery-3-0/

art-mickiewicz added a commit to art-mickiewicz/django-suit that referenced this issue Nov 17, 2016

Update suit.js
Fixes tab activation with invalid jquery selector # emerged on the updated jquery.
Related jquery issue: jquery/jquery#2824
@alijamal14

This comment has been minimized.

Show comment
Hide comment
@alijamal14

alijamal14 Dec 17, 2016

Salaamun Alekum Can Anyone Tell me What Can I Write Alternate To This

linkElement: '#primary-menu ul li a:not([target="_blank"]):not([href*=#]):not([data-lightbox])',

alijamal14 commented Dec 17, 2016

Salaamun Alekum Can Anyone Tell me What Can I Write Alternate To This

linkElement: '#primary-menu ul li a:not([target="_blank"]):not([href*=#]):not([data-lightbox])',

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 17, 2016

Member

@alijamal14 please ask for help on StackOverflow

Member

dmethvin commented Dec 17, 2016

@alijamal14 please ask for help on StackOverflow

@jquery jquery locked and limited conversation to collaborators Dec 17, 2016

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