974701 - onWidgetBefore/AfterDOMChange don't distinguish destroyed widgets from those removed to the palette

Status: RESOLVED WONTFIX

(Firefox :: Toolbars and Customization, defect)

Description

[Luís Miguel [:quicksaver] (not reading bugmail)]

Ever since bug 964433, onWidgetBefore/AfterDOMChange don't distinguish between the widget being removed to palette or destroyed; both pass a true aWasRemoval parameter and in the case of API widgets the end result is the same: the node is removed from DOM.

Any chance it could pass another parameter "aWasDestroyed"? Otherwise, keeping track of this distinction needs quite the convoluted and unnecessary process on the add-on's end (listening to onWidgetBefore/AfterDOMChange to do something with the node if it was removed, and listening to onWidgetDestroyed to undo what it just did is IMO overkill, not to mention a waste of resources; also can't rely on listening to onWidgetRemoved because it would rely on the "order" that these "listeners" would happen since itself fires onWidgetBefore/AfterDOMChange, a concept I'm not a big fan of)

Comment 1

[Luís Miguel [:quicksaver] (not reading bugmail)]

Food for thought: instead of these passing a bunch of parameters, how about grouping them into an object and pass that as their single parameter? Could make things a little easier on the listeners, maybe perhaps possibly perchance? :)

Comment 2

[:Gijs (he/him)]

