Page MenuHomePhabricator

Test failure in "ve.ce.Surface.test: special key down: table cells: Tab at end of table inserts new row" with OOjs UI v0.19.2 (clipping a PopupWidget that is unattached)
Closed, ResolvedPublic8 Estimated Story Points

Description

OOjs UI v0.19.2 https://gerrit.wikimedia.org/r/#/c/337760/ let's tests fail in lib/ve.
From conversation with @matmarex earlier

MatmaRex> i guess we assume that direction will always be 'ltr' or 'rtl', and it isn't in this case?
MatmaRex> if it comes from test, it might just be a test bug
MatmaRex> Volker_E: what's the name of the failing test?
MatmaRex> Volker_E: ok, i see it myself. it's failing on "ve.ce.Surface: special key down: linear enter"
MatmaRex> Volker_E: i think this is a tests-only problem. the actual editor seems fine.
MatmaRex> there is definitely a bug in the VE tests. they're trying to clip() a PopupWidget that is invisible.
MatmaRex> this almost certainly doesn't occur in the normal operation
MatmaRex> s/invisible/unattached/
MatmaRex> clipping unattached elements was always documented not to work correctly. it just never thrown exceptions before.
MatmaRex> i'm not going to try to track down where this is coming from right now. it seems to be something asynchronous - every time i run the tests, the exception appears inside a different one
MatmaRex> Volker_E: can you file a task in VE? this might be obvious to someone more familiar with the code than me.

Event Timeline

Jdforrester-WMF renamed this task from OOjs UI v0.19.2 let lib/ve tests fail to Test failure in ve.ce.Surface.test: special key down: linear arrow keys with OOjs UI v0.19.2 (clipping a PopupWidget that is unattached).Feb 15 2017, 8:26 PM
Jdforrester-WMF assigned this task to Esanders.
Jdforrester-WMF raised the priority of this task from High to Unbreak Now!.
Jdforrester-WMF set the point value for this task to 8.

This is very much a bug in VE's test code. It's trying to call ve.ui.Surface.prototype.initialize while the surface is not attached to the DOM, which is explicitly prohibited in the documentation. This causes OO.ui.PopupWidget.prototype.toggle to be called while the popup is not attached to the DOM either, which we technically did not document to be prohibited, but it's clearly meant to be (it calls methods like togglePositioning and toggleClipping which are documented so).

The culprit is this:

lib/ve/tests/ce/ve.ce.Surface.test.js
QUnit.test( 'special key down: table cells', function ( assert ) {

				// Create a full surface and return the view, as the UI surface is required for the insert action
				htmlOrDoc: ve.test.utils.createSurfaceFromDocument( ve.dm.example.createExampleDocument( 'mergedCells' ) ).view,

} );
matmarex renamed this task from Test failure in ve.ce.Surface.test: special key down: linear arrow keys with OOjs UI v0.19.2 (clipping a PopupWidget that is unattached) to Test failure in "ve.ce.Surface.test: special key down: table cells: Tab at end of table inserts new row" with OOjs UI v0.19.2 (clipping a PopupWidget that is unattached).Feb 15 2017, 9:51 PM

So actually, ve.test.utils.createSurfaceFromDocument appends the surface to the DOM, to #qunit-fixture, which gets cleaned up immediately when the test finishes. The problem comes from the 'afterContextChange' handler, which is async, and runs after that test finishes and the surface is not attached anymore. (This is why the failure is showing up for different tests than the one really causing the problem). I'll note that the ve.ui.Surface never gets #destroy called on it, either, it's just unceremoniously removed.

Change 338011 had a related patch set uploaded (by Bartosz Dziewoński):
ve.ce.Surface.test: Gross workaround for failing tests

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

The above one-line workaround makes the problematic test async, and magically makes everything stop failing, at least locally for me. I'm leaving this assigned to Ed to work on a real fix, or decide that this is enough. :)

Change 338011 merged by jenkins-bot:
ve.ce.Surface.test: Gross workaround for failing tests

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

Jdforrester-WMF lowered the priority of this task from Unbreak Now! to High.Feb 15 2017, 11:03 PM
Jdforrester-WMF removed a project: Patch-For-Review.

OK, MW/core and related repos' CI should be back to working. Sorry everyone.

Change 338025 had a related patch set uploaded (by Jforrester):
[Re-apply] Update OOjs UI to v0.19.2

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

Change 338028 had a related patch set uploaded (by Bartosz Dziewoński):
ve.test.utils.createSurfaceFromDocument: Actually attach surface to DOM

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

The issue was still occurring with VE-MW, @Krinkle and I spent more time debugging on #wikimedia-dev.

Krinkle noticed that ve.test.utils.createSurfaceFromDocument is overridden in VE-MW… so I started looking into that function:

[01:05]	<MatmaRex> the cause is that we have a ui.Surface that is somehow not attached to the DOM when it should be, and that breaks things
[01:06]	<MatmaRex> in VE core, the problem was that it was being cleared from #qunit-fixture while things were still ongoing. i fixed that with the setTimeout( assert.async() ).
[01:06]	<MatmaRex> i think right now, the problem is that it does not get attached at all. i'm still trying to figure out how to verify this.

Change 338028 merged by jenkins-bot:
ve.test.utils.createSurfaceFromDocument: Actually attach surface to DOM

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

Change 338025 merged by jenkins-bot:
[Re-apply] Update OOjs UI to v0.19.2

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

I submitted https://gerrit.wikimedia.org/r/338181 "PopupWidget (and similar): Document why it is unwise to show unattached widgets, and emit warnings" in OOjs UI which should make it more difficult to make this kind of a mistake in the future.

The OOjs UI change in v0.19.2 (rGOJU799f500934eb: FloatableElement, PopupWidget: Do positioning from the right in RTL) only exposed a pre-existing bug in VE tests. This was not an OOjs UI bug.