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

Categories

(DevTools :: Inspector, defect)

x86_64
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: Optimizer, Assigned: Optimizer)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Attached patch WIP (obsolete) — Splinter Review
Works, but breaks tests. Need to fix them.

Asking for feedback to know if this is the right approach.
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
Attachment #790946 - Flags: feedback?(mratcliffe)
Attachment #790946 - Flags: feedback?(jwalker)
Attached patch patch v0.1 (obsolete) — Splinter Review
Fixes test failures.

try at : https://tbpl.mozilla.org/?tree=Try&rev=9b49eceaedcd
Attachment #790946 - Attachment is obsolete: true
Attachment #790946 - Flags: feedback?(mratcliffe)
Attachment #790946 - Flags: feedback?(jwalker)
Attachment #791843 - Flags: review?(mratcliffe)
Attachment #791843 - Flags: feedback?(jwalker)
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 [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
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.
Attached patch Rebased patchSplinter Review
Rebased on latest fx-team.

Multiple green trys:
https://tbpl.mozilla.org/?tree=Try&rev=51663149477a
https://tbpl.mozilla.org/?tree=Try&rev=a8af567e17aa
Attachment #791843 - Attachment is obsolete: true
Attachment #805012 - Flags: review+
landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/5b5ea8584460
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5b5ea8584460
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.