(In reply to Luís Miguel [:Quicksaver] from comment #0)
> Ever since bug 964433, onWidgetBefore/AfterDOMChange don't distinguish
> between the widget being removed to palette or destroyed; both pass a true
> aWasRemoval parameter and in the case of API widgets the end result is the
> same: the node is removed from DOM.
> 
> Any chance it could pass another parameter "aWasDestroyed"? Otherwise,
> keeping track of this distinction needs quite the convoluted and unnecessary
> process on the add-on's end (listening to onWidgetBefore/AfterDOMChange to
> do something with the node if it was removed, and listening to
> onWidgetDestroyed to undo what it just did

I don't understand. Only API widgets get destroyed, and as you say, there the endresult is the same. Why do you need to undo things you did when the widget was removed if the widget was destroyed?

Separately, under what circumstances does the widget get destroyed without you knowing, that is, isn't it always you who destroys the widget? Seeing as all the code is sync, that sounds like you should know, too, when the handler gets called, if you're in the process of destroying your widget or not...

It sounds like I'm missing something. Can you clarify?

Comment 3

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to :Gijs Kruitbosch from comment #2)
> under what circumstances does the widget get destroyed without you knowing
Never of course, but this is not what I'm interested in, what I need is the specific case when the widget is removed from a toolbar/panel into the palette. I used to check for the aWasRemoval flag (well, technically for !widgetNode.parentNode, same effect).

But now when destroying the widget, the same onWidgetAfterDOMChange is fired, with the same flag set to true (and widgetNode.parentNode is also null of course).

(In reply to :Gijs Kruitbosch from comment #2)
> I don't understand. Only API widgets get destroyed, and as you say, there
> the endresult is the same. Why do you need to undo things you did when the
> widget was removed if the widget was destroyed?
Basically I have my API-based widgets working in a way as if they were XUL-based, in the sense that when removing them from a toolbar, their nodes end up in the palette element of the toolbox, instead of being removed altogether (I need them somewhere, whether in the palette or somewhere else is irrelevant, but I can't have the nodes completely destroyed; I just use the palette for this because that's what it's there for anyway).

Checking for the aWasRemoval flag of onWIdgetAfterDOMChange, I can move the node as I want; but because onWidgetDestroyed now also fires those, I would have to undo that (remove the node from the palette) in that case by implementing a specific onWidgetDestroyed listener for this. Listening purely for onWidgetRemoved, and if for some reason my listener was fired before CUI's, it would be useless, as CUI's listener would remove it again from the DOM tree.

Comment 4

[:Gijs (he/him)]

(In reply to Luís Miguel [:Quicksaver] from comment #3)
> Listening
> purely for onWidgetRemoved, and if for some reason my listener was fired
> before CUI's

This can never happen. CUI's own listener is added in CUI.init(), which runs synchronously when the module is loaded (Cu.import'd). There is therefore literally no way your listener can be added before CUI's own listener, AIUI. I'm pretty sure that if we reversed the order in which we run listeners, or otherwise messed with this, some of our automated and 'real life' tests would start failing, so I don't think this'll be changing in a hurry.

This all seems pretty niche. I'm going to mark this P5. If someone wants to implement it, they can, but there are so many actual user-facing bugs to work on that I can't justify spending time on this right now.

The relevant code is in CustomizableUI.jsm. The onWidgetBefore/AfterDOMChange calls for destroyWidget would need to be updated to pass an extra argument.

Comment 5

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Luís Miguel [:Quicksaver] from comment #3)
> > Listening
> > purely for onWidgetRemoved, and if for some reason my listener was fired
> > before CUI's
> 
> This can never happen. CUI's own listener is added in CUI.init(), which runs
> synchronously when the module is loaded (Cu.import'd). There is therefore
> literally no way your listener can be added before CUI's own listener, AIUI.
> I'm pretty sure that if we reversed the order in which we run listeners, or
> otherwise messed with this, some of our automated and 'real life' tests
> would start failing, so I don't think this'll be changing in a hurry.

Couldn't an add-on remove CUI's listener (for whatever reason)? I'm really not sure, and because of that I've been running on the assumption that this was possible.

> This all seems pretty niche. I'm going to mark this P5. If someone wants to
> implement it, they can, but there are so many actual user-facing bugs to
> work on that I can't justify spending time on this right now.

Of course, I never considered this a high-priority issue at all, as I said I could get around it myself, just thought I should mention my suggestion. :)

Comment 6

[:Gijs (he/him)]

(In reply to Luís Miguel [:Quicksaver] from comment #5)
> (In reply to :Gijs Kruitbosch from comment #4)
> > (In reply to Luís Miguel [:Quicksaver] from comment #3)
> > > Listening
> > > purely for onWidgetRemoved, and if for some reason my listener was fired
> > > before CUI's
> > 
> > This can never happen. CUI's own listener is added in CUI.init(), which runs
> > synchronously when the module is loaded (Cu.import'd). There is therefore
> > literally no way your listener can be added before CUI's own listener, AIUI.
> > I'm pretty sure that if we reversed the order in which we run listeners, or
> > otherwise messed with this, some of our automated and 'real life' tests
> > would start failing, so I don't think this'll be changing in a hurry.
> 
> Couldn't an add-on remove CUI's listener (for whatever reason)? I'm really
> not sure, and because of that I've been running on the assumption that this
> was possible.

Yes, but that seems like a dumb thing to do. I'm not sure you could do it without breaking the world. Why would anyone do this? :-)

> > This all seems pretty niche. I'm going to mark this P5. If someone wants to
> > implement it, they can, but there are so many actual user-facing bugs to
> > work on that I can't justify spending time on this right now.
> 
> Of course, I never considered this a high-priority issue at all, as I said I
> could get around it myself, just thought I should mention my suggestion. :)

Want to have a go? :-)

Comment 7

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Gijs Kruitbosch (Gone until Jan 5) from comment #6)
> (In reply to Luís Miguel [:Quicksaver] from comment #5)
> > (In reply to :Gijs Kruitbosch from comment #4)
> > > (In reply to Luís Miguel [:Quicksaver] from comment #3)
> > > > Listening
> > > > purely for onWidgetRemoved, and if for some reason my listener was fired
> > > > before CUI's
> > > 
> > > This can never happen. CUI's own listener is added in CUI.init(), which runs
> > > synchronously when the module is loaded (Cu.import'd). There is therefore
> > > literally no way your listener can be added before CUI's own listener, AIUI.
> > > I'm pretty sure that if we reversed the order in which we run listeners, or
> > > otherwise messed with this, some of our automated and 'real life' tests
> > > would start failing, so I don't think this'll be changing in a hurry.
> > 
> > Couldn't an add-on remove CUI's listener (for whatever reason)? I'm really
> > not sure, and because of that I've been running on the assumption that this
> > was possible.
> 
> Yes, but that seems like a dumb thing to do. I'm not sure you could do it
> without breaking the world. Why would anyone do this? :-)

For instance, I already have to modify CUI to get around bug 1058990. If for some reason I had to also modify one of its listeners for the same or any other motive, I would have to remove and re-add it so it takes effect. Obviously this is very very extreme edge case, just saying it's theoretically possible to need to do this. :)

> 
> > > This all seems pretty niche. I'm going to mark this P5. If someone wants to
> > > implement it, they can, but there are so many actual user-facing bugs to
> > > work on that I can't justify spending time on this right now.
> > 
> > Of course, I never considered this a high-priority issue at all, as I said I
> > could get around it myself, just thought I should mention my suggestion. :)
> 
> Want to have a go? :-)

I meant "myself" as in "within the add-on" (but extra kudos if you know that and turned my phrasing around me on purpose!). But either way, I'm swamped with my RL projects and really have no time unfortunately, I'm sorry to disappoint yet again. (I should make this phrase my signature...)

Comment 8

[:Gijs (he/him)]

Without xpcom add-ons I guess there's no pressing need to do this anymore.