Page MenuHomePhabricator
Create Task
Maniphest T169272

[Regression pre-wmf.8] VE crashes while opening Gallery and some other dialogs
Closed, ResolvedPublic1 Estimated Story Points
Actions

Description

Gallery, Comment, Hieroglyphs, Musical notation, Chemical Formula, Math, Map, Code block, Graph these dialogs are not opening.

Media and Citation dialog are uneffected.

There is an error in the console:

Uncaught TypeError: Cannot read property 'val' of undefined (`this.$input`)
 at VeUiWhitespacePreservingTextInputWidget (OO.ui.InputWidget) .setValue
 at new VeUiWhitespacePreservingTextInputWidget
 at VeUiMWGalleryDialog.initialize

Uncaught TypeError: Cannot read property 'setWhitespace' of null (`this.input`)
 at VeUiMWGalleryDialog.<anonymous>

Details

SubjectRepoBranchLines +/-
TextInputWidgets: update for deprecated multiline behaviormediawiki/extensions/VisualEditormaster+4 -9
Update VE core submodule to master (65ea4cf36)mediawiki/extensions/VisualEditormaster+1 -1
ui.WhitespacePreservingTextInputWidget: inherit from MultilineTextInputWidgetVisualEditor/VisualEditormaster+2 -3
Customize query in gerrit

Related Objects

Event Timeline

Ryasmeen created this task.Jun 29 2017, 9:51 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 29 2017, 9:51 PM
Ryasmeen renamed this task from [Regression pre-wmf.8] Gallery and some other dialogs are not opening in VE to [Regression pre-wmf.8] VE crashes while opening Gallery and some other dialogs .Jun 29 2017, 9:56 PM
Ryasmeen updated the task description. (Show Details)
Krinkle updated the task description. (Show Details)Jun 30 2017, 9:20 PM
Ryasmeen added subscribers: Esanders, matmarex.Jun 30 2017, 9:22 PM
DLynch claimed this task.Jul 3 2017, 4:49 PM
DLynch subscribed.

Seems to be fallout from 1eb25592c removing multiline support from TextInputWidget. Only landed in OOui in 0.22.2, so it's only been visible in VE since 52e9cc34 on the 29th.

DLynch added a comment.Jul 3 2017, 4:55 PM

Also, I don't think that's quite the standard meaning of "deprecating change", insofar as it outright broke the functionality...

gerritbot subscribed.Jul 3 2017, 4:56 PM

Change 363034 had a related patch set uploaded (by DLynch; owner: DLynch):
[VisualEditor/VisualEditor@master] ui.WhitespacePreservingTextInputWidget: inherit from MultilineTextInputWidget

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

gerritbot added a project: Patch-For-Review.Jul 3 2017, 4:56 PM
• Deskana triaged this task as High priority.Jul 3 2017, 4:57 PM
• Deskana moved this task from To Triage to TR0: Interrupt on the VisualEditor board.
gerritbot added a comment.Jul 3 2017, 5:04 PM

Change 363034 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] ui.WhitespacePreservingTextInputWidget: inherit from MultilineTextInputWidget

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

gerritbot added a comment.Jul 3 2017, 5:12 PM

Change 363036 had a related patch set uploaded (by Jforrester; owner: DLynch):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (65ea4cf36)

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

gerritbot added a comment.Jul 3 2017, 5:21 PM

Change 363036 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (65ea4cf36)

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

Jdforrester-WMF edited projects, added MediaWiki-Gallery, VisualEditor-MediaWiki; removed Patch-For-Review.Jul 3 2017, 5:44 PM
Jdforrester-WMF set the point value for this task to 1.
• Deskana closed this task as Resolved.Jul 3 2017, 5:45 PM
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptJul 3 2017, 5:45 PM
DLynch added a comment.Jul 3 2017, 8:00 PM

To elaborate, I think the reason the deprecation didn't work is that it relied on returning a new OO.ui.MultilineTextInputWidget from TextInputWidget's constructor, and our WhitespacePreservingTextInputWidget emphatically didn't expect this (technically valid but unusual) possibility. As such, the widget we created effectively never had the parent constructor run on it, and all the expected properties were therefore missing.

DLynch mentioned this in T130434: Break up TextInputWidget.Jul 3 2017, 8:58 PM
matmarex added a comment.Jul 3 2017, 8:58 PM

Hmm, I suppose we never tested this magical deprecation with custom subclasses.

I think by now this is spilled milk. But we should keep this in mind for the future and do any other deprecations the boring way (by just keeping all the old code in place, unchanged).

gerritbot added a comment.Jul 3 2017, 9:07 PM

Change 363095 had a related patch set uploaded (by DLynch; owner: DLynch):
[mediawiki/extensions/VisualEditor@master] Custom TextInputWidgets: clean up unneeded multiline config

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

gerritbot added a project: Patch-For-Review.Jul 3 2017, 9:07 PM
gerritbot added a comment.Jul 3 2017, 9:21 PM

Change 363095 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] TextInputWidgets: update for deprecated multiline behavior

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

Ryasmeen added a project: Verified.Jul 3 2017, 11:22 PM
Nikerabbit mentioned this in T169610: Use of `multiline: true` in subclasses of TextInputWidget broken.Jul 4 2017, 9:29 AM
Schnark subscribed.Jul 4 2017, 10:17 AM
In T169272#3402555, @matmarex wrote:

Hmm, I suppose we never tested this magical deprecation with custom subclasses.

I think by now this is spilled milk. But we should keep this in mind for the future and do any other deprecations the boring way (by just keeping all the old code in place, unchanged).

Just wondering (I don't have the time now to look that strange stuff up in the specification): What would happen if instead of return new TheNewConstructor() you'd use this = new TheNewConstructor()? Does this even work, and will it make inheritance work as expected?

Schnark added a comment.Jul 5 2017, 7:23 AM

Answering my own question: No, this is not possible, this is read-only. So I don't think the magic way would have worked even with more magic. One of the problems is the fact, that the prototype chain is just wrong, which would require the constructor to change the prototype of the object it is currently creating, which would be a huge mess.
I think a good basic rule for the future is: "Never return anything from a constructor, it will break inheritance."

DLynch added a comment.EditedJul 6 2017, 2:13 PM

@Schnark: I actually did a little bit of looking into changing the prototype on the returned object, and I sadly realized it's impossible in IE10, which has neither Object.setPrototypeOf nor __proto__. If not for that it'd be workable, albeit horrific. (Though admittedly I didn't confirm that it wouldn't do something weird mid-constructor.)

ReleaseTaggerBot added a project: MW-1.30-release-notes (WMF-deploy-2017-07-11_(1.30.0-wmf.9)).Jul 8 2017, 2:01 AM
Ryasmeen edited projects, added Verified-in-Phase0; removed Patch-For-Review.Jul 12 2017, 2:18 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL