ibm-js / delite

HTML Custom Element / Widget infrastructure
http://ibm-js.github.io/delite/
Other
68 stars 28 forks source link

HasDropDown: DOM node leak, old popups not always destroyed. #499

Closed wkeese closed 5 years ago

wkeese commented 5 years ago

On ComboPopup.html test page:

  1. Click to open popup.
  2. Close popup (focus returned to Combobox).
  3. Click page title (blurs Combobox).
  4. Repeat.

There are multiple d-popup nodes that show up in the document.


Here's what's happening:

Combobox (at least on mobile) recreates the popup every time it's opened. So to be simple and consistent, the popup (and wrapper) should be destroyed on closeDropDown(). Instead, Combobox leverages HasDropDown to delete the old popup.

HasDropDown was written to support two cases:

1) a new dropdown is created every time you call openDropDown() (like what Combobox does) 2) the same dropdown is reused every time.

So HasDropDown keeps track of the old dropdown, and then if a new one is created, loadDropDown() deletes the old one:

if (this._previousDropDown && this._previousDropDown !== dropDown) {
    popup.detach(this._previousDropDown); 
    delete this._previousDropDown;
}

However, this isn't working consistently:

So at a basic level, the bug is that HasDropDown#closeDropDown() isn't idempotent, specifically that this code loses the reference to the dropdown when run twice:

this._previousDropDown = this._currentDropDown;
delete this._currentDropDown;

At a more philosophical level, maybe the bug is that this code is all too complicated.

See also related ticket #487.