947987 - (Re-)Building an area logic about non-removable widgets is a bit faulty because of API-based widgets

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Attachment: Non-removable widget test

The test package (never mind its name, I'm recycling from another package) creates two API-based widgets, one removable and one not removable, that will be placed by default in the nav-bar.

After you install the xpi, both widgets will be in the nav-bar, but after you restart the browser, only the removable widget will be there. (The widgets have no icons, mouse over the empty buttons to distinguish them by their tooltips)

I think this is because of http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/src/CustomizableUI.jsm#458 in CUI.buildArea():
> if (node.parentNode != container && !this.isWidgetRemovable(node)) {
>   placementsToRemove.add(id);
>   continue;
> }

(node is the widget node) Obviously, node.parentNode is null when it is created, as it has never been appended to the DOM tree at this point, so it will never be == container.

Or am I mis-reading the whole thing? Either way, the non-removable widget is not appended to the toolbar so if it's not this, something else is happening.

Comment 1

[:Gijs (he/him)]

Hrm. This is interesting though, because if we just opened this check up for API-based widgets, you couldn't really have a non-removable widget anymore (that is, if an instance would still need to be created for it, it'd always work, because it would never be in the DOM already, so you wouldn't really be constraining the widget to anywhere).

I suspect what really needs to happen is more involved than just this check. The check should check canWidgetMoveToArea instead of isWidgetRemovable, and we should require a defaultArea for a widget if it's made non-removable. And I'd like to default API-based widgets to removable because that's what people will want 99% of the time anyway.

Comment 2

[:Gijs (he/him)]

(btw, thanks for the (continued) detailed bugreports regarding the API, they are very helpful. I'm working on docs in bug 942393, which hopefully help in terms of understanding the desired behaviour, once I've written some more of them...)

Comment 3

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

(In reply to :Gijs Kruitbosch from comment #1)
> The check should check canWidgetMoveToArea instead of isWidgetRemovable, and
> we should require a defaultArea for a widget if it's made non-removable. And
> I'd like to default API-based widgets to removable because that's what
> people will want 99% of the time anyway.

I agree, sounds good to me, but only if it is ensured that the non-removable widget ends up in its defaultArea if it has no placement.

For example (I haven't tested this, just throwing it out of my head from reading the code) if I create a custom aToolbar, then create a non-removable widget defaulting to aToolbar, then unregisterArea with aDestroyPlacements = true and then destroy the widget. At this point the widget placement is destroyed, but if I re-create aToolbar and the widget (in that order) the widget won't be placed in its defaultArea because gSeenWidgets will have the widget, and so its defaultArea won't be used for placing it. I think the defaultPlacements handling in restoreStateForArea only catches this situation if the toolbar is registered before the widget is created, or am I wrong?

(In reply to :Gijs Kruitbosch from comment #2)
> (btw, thanks for the (continued) detailed bugreports regarding the API, they
> are very helpful. I'm working on docs in bug 942393, which hopefully help in
> terms of understanding the desired behaviour, once I've written some more of
> them...)

You're very welcome! You can thank my incredibly convoluted add-on that can apparently make use of everything in CUI apart from custom panels (and I'm already at the "hmm..." stage concerning those :) )

Comment 4

[:Gijs (he/him)]

I'll try to get to this today or tomorrow.

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #1)
> I suspect what really needs to happen is more involved than just this check.
> The check should check canWidgetMoveToArea instead of isWidgetRemovable,

Hrm. But this doesn't work either because at the point where buildArea is called the widget will already be in that area when looking purely at placements. Conundrum. :-\

Comment 6

[:Gijs (he/him)]

(In reply to Luís Miguel [:Quicksaver] from comment #3)
> (In reply to :Gijs Kruitbosch from comment #1)
> > The check should check canWidgetMoveToArea instead of isWidgetRemovable, and
> > we should require a defaultArea for a widget if it's made non-removable. And
> > I'd like to default API-based widgets to removable because that's what
> > people will want 99% of the time anyway.
> 
> I agree, sounds good to me, but only if it is ensured that the non-removable
> widget ends up in its defaultArea if it has no placement.
> 
> For example (I haven't tested this, just throwing it out of my head from
> reading the code) if I create a custom aToolbar, then create a non-removable
> widget defaulting to aToolbar, then unregisterArea with aDestroyPlacements =
> true and then destroy the widget. At this point the widget placement is
> destroyed, but if I re-create aToolbar and the widget (in that order) the
> widget won't be placed in its defaultArea because gSeenWidgets will have the
> widget, and so its defaultArea won't be used for placing it. I think the
> defaultPlacements handling in restoreStateForArea only catches this
> situation if the toolbar is registered before the widget is created, or am I
> wrong?

I think you're right. Separate issue, though, which already existed without this, right? One bug at a time and all that. :-)

Comment 7

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

(In reply to :Gijs Kruitbosch from comment #5)
> Hrm. But this doesn't work either because at the point where buildArea is
> called the widget will already be in that area when looking purely at
> placements. Conundrum. :-\

I didn't understand this, if the widget is already in that area (from placements info), then canWidgetMoveToArea will return true and buildArea will proceed forward, instead of "continue"ing the widgets loop. (sorry, I'm just trying to make sense of it)

(In reply to :Gijs Kruitbosch from comment #6)
> I think you're right. Separate issue, though, which already existed without
> this, right? One bug at a time and all that. :-)

Since it was also about non-removable widgets not being properly positioned in toolbars, I thought it fit in well here, and to be honest I thought it would be easier to fix by tackling it together with this bug (something like "make sure non-removable widgets are properly positioned -always" instead of "-specifically when..."). But I'll try to come up with a reproducible test-case and I'll open a new bug if I can. I'm not sure if it exists now because this bug kind of gets in the way of that particular process. :)

Comment 8

[:Gijs (he/him)]

(In reply to Luís Miguel [:Quicksaver] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #5)
> > Hrm. But this doesn't work either because at the point where buildArea is
> > called the widget will already be in that area when looking purely at
> > placements. Conundrum. :-\
> 
> I didn't understand this, if the widget is already in that area (from
> placements info), then canWidgetMoveToArea will return true and buildArea
> will proceed forward, instead of "continue"ing the widgets loop. (sorry, I'm
> just trying to make sense of it)

Right. So imagine the following, on mac:

0. Start Fx with one window
1. Add a XUL button to the navbar, non-removable.
2. Close the window
3. Some timeout/addon/whatever calls CustomizableUI.addWidgetToArea(widget, AREA_TABSTRIP);

This would be stopped if we knew the widget was removable. But we don't, because there are no windows and we have no information about the XUL widget.

So, this will go ahead and update the placements information for the widget to move to the tabstrip.

4. Reopen a browser window.

As it is, even if the tabstrip is restored first (if the navbar is restored first, we will pull it back into the placements there because the node is there and we can't remove it, so it gets inserted into the placements and removed from wherever it was), we will nuke the button out of there because it's nonremovable. Checking canWidgetMoveToArea instead of isWidgetRemovable and manually checking the location of the actual widget node will return true and therefore not nuke the node, because, while the widget is not removable, the placements information indicates it is already in the desired area, so it claims that yes, we can move the node, because there's no need for it to change area.

Make more sense this way?

(and yeah, the removability thing causes a lot of headaches implementation-wise)

Comment 9

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

Yes it does, (thanks for the walk-through btw), I was considering only API-widgets for which we do have that information, so I wasn't seeing that problem. Also here:
> This would be stopped if we knew the widget was removable.
I'm assuming you meant "... if we knew the widget was not removable.", otherwise I am completely lost and you should probably just give up on me. :)

How about this:
> if (node.parentNode != container && (
>   ( provider == CUI.PROVIDER_API && !this.canWidgetMoveToArea(id, aArea) )
>   || ( provider == CUI.PROVIDER_XUL && !this.isWidgetRemovable(node) )
> )

Basically, API-widgets would be handled according to their placements info, and XUL-widgets would still be removed from the area if they're not supposed to be there, as they are now. It seems a bit ugly-looking yes, but from your words, it seems like the best approach to treat both kinds of widgets (API and XUL) correctly would be to treat them differently like this. Or not. :)

Comment 10

[:Gijs (he/him)]

Attachment: Adjust how Australis' CustomizableUI deals with removable widgets,

So this addresses the original issue in this bug, and swaps our default to removable: true, and adds tests (whee, tests).

Comment 11

[:Gijs (he/him)]

(In reply to Luís Miguel [:Quicksaver] from comment #9)
> Yes it does, (thanks for the walk-through btw), I was considering only
> API-widgets for which we do have that information, so I wasn't seeing that
> problem. Also here:
> > This would be stopped if we knew the widget was removable.
> I'm assuming you meant "... if we knew the widget was not removable.",
> otherwise I am completely lost and you should probably just give up on me. :)

Uh, yeah, "not removable" was what I meant. Someone had better check those docs I wrote...


> How about this:
> > if (node.parentNode != container && (
> >   ( provider == CUI.PROVIDER_API && !this.canWidgetMoveToArea(id, aArea) )
> >   || ( provider == CUI.PROVIDER_XUL && !this.isWidgetRemovable(node) )
> > )
> 
> Basically, API-widgets would be handled according to their placements info,
> and XUL-widgets would still be removed from the area if they're not supposed
> to be there, as they are now. It seems a bit ugly-looking yes, but from your
> words, it seems like the best approach to treat both kinds of widgets (API
> and XUL) correctly would be to treat them differently like this. Or not. :)

I did something similar, yeah. :-)

Comment 12

[:Gijs (he/him)]

(In reply to Luís Miguel [:Quicksaver] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > I think you're right. Separate issue, though, which already existed without
> > this, right? One bug at a time and all that. :-)
> 
> Since it was also about non-removable widgets not being properly positioned
> in toolbars, I thought it fit in well here, and to be honest I thought it
> would be easier to fix by tackling it together with this bug (something like
> "make sure non-removable widgets are properly positioned -always" instead of
> "-specifically when..."). But I'll try to come up with a reproducible
> test-case and I'll open a new bug if I can. I'm not sure if it exists now
> because this bug kind of gets in the way of that particular process. :)

As I said, I think your analysis is correct (note also that if you create a widget with defaultArea = "somearea" and that area is not registered, the defaultArea part of the spec will be ignored, so you can only do this by first registering the area and then adding the widget). I'll try to look at the issue tomorrow or otherwise next week, and file a bug.

If you want to add to the awesome you're doing, you could try to get a build env and write a browser mochitest that tests the scenario you described, but don't feel obliged. :-)

Comment 13

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

(In reply to :Gijs Kruitbosch from comment #12)
> If you want to add to the awesome you're doing, you could try to get a build
> env and write a browser mochitest that tests the scenario you described, but
> don't feel obliged. :-)

I do need to learn how to work these things some day, this isn't the first time it would have come in handy. :) But realistically speaking, this is one of those things that I wouldn't even know where to start. So it's not something I can invest any more time in right now to learn a new craft, so-to-speak (I'm already spending far longer than I should on my own add-ons), not until the end of the month at least. Just thought I should at least mention this, to justify why I can't be of much more help besides my usual "Hey, I noticed something..."s, and not leave you hanging for an answer on this from me.

Comment 14

[:Gijs (he/him)]

(In reply to Luís Miguel [:Quicksaver] from comment #13)
> (In reply to :Gijs Kruitbosch from comment #12)
> > If you want to add to the awesome you're doing, you could try to get a build
> > env and write a browser mochitest that tests the scenario you described, but
> > don't feel obliged. :-)
> 
> I do need to learn how to work these things some day, this isn't the first
> time it would have come in handy. :) But realistically speaking, this is one
> of those things that I wouldn't even know where to start. So it's not
> something I can invest any more time in right now to learn a new craft,
> so-to-speak (I'm already spending far longer than I should on my own
> add-ons), not until the end of the month at least. Just thought I should at
> least mention this, to justify why I can't be of much more help besides my
> usual "Hey, I noticed something..."s, and not leave you hanging for an
> answer on this from me.

No worries. I just filed bug 948985 for this, and I'll deal with it there once this gets review and lands.

Comment 15

[:Gijs (he/him)]

Attachment: Fix other removable widgets test.

Just realized I needed to fix this test because it tries to use API-based widgets that aren't removable without specifying a defaultArea, and specifying a defaultArea would defeat the point (which is to check non-removable-non-default widgets don't get taken into account for restore defaults, as otherwise they'd mean we'd never be in default state...). Will land this together with the r+'d patch in a bit.

Comment 16

[:Gijs (he/him)]

Landed as a single patch,

remote:   https://hg.mozilla.org/integration/fx-team/rev/a804603b87c6

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a804603b87c6

Comment 18

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

Just to confirm that I tested both with the xpi package attached and with my add-on, and the patch seems to work great.