900418 - Inplace editor should not destroy itself when the focus is lost to the autocompletion popup

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Girish Sharma [:Optimizer]]

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

Comment 1

[Girish Sharma [:Optimizer]]

Attachment: WIP

Works, but breaks tests. Need to fix them.

Asking for feedback to know if this is the right approach.

Comment 2

[Girish Sharma [:Optimizer]]

Attachment: patch v0.1

Fixes test failures.

try at : https://tbpl.mozilla.org/?tree=Try&rev=9b49eceaedcd

Comment 3

[Girish Sharma [:Optimizer]]

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

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I will wait for a green try before reviewing this.

Comment 5

[Girish Sharma [:Optimizer]]

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 [0] 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

[0] : https://tbpl.mozilla.org/php/getParsedLog.php?id=26724784&tree=Fx-Team&full=1

Comment 6

[Girish Sharma [:Optimizer]]

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  :)

Comment 7

[Joe Walker [:jwalker]]

I'm happy for Mike to continue reviewing this.

Comment 8

[Girish Sharma [:Optimizer]]

Attachment: Rebased patch

Rebased on latest fx-team.

Multiple green trys:
https://tbpl.mozilla.org/?tree=Try&rev=51663149477a
https://tbpl.mozilla.org/?tree=Try&rev=a8af567e17aa

Comment 9

[Girish Sharma [:Optimizer]]

landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/5b5ea8584460

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b5ea8584460