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

button.js: Set disabled property in addition to disabled attribute #20278

Merged
merged 1 commit into from Jul 12, 2016

Conversation

cvrebert
Copy link
Collaborator

So as to preserve the existing behavior when running under jQuery 3.

This code ought to have used .prop instead of .attr in the first place, but we can't get rid of the attr manipulation now, due to backward compatibility constraints.

This addresses the only warning that the jQuery Migrate Plugin emitted.

Refs https://github.com/jquery/jquery-migrate/blob/3.0.0/warnings.md#jqmigrate-jqueryfnremoveattr-no-longer-sets-boolean-properties
Refs #16834.

No v4 port is necessary here, since the relevant feature has already been excised from v4's buttons plugin.

CC: @XhmikosR @hnrch02 for review

… preserve behavior under jQuery 3

This code ought to have used .prop instead of .attr in the first place,
but we can't get rid of the attr manipulation now due to backward compatibility constraints.

Refs https://github.com/jquery/jquery-migrate/blob/3.0.0/warnings.md#jqmigrate-jqueryfnremoveattr-no-longer-sets-boolean-properties
Refs #16834

[skip validator]
@cvrebert cvrebert added this to the v3.3.7 milestone Jul 12, 2016
@twbs-savage
Copy link

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: fbeac01
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/144092219
Docs preview: http://preview.twbsapps.com/c/${commitSha.sha}

(Please note that this is a fully automated comment.)

@cvrebert
Copy link
Collaborator Author

Again, the only failure is due to the recent unrelated Windows Firefox issue:
https://saucelabs.com/beta/tests/4f64fe5b76a64b72b22a783e57bd6429/watch

@XhmikosR
Copy link
Member

Why do we need to keep attr if we use prop?

@cvrebert
Copy link
Collaborator Author

Because jQuery v2's attr behaved like a combination of jQuery 3's attr and prop (at least for the 'disabled' attribute), and users' code might be unwittingly relying upon that behavior.
The disabled property controls the current dynamic disabledness; the attribute controls the initial/default disabledness.

@XhmikosR
Copy link
Member

All right, LGTM then.

@hnrch02
Copy link
Collaborator

hnrch02 commented Jul 12, 2016

LGTM

@cvrebert cvrebert merged commit e67e3e9 into master Jul 12, 2016
@cvrebert cvrebert deleted the jq3-attr-prop branch July 12, 2016 11:04
@mdo mdo mentioned this pull request Jul 25, 2016
chiraggmodi pushed a commit to chiraggmodi/bootstrap that referenced this pull request Apr 8, 2019
… preserve behavior under jQuery 3 (twbs#20278)

This code ought to have used .prop instead of .attr in the first place,
but we can't get rid of the attr manipulation now due to backward compatibility constraints.

Refs https://github.com/jquery/jquery-migrate/blob/3.0.0/warnings.md#jqmigrate-jqueryfnremoveattr-no-longer-sets-boolean-properties
Refs twbs#16834

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

Successfully merging this pull request may close these issues.

None yet

4 participants