Closed Bug 900418 Opened 6 years ago Closed 6 years ago
Inplace editor should not destroy itself when the focus is lost to the autocompletion popup
When any CSS suggestion is shown, as soon as you click on any suggestion in the popup to complete that, the editor looses focus and thus destroys itself. It should somehow recognize that focus is lost to popup and not destroy itself
Works, but breaks tests. Need to fix them. Asking for feedback to know if this is the right approach.
Fixes test failures. try at : https://tbpl.mozilla.org/?tree=Try&rev=9b49eceaedcd
pushed to try again with the latest fx-team (which has the fix for all those oranges) : https://tbpl.mozilla.org/?tree=Try&rev=f8bdf63e995d
I will wait for a green try before reviewing this.
So turns out that the leakage was caused because the destroy method of rule view (rule-view.js:1009) was throwing as this.elementStyle is undefined. This is the case for m-c also (see fx-team  logs). This was leading to the call of "this.popup.destroy()" to never happend. Prior to this bug that did not cause any leakage as I was not touching the panel of the popup (this.popup._panel) via addEventListeners, but in this patch, I am. Although I am removing the listeners too, but some race condition is causing the call of destroy before the removal and thus messing up stuff and leading to a leakage caused by the popup (its what I believe). Simply moving "this.popup.destroy()" call before "this.elementStyle.destroy();" fixes the issue. I am going ahead with that and filing a bug for the "this.elementStyle being undefined" issue. new try at https://tbpl.mozilla.org/?tree=Try&rev=4af9afc0c898  : https://tbpl.mozilla.org/php/getParsedLog.php?id=26724784&tree=Fx-Team&full=1
Ah. I found out why I was actually leaking, it was because the added mthod to the event listener had .bind, while the removed method did not :-/ Will fix it. Mike, you can continue with the review for the rest of the things if you are planning to :)
Attachment #791843 - Flags: review?(mratcliffe) → review+
I'm happy for Mike to continue reviewing this.
6 years ago
Rebased on latest fx-team. Multiple green trys: https://tbpl.mozilla.org/?tree=Try&rev=51663149477a https://tbpl.mozilla.org/?tree=Try&rev=a8af567e17aa
landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/5b5ea8584460
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.