995164 - Creating a custom toolbar while in customize mode doesn't make it immediately accessible

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

STR:

1. New profile in latest Nightly
2. Enter customize mode:
3. Create a custom toolbar though browser console (just like in bug 989689):
> var testBar = document.createElement('toolbar');
> testBar.id = 'testBar';
> testBar.setAttribute('toolbarname', 'Test Bar');
> testBar.hidden = false;
> testBar.collapsed = false;
> testBar.setAttribute('class', 'toolbar-primary chromeclass-toolbar');
> testBar.setAttribute('context', 'toolbar-context-menu');
> testBar.setAttribute('customizable', 'true');
> CustomizableUI.registerArea('testBar', 
>   { type: CustomizableUI.TYPE_TOOLBAR, legacy: false }
> );
> gNavToolbox.appendChild(testBar);

If you try to drag any widget into the testBar (or out of it, if you had previously added anything to it), it won't work. It seems like it can be customized (has all the hover effects and all), but the widgets won't land there, or can't be dragged out of it. You need to close customize mode and open it again to customize the toolbar.

This seems important (I don't think the system was meant to force new toolbars to be added only when not in customize mode, please let me know if I'm mistaken), so I'm CC'ing you directly again.

Comment 1

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

Addendum: closing customize mode after adding the toolbar also makes the widgets unclickable, it still tries to drag them as if it were still in customize mode. Only after re-entering customize mode does everything go back to normal (so far as I've seen at least).

Comment 2

[:Gijs (he/him)]

Tentatively marking as P3-, although it's a bit of an edgecase and might be closer to P4...

Unfortunately I suspect fixing this will require API changes, which I'd rather have in 29, hence the P3-.

Comment 3

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

For what it's worth, I don't think it's so edgy-casy. Many users use multiple windows, and if one of them is in customize mode and in the other they enable/restart an add-on, or if an add-on updates in the background (this is possible right?) while in customize mode for example they would experience this.

But of course the fix is easy for the user (close and re-open customize mode) and for the add-on (close customize mode before any changes are made), so I can see how by that it wouldn't be very high priority. Just to say, I don't think it's as edgy as you make it sound. :)

Comment 4

[:Gijs (he/him)]

(In reply to Luís Miguel [:Quicksaver] from comment #3)
> For what it's worth, I don't think it's so edgy-casy. Many users use
> multiple windows
I think this isn't true on Mozilla's scale (ie, comparatively few people use multiple windows)

> and if one of them is in customize mode
This is also not very likely.

> and in the other
> they enable/restart an add-on, or if an add-on updates in the background
> (this is possible right?)

I'm not sure it is.

Comment 5

[:Gijs (he/him)]

Attachment: registerArea should work correctly with customize mode,

I believe this should fix the issue (wrote the test first, then went to fix everything else...) took a bit more work than I had hoped, but it works now.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8405925 [details] [diff] [review]
registerArea should work correctly with customize mode,

Review of attachment 8405925 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/test/head.js
@@ +86,5 @@
>  }
>  
>  function removeCustomToolbars() {
> +  if (window.CustomizationHandler.isCustomizing()) {
> +    gCustomizeMode.reset();

Hrm, this should probably return a promise so that we can wait for this to finish...

Comment 7

[:Gijs (he/him)]

Comment on attachment 8405925 [details] [diff] [review]
registerArea should work correctly with customize mode,

Clearing review because the test needs improvements.

Comment 8

[:Gijs (he/him)]

Attachment: registerArea should work correctly with customize mode,

Better test, less head.js futzing.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8405930 [details] [diff] [review]
registerArea should work correctly with customize mode,

Review of attachment 8405930 [details] [diff] [review]:
-----------------------------------------------------------------

This looks sensible - but also a bit risky for a 29 uplift, so I suspect you weren't hoping for that?

I'd like to see a CART run with this patch compared with a baseline. I'm particularly worried about the removal of the deferred toolbar item wrapping method calls.

We should also set dev-doc-needed for the new events.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +2618,5 @@
> +   *     could fire when the node has just been registered with CustomizableUI
> +   *     after an add-on update or disable/enable sequence.
> +   *   - onAreaNodeUnregistered(aArea, aContainer)
> +   *     Fired when an area node is explicitly unregistered by an API caller.
> +   *     Does NOT fire when its containing window unloads and that causes the

Hm, not exactly intuitive, but I get the rationale...

Perhaps instead, each event could be passed a boolean to indicate whether the reason was due to a window opening / closing, or for some other reason (add-on enabled / disabled). Just an idea. I'm not adamant about it, but it would make these two events more...er... "symmetrical"? There might be a better word for it.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +891,5 @@
>      return Task.spawn(function() {
>        this.areas = [];
>        for (let area of CustomizableUI.areas) {
>          let target = CustomizableUI.getCustomizeTargetForArea(area, window);
> +        this._wrapItemsInArea(target);

Hm, this isn't _exactly_ the same. Before, the deferredWrapToolbarItem allows us to let the event loop tick a few times between wrappings. _wrapItemsInArea does them all in one shot...

@@ +933,5 @@
>  
>    _unwrapToolbarItems: function() {
>      return Task.spawn(function() {
>        for (let target of this.areas) {
> +        this._unwrapItemsInArea(target);

Same as above - we lose the event loop breathes by doing it this way...

::: browser/components/customizableui/test/browser_993322_widget_notoolbar.js
@@ +32,5 @@
>    CustomizableUI.destroyWidget(BUTTONID);
> +  try {
> +    CustomizableUI.unregisterArea(TOOLBARID, true);
> +  } catch (ex) {
> +    ok(false, "Failed to properly unregister area");

Why is this now necessary? If we're doing this, we should probably output the ex.message while we're at it.

::: browser/components/customizableui/test/browser_995164_registerArea_during_customize_mode.js
@@ +43,5 @@
> +  CustomizableUI.unregisterArea(TOOLBARID);
> +  ok(CustomizableUI.inDefaultState, "Now that the toolbar is no longer registered, should be in default state.");
> +  ok(!(new Set(gCustomizeMode.areas)).has(toolbar), "Toolbar shouldn't be known to customize mode.");
> +
> +

Nit - extra newline.

Comment 10

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #9)
> Comment on attachment 8405930 [details] [diff] [review]
> registerArea should work correctly with customize mode,
> 
> Review of attachment 8405930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks sensible - but also a bit risky for a 29 uplift, so I suspect you
> weren't hoping for that?

Actually, yes, if plausible I think we should uplift this. I'm uncomfortable with the public API we propose being able to mess itself up like it does right now.

> I'd like to see a CART run with this patch compared with a baseline. I'm
> particularly worried about the removal of the deferred toolbar item wrapping
> method calls.

If I revert the relevant bits of yield-removal, do you still need one? (needinfo for this)

> We should also set dev-doc-needed for the new events.

Done.

> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +2618,5 @@
> > +   *     could fire when the node has just been registered with CustomizableUI
> > +   *     after an add-on update or disable/enable sequence.
> > +   *   - onAreaNodeUnregistered(aArea, aContainer)
> > +   *     Fired when an area node is explicitly unregistered by an API caller.
> > +   *     Does NOT fire when its containing window unloads and that causes the
> 
> Hm, not exactly intuitive, but I get the rationale...
> 
> Perhaps instead, each event could be passed a boolean to indicate whether
> the reason was due to a window opening / closing, or for some other reason
> (add-on enabled / disabled). Just an idea. I'm not adamant about it, but it
> would make these two events more...er... "symmetrical"? There might be a
> better word for it.

I suspect people won't be interested in the 'window died' case, but perhaps I'm wrong. In any case, I guess I can do this, yes.

> ::: browser/components/customizableui/src/CustomizeMode.jsm
> @@ +891,5 @@
> >      return Task.spawn(function() {
> >        this.areas = [];
> >        for (let area of CustomizableUI.areas) {
> >          let target = CustomizableUI.getCustomizeTargetForArea(area, window);
> > +        this._wrapItemsInArea(target);
> 
> Hm, this isn't _exactly_ the same. Before, the deferredWrapToolbarItem
> allows us to let the event loop tick a few times between wrappings.
> _wrapItemsInArea does them all in one shot...

Ugh, I should have realized this. The refactoring isn't necessary, so I'll just undo it instead.


> ::: browser/components/customizableui/test/browser_993322_widget_notoolbar.js
> @@ +32,5 @@
> >    CustomizableUI.destroyWidget(BUTTONID);
> > +  try {
> > +    CustomizableUI.unregisterArea(TOOLBARID, true);
> > +  } catch (ex) {
> > +    ok(false, "Failed to properly unregister area");
> 
> Why is this now necessary? If we're doing this, we should probably output
> the ex.message while we're at it.

It isn't *necessary* - debug code I left in - but it doesn't hurt much. I should probably still remove it.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Yeah, if you revert the yield bits, then I'm last concerned about CART. :)

Comment 12

[Mike Conley (:mconley) (:⚙️)]

s/last/less

Comment 13

[:Gijs (he/him)]

Attachment: registerArea should work correctly with customize mode,

Now with a reason parameter, plus associated constants, plus a test for that functionality.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8406489 [details] [diff] [review]
registerArea should work correctly with customize mode,

Review of attachment 8406489 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, I like this better - thanks Gijs.

The one thing to point out is that your test is absolutely humongous - it could probably be broken up into smaller tests. I'm fine landing it this way, but we should probably file a bug to break that test up a bit.

::: browser/components/customizableui/test/browser_995164_registerArea_during_customize_mode.js
@@ +6,5 @@
> +
> +const TOOLBARID = "test-toolbar-added-during-customize-mode";
> +
> +add_task(function*() {
> +  yield startCustomizing();

This is an absolutely massive test, and it seems to me it's testing a number of smaller things.

I think we should (at some point) break this up.

Comment 15

[:Gijs (he/him)]

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

Comment 16

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #14)
> Comment on attachment 8406489 [details] [diff] [review]
> registerArea should work correctly with customize mode,
> 
> Review of attachment 8406489 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes, I like this better - thanks Gijs.
> 
> The one thing to point out is that your test is absolutely humongous - it
> could probably be broken up into smaller tests. I'm fine landing it this
> way, but we should probably file a bug to break that test up a bit.

Filed bug 996324.

Note that actually, it could probably be formulated to be shorter. Could also split up the events testing from the testing that customize mode deals correctly with such events.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8406489 [details] [diff] [review]
registerArea should work correctly with customize mode,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: adding areas while customize mode is open doesn't integrate them with customize mode
Testing completed (on m-c, etc.): has automated test, hopefully reaches m-c in the next few hours
Risk to taking this patch (and alternatives if risky): medium. I can't pretend this is a tiny patch with no risk. OTOH, it's mostly generating events, plus making customize mode do stuff with those events, and for both the events and the customize mode actions, there's an automated test. Assuming this sticks and makes it through a nightly (Tuesday's) with no regressions turn up (Wednesday), I'd like to land this on 29 for consistency and add-on compat.
String or IDL/UUID changes made by this patch: none, but added two more events to CustomizableUI listeners (in a JSM).

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8406489 [details] [diff] [review]
registerArea should work correctly with customize mode,

Gijs, I am sorry but I think I have to decline this uplift for beta. I will now only accept critical stability bugs for beta9...

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/e72e0a2f0bef

Comment 21

[u279076]

Flagging in-testsuite+ since this landed with tests.

Comment 22

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

Quick question, these events are still not documented in https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/CustomizableUI.jsm, it's been a few releases already and they're kind of important. Should I also file a new bug for these kinds of things or just ask here directly? (I'm not sure how docs-related things should be handled.)

Comment 23

[:Gijs (he/him)]

(In reply to Luís Miguel [:Quicksaver] from comment #22)
> Quick question, these events are still not documented in
> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> CustomizableUI.jsm, it's been a few releases already and they're kind of
> important. Should I also file a new bug for these kinds of things or just
> ask here directly? (I'm not sure how docs-related things should be handled.)

It's a wiki, you should feel free to add stuff yourself! As it is, I just added docs: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/CustomizableUI.jsm$compare?to=681137&from=615461

Please feel free to improve if necessary.

Comment 24

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

I wasn't sure if I should, but I will do from now on, thanks for the info.