1194174 - Fixing up the lifetime management for dnd panels used for tabs

Status: NEW

(Firefox :: Tabbed Browser, defect)

Description

[Gabor Krizsanits (INACTIVE)]

See Bug 863512 Comment 39. I assumed that after the dnd session the used panel is removed from the tree but it seems like it is only set to hidden. I guess I should hook up an event listener that listens to the nsXULPopupHidingEvent that is fired but maybe some end dnd session event should be used instead... Neil, what is the preferred strategy to clean up the panel after the dnd session?

Comment 1

[Neil Deakin]

You should either just place a fixed <panel> element directly in the tabbrowser content and not need to add and remove it, or use the dragend event, which always fires on the source of the drag no matter how the drag ended.

Comment 2

[Gabor Krizsanits (INACTIVE)]

Patch in Bug 1194311 covers this one too.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2)
> Patch in Bug 1194311 covers this one too.

Not quite. It still randomly appends the panel to document.documentElement. As suggested in comment 1, the panel should be part of tabbrowser.

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to Dão Gottwald [:dao] from comment #3)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2)
> > Patch in Bug 1194311 covers this one too.
> 
> Not quite. It still randomly appends the panel to document.documentElement.
> As suggested in comment 1, the panel should be part of tabbrowser.

Oh, I might have misunderstood that part. Thanks for noticing it. Anyway, I think the bug in the title is fixed, but I'm totally OK to write up a follow-up patch for this. Since I'm not that experienced on the front end, could you explain to me what is the difference in practice between attaching this panel to the document of the tabbrowser or the "tabbrowser content"? And if the tabbrowser content refers to tabbrowser.contentDocument or something else? Thanks!

Comment 5

[Dão Gottwald [:dao]]

The panel should probably be just here:
http://hg.mozilla.org/mozilla-central/annotate/dd509db16a13/browser/base/content/tabbrowser.xml#l17

The problem with just using the document's root element is that this litters up the DOM with a node that other code running in the same window won't be able to make sense of. tabbrowser.contentDocument is the content document, so you don't want to use that either.