Can't catch onWidgetAfterDOMChange notification

RESOLVED INVALID

Status

()

Firefox
Toolbars and Customization
RESOLVED INVALID
5 years ago
5 years ago

People

(Reporter: quicksaver, Unassigned)

Tracking

(Blocks: 2 bugs)

Trunk
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(I'm not sure if this is specific to my OS, as I've only tested in Windows 7, so I'm leaving that info as is for now)

During my debugging, I set up a dummy listener to be registered for CustomizableUI notifications. The dummy listener successfully catches the  onWidgetBeforeDOMChange notifications, but it doesn't catch any  onWidgetAfterDOMChange notification. Not when unregisterArea (with its subsequent onWidgetRemoved calls) is called, or when the widgets are first added my add-on's custom toolbar either.

I noticed that the code removeChild()'s, appendChild()'s or insertBefore()'s between those points, without keeping reference to the new nodes. It is my understanding that these methods kill the previous references, so you have to do something like "widgetNode = parent.removeChild(widgetNode)" to keep the reference, maybe that's what's keeping the onWidgetAfterDOMChange notification from firing, since it doesn't have the widgetNode reference after it's been moved?

Possibly related: I'm trying to fix a couple of remaining bugs in my add-on, after migrating to Australis code. One of the bugs happens when calling unregisterArea to remove the toolbar node, it doesn't send the widget nodes in it into the palette, instead it destroys the nodes completely.

It seems it expects the nodes to be re-created in the palette when entering customize mode after that; I think this is another situation that should be considered for bug 944887, even though the same "you can clean up before you call unregisterArea" solution applies here. But could this issue be related with not catching the notification? At least it seems to me that there is some case when the widgets would be moved to the palette, but apparently that is not being triggered because gPalette.has(aWidgetId) is true. When would this return false?

Comment 1

5 years ago
(In reply to Luís Miguel [:Quicksaver] from comment #1)
> (I'm not sure if this is specific to my OS, as I've only tested in Windows
> 7, so I'm leaving that info as is for now)
> 
> During my debugging, I set up a dummy listener to be registered for
> CustomizableUI notifications. The dummy listener successfully catches the 
> onWidgetBeforeDOMChange notifications, but it doesn't catch any 
> onWidgetAfterDOMChange notification. Not when unregisterArea (with its
> subsequent onWidgetRemoved calls) is called, or when the widgets are first
> added my add-on's custom toolbar either.

Can you come up with a reduced testcase? Customize Mode itself uses these notifications, and works, so presumably they do work, generally speaking. Without more detail it's hard to know why they don't in your case.

> I noticed that the code removeChild()'s, appendChild()'s or insertBefore()'s
> between those points, without keeping reference to the new nodes. It is my
> understanding that these methods kill the previous references, so you have
> to do something like "widgetNode = parent.removeChild(widgetNode)" to keep
> the reference, maybe that's what's keeping the onWidgetAfterDOMChange
> notification from firing, since it doesn't have the widgetNode reference
> after it's been moved?

I don't understand. If you have a variable referencing a DOM node, removing that node from the DOM (or appending it elsewhere) does NOT invalidate the reference, as a general rule.

> 
> Possibly related: I'm trying to fix a couple of remaining bugs in my add-on,
> after migrating to Australis code. One of the bugs happens when calling
> unregisterArea to remove the toolbar node, it doesn't send the widget nodes
> in it into the palette, instead it destroys the nodes completely.
> 
> It seems it expects the nodes to be re-created in the palette when entering
> customize mode after that; I think this is another situation that should be
> considered for bug 944887, even though the same "you can clean up before you
> call unregisterArea" solution applies here. But could this issue be related
> with not catching the notification? At least it seems to me that there is
> some case when the widgets would be moved to the palette, but apparently
> that is not being triggered because gPalette.has(aWidgetId) is true. When
> would this return false?

One issue per bug, please. This, too, would greatly benefit from a reduced testcase.

Updated

5 years ago
Flags: needinfo?(quicksaver)
(In reply to :Gijs Kruitbosch from comment #1)
> Can you come up with a reduced testcase? Customize Mode itself uses these
> notifications, and works, so presumably they do work, generally speaking.
> Without more detail it's hard to know why they don't in your case.

Sorry, this bug can be marked as invalid. It was a typo in my own code which wasn't throwing anything but was preventing the notification from being caught. I only saw that when rewriting the thing to the testcase I was making for this.

> I don't understand. If you have a variable referencing a DOM node, removing
> that node from the DOM (or appending it elsewhere) does NOT invalidate the
> reference, as a general rule.

Sorry again, I have no idea where I got that misconception from, maybe I misread the MDN docs at the time, but of course they confirm what you said. Twice wrong in a single bug, this is a personal best for me :)

> > Possibly related: I'm trying to fix a couple of remaining bugs in my add-on,
> > after migrating to Australis code. One of the bugs happens when calling
> > unregisterArea to remove the toolbar node, it doesn't send the widget nodes
> > in it into the palette, instead it destroys the nodes completely.
> > 
> > It seems it expects the nodes to be re-created in the palette when entering
> > customize mode after that; I think this is another situation that should be
> > considered for bug 944887, even though the same "you can clean up before you
> > call unregisterArea" solution applies here. But could this issue be related
> > with not catching the notification? At least it seems to me that there is
> > some case when the widgets would be moved to the palette, but apparently
> > that is not being triggered because gPalette.has(aWidgetId) is true. When
> > would this return false?
> 
> One issue per bug, please. This, too, would greatly benefit from a reduced
> testcase.

I'll open a new bug if I find I can't figure this one out, I just thought that it might have been related. The "When is widgetNode moved to the palette instead of being destroyed on unregisterArea?" question stands though, even though I'll understand not getting an answer in this bug of course. :)
Flags: needinfo?(quicksaver)

Comment 3

5 years ago
> > > Possibly related: I'm trying to fix a couple of remaining bugs in my add-on,
> > > after migrating to Australis code. One of the bugs happens when calling
> > > unregisterArea to remove the toolbar node, it doesn't send the widget nodes
> > > in it into the palette, instead it destroys the nodes completely.
> > > 
> > > It seems it expects the nodes to be re-created in the palette when entering
> > > customize mode after that; I think this is another situation that should be
> > > considered for bug 944887, even though the same "you can clean up before you
> > > call unregisterArea" solution applies here. But could this issue be related
> > > with not catching the notification? At least it seems to me that there is
> > > some case when the widgets would be moved to the palette, but apparently
> > > that is not being triggered because gPalette.has(aWidgetId) is true. When
> > > would this return false?
> > 
> > One issue per bug, please. This, too, would greatly benefit from a reduced
> > testcase.
> 
> I'll open a new bug if I find I can't figure this one out, I just thought
> that it might have been related. The "When is widgetNode moved to the
> palette instead of being destroyed on unregisterArea?" question stands
> though, even though I'll understand not getting an answer in this bug of
> course. :)

We call removeWidgetFromArea on all the widgets. That would trigger onWidgetRemoved in CustomizableUI, which takes care of moving the physical elements to the palette. Unless, of course, there are elements in the area that aren't removable, in which case they'll stay in there because removeWidgetFromArea will bail out early. If you then remove the containing element (toolbar), the inner elements (toolbarbutton removable="false" or whatever) would be lost. I don't think that's fixable on our side.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID

Comment 4

5 years ago
Oh, exception being: if you remove an API-based or special (spacer/separator/spring) widget, those just get nuked.
(In reply to :Gijs Kruitbosch from comment #4)
> Oh, exception being: if you remove an API-based or special
> (spacer/separator/spring) widget, those just get nuked.

API-based being those widgets created through CUI.createWidget?

Comment 6

5 years ago
(In reply to Luís Miguel [:Quicksaver] from comment #5)
> (In reply to :Gijs Kruitbosch from comment #4)
> > Oh, exception being: if you remove an API-based or special
> > (spacer/separator/spring) widget, those just get nuked.
> 
> API-based being those widgets created through CUI.createWidget?

Yes.
You need to log in before you can comment on or make changes to this bug.