Page MenuHomePhabricator

Fix attr v prop issues in Collaboration team extensions
Closed, ResolvedPublic

Description

jQuery 1.9+ is more strict in regards to enforcing the distinction between prop and attr. This impacts these extensions in a few places.

checked:

  • Apparent problem in displayTags in ext.pageTriage.tags.js (PageTriage), for both activate and deactivate, as well as in showParamsForm in the same file.
  • Same issue in displayTags in ext.pageTriage.delete.js

disabled:

  • on click callback in createThankLink in ext.thanks.mobilediff.js (Thanks) should use prop
  • enableFormWithRequiredFields in mw-ui.enhance.js (Flow) should use prop disabled.
  • MoodBar ext.moodBar.dashboard.js has "remove disabled prop" which is advised against by jQuery (https://api.jquery.com/removeProp/, which also discusses this memory leak and says it's only applicable if the value is a non-primitive, whereas it does seem to be primitive here). However, it may be necessary for some reason, and it shouldn't cause problems re-setting the property since the whole form is removed immediately after. So I don't think there's an action item here.

Event Timeline

Mattflaschen-WMF raised the priority of this task from to High.
Mattflaschen-WMF updated the task description. (Show Details)
Mattflaschen-WMF added subscribers: Aklapper, Unknown Object (MLST), greg and 6 others.
Krinkle removed a subscriber: Krinkle.Jan 17 2015, 1:37 AM
gerritbot added a subscriber: gerritbot.

Change 187703 had a related patch set uploaded (by Mattflaschen):
Use jQuery prop instead of attr for whether box is checked

https://gerrit.wikimedia.org/r/187703

Patch-For-Review

Change 187711 had a related patch set uploaded (by Mattflaschen):
Use jQuery prop instead of attr for whether btn is disabled

https://gerrit.wikimedia.org/r/187711

Patch-For-Review

Change 187799 had a related patch set uploaded (by Mattflaschen):
Update to follow jQuery 1.11 upgrade guide

https://gerrit.wikimedia.org/r/187799

Patch-For-Review

After those three patches are merged, this can be marked fixed.

Nemo_bis removed a subscriber: Nemo_bis.Feb 1 2015, 3:38 PM

Change 187799 merged by jenkins-bot:
Update to follow jQuery 1.9 upgrade guide

https://gerrit.wikimedia.org/r/187799

Change 187711 merged by jenkins-bot:
Use jQuery prop instead of attr for whether btn is disabled

https://gerrit.wikimedia.org/r/187711

Change 187703 merged by jenkins-bot:
Use jQuery prop instead of attr for whether box is checked

https://gerrit.wikimedia.org/r/187703

DannyH closed this task as Resolved.Feb 3 2015, 11:57 PM
DannyH added a subscriber: DannyH.