Page MenuHomePhabricator

Audit deployed Collaboration team extensions for jQuery 1.11 upgrade issues
Closed, ResolvedPublic

Description

I did this for the Growth team extensions earlier, so I'm familiar with what to grep for.

See also https://jquery.com/upgrade-guide/1.9/ and T84996: Buttons requiring a rationale on Curation toolbar appear to be broken.

Event Timeline

Mattflaschen-WMF claimed this task.
Mattflaschen-WMF raised the priority of this task from to High.
Mattflaschen-WMF updated the task description. (Show Details)
Mattflaschen-WMF changed Security from none to None.
Mattflaschen-WMF updated the task description. (Show Details)

Moodbar has live() calls in modules/ext.moodBar.dashboard/ext.moodBar.dashboard.js

With jquery.migrate loaded (core commit 4882249), PageTriage warns:

JQMIGRATE: jQuery.fn.andSelf() replaced by jQuery.fn.addBack()
JQMIGRATE: jQuery.fn.attr('checked') may use property instead of attribute
JQMIGRATE: $(html) HTML strings must start with '<' character

but it seems to work OK.

With jquery.migrate loaded (core commit 4882249), PageTriage warns:

JQMIGRATE: jQuery.fn.andSelf() replaced by jQuery.fn.addBack()

I can't find either in any version of PageTriage, so this probably came from something else. andSelf is actually not used in any of the extensions (but I didn't check past history for all of them).

Okay, these are all the issues I found by going through the 1.9 upgrade guide (that is the only guide there is so far, although we're now at 1.11). I'm not listing the ones I checked/grepped where there was no issue (but I have detailed notes if anyone wants more info), but I've included brief notes where relevant.

All the action items below should have associated tasks (the root for the fixes is T85506: Update extensions maintained by Collaboration team for jQuery 1.9 changes). The audit itself is now done.

.live - As S said, multiple calls in MoodBar: https://phabricator.wikimedia.org/T85300

.add - This was hard to analyze, but as far as I can tell, it looks okay.

Order of triggered "focus" events

  • Triggered focus in mw-ui.modal.js, PageTriage delete, flow-board-misc, etc. but didn't find any focus or blur listeners.
  • There are also some triggered focus in flow-component-events, but no apparent blur listeners. There are focusin and focusout. No behavior change is mentioned in the guide for those, and if it is changed to the new behavior, it seems that is consistent with the listeners.

jQuery(htmlString) versus jQuery(selectorString)

  • MoodBar (T85507)
    • Apparent arbitrary HTML used in ext.moodBar.dashboard.js (245): Should use parseHTML for safety.
    • moodbar.tpl.type on 256 of ext.moodbar.core.js (line continuation), similar ones elsewhere in file (moodbar.tpl/mb.tpl in general)
  • Flow (T85508)
    • mwUiTooltipShow looks like it allows passing arbitrary HTML as content, which would be affected by this; this should also be documented.
    • plaintextSnippet should use $.parseHTML for safety when contentFormta is 'html';
    • 'html' is a misleading variable name for the return value of processTemplateGetFragment (e.g. in flow-board-api-events.js)
    • FlowBoardComponentApiEventsMixin.UI.events.apiHandlers.moderateTopic and FlowBoardComponentApiEventsMixin.UI.events.apiHandlers.moderatePost use HTML directly from processTemplate (also, apparently processTemplateGetFragment should be used instead of this in general, and we may want to make processTemplate private).
  • WikiLove (T85509)
    • $.wikiLove.openDialog in ext.wikiLove.core.js
  • .attr() versus .prop() (T85511)
    • checked
      • Apparent problem in displayTags in ext.pageTriage.tags.js, 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 should use prop
      • 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.
      • enableFormWithRequiredFields in mw-ui.enhance.js should use prop disabled.
  • $("input").attr("type", newValue) in oldIE
    • Should be safe for old code because it previously threw. Now that it no longer does, we need to refrain from changing the type dynamically since it will still fail in IE 8, which we support.
  • "hover" pseudo-event (T85512)
    • One in ext.moodBar.dashboard.js.
    • Another in flow-component.js
  • jQuery.ajax returning a JSON result of an empty string
    • Not checked. It's not grep-friendly, and I don't think our API can normally return an empty string. I don't think our happy path behavior is likely to rely on it.
  • .data("events")
    • Didn't see anything about this using .data when looking for the other data-related change. However, there is _eventForwardDispatch (in Flow), which uses ._data (it's actually a modified version of jQuery's own dispatch code), and is very liable to break in the future (since it uses internal, undocumented members), but should be fine for the time being.
  • Other undocumented properties and methods
    • jQuery.event.handle() - None, although there is some related code in _eventForwardDispatch

I also checked for delegate as a bonus. It's not actually deprecated yet, but it's "superseded". There are only two places where it's used, in the version we use of backbone.js (a third-party library) and MoodBar. We can change the MoodBar one when we fix the 'hover' string (T85512) (it's in the same place)