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

Merged
merged 1 commit into from Jul 12, 2016

Projects

None yet

4 participants

@cvrebert
Member

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

@cvrebert cvrebert button.js: Set disabled property in addition to disabled attribute to…
… 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]
fbeac01
@cvrebert cvrebert added js v3 labels Jul 12, 2016
@cvrebert cvrebert added this to the v3.3.7 milestone Jul 12, 2016
@twbs-savage
Member

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
Member

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

@XhmikosR
Member

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

@cvrebert
Member

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
Member

All right, LGTM then.

@hnrch02
Member
hnrch02 commented Jul 12, 2016

LGTM

@cvrebert cvrebert merged commit e67e3e9 into master Jul 12, 2016

0 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
savage BUSTED: Savage cross-browser JS tests failed
hound Hound is busy reviewing changes...
@cvrebert cvrebert deleted the jq3-attr-prop branch Jul 12, 2016
@mdo mdo referenced this pull request Jul 25, 2016
Closed

v3.3.7 ship list #18331

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