Page MenuHomePhabricator

Reference Drawers execute events for previously closed Drawers
Closed, ResolvedPublic

Description

When a reference drawer is opened and closed, the onShowDrawer event is executed additively every time a new Drawer is open. This means that event-handlers for drawers that were closed are still being triggered.

Reproduction steps (for developers)

Add a console.log( "calling Drawer.onShowDrawer" ) on line 94 of Drawer.js
Add a console.log( "calling Drawer.onHideDrawer" ) on line 116 of Drawer.js

Expected results

The onShowDrawer and onHideDrawer methods should be called once when the drawer is open, and once when the drawer is closed.

Actual results

The onShowDrawer method is called 3 times on the first open, and 2 additional times on each subsequent open.
The onHideDrawer method is called 9 times on the first close, 25 times on the second close, 49 times on the third close, 81 times on the fourth close…

This behaviour might have something to do with the way the onShowDrawer event is bound inside a setTimeout, and/or the multitude of events being emitted down the entire inheritance chain of Drawer.

Developer notes
Out of frustration I tried removing all event-emitters from Drawer and removing the setTimeout in onShowDrawer. This seems to fix the issue, but breaks the Drawer animation. The root cause of this behaviour is still uncertain.

POC Patch: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/485034/

before POC patchafter POC patch

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 17 2019, 2:58 PM

Change 485034 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] [POC] Ripping out all custom event-emitters from Drawer.

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

Jdlrobson changed the task status from Open to Stalled.Jan 22 2019, 11:55 PM
Jdlrobson claimed this task.
Jdlrobson added a subscriber: Jdlrobson.

Nice catch. Could we remedy this by simply using this.off in appropriate places? I'm interested to see what comes of T211724 first. Will reopen when that's done.

Jdlrobson changed the task status from Stalled to Open.Oct 29 2019, 5:17 PM
Jdlrobson triaged this task as Medium priority.

This overlaps with some other work we've been doing recently so I'm gonna take a look and see if it's been resolved.

Jdlrobson reassigned this task from Jdlrobson to Jdrewniak.Dec 11 2019, 10:24 PM

This seems to be resolved on master. Moving to sign off. @Jdrewniak would you mind verifying what I'm seeing?

Jdrewniak closed this task as Resolved.Jan 6 2020, 2:35 PM

yippee! looks fixed.

Change 485034 abandoned by Jdlrobson:
[POC] Ripping out all custom event-emitters from Drawer.

Reason:
Associated bug has been resolved so I think this can be abandoned. PLease reopen if there is additioanl work in here to be done.

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