972267 - Australis: StarUI not responding after toggle position

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Alice0775 White]

Steps To Reproduce:
1. Start Firefox[A]
2. Open New Window[B]

3. Click StarUI on Window[B]
   --- bookmarked
4. Right click on StarUI And Move to Menu

5. Switch window[A]
6. Open PanelUI and Right click on StarUI and Move to Toolbar

7. Switch window[B]
8. Click StarUI

Actual Results:
Australis: StarUI not responding

Comment 1

[:Gijs (out for now; he/him)]

My fix in bug 969780 seems to fix this.

Comment 2

[:Gijs (out for now; he/him)]

However... this will affect anyone else relying on the customizationchange event as well, as the basic issue is that the context menu methods only transmit that event in one window. I can do a patch for that bug in here, and that should also fix the issue.

Comment 3

[:Gijs (out for now; he/him)]

Attachment: adjust Australis' dispatching of customization events so customizationchange fires on all windows,

This still needs a test, but other than that, what do you think, Jared?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8376572 [details] [diff] [review]
adjust Australis' dispatching of customization events so customizationchange fires on all windows,

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

::: browser/components/customizableui/content/toolbar.xml
@@ +534,5 @@
>            this._updateMigratedSet();
>            // We will now have moved stuff around; kick off an aftercustomization event
>            // so add-ons know we've just moved their stuff:
>            if (window.gCustomizeMode) {
> +            CustomizableUI.dispatchToolboxEvent("customizationchange", {}, window);

Why do we still need the window.gCustomizeMode check here?

Comment 5

[:Gijs (out for now; he/him)]

(In reply to Jared Wein [:jaws] from comment #4)
> Comment on attachment 8376572 [details] [diff] [review]
> adjust Australis' dispatching of customization events so customizationchange
> fires on all windows,
> 
> Review of attachment 8376572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/content/toolbar.xml
> @@ +534,5 @@
> >            this._updateMigratedSet();
> >            // We will now have moved stuff around; kick off an aftercustomization event
> >            // so add-ons know we've just moved their stuff:
> >            if (window.gCustomizeMode) {
> > +            CustomizableUI.dispatchToolboxEvent("customizationchange", {}, window);
> 
> Why do we still need the window.gCustomizeMode check here?

Because in customize mode, _onUIChange sends this event.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8376572 [details] [diff] [review]
adjust Australis' dispatching of customization events so customizationchange fires on all windows,

Clearing review request because Gijs said on IRC that this 'if (window.gCustomizeMode)' check is broken and will need to be investigated further.

<Gijs> it will need tweaking to actually check if we're not in customize mode, though

Comment 7

[:Gijs (out for now; he/him)]

Attachment: adjust Australis' dispatching of customization events so customizationchange fires on all windows,

With that bit fixed + a test

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8378482 [details] [diff] [review]
adjust Australis' dispatching of customization events so customizationchange fires on all windows,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1759,5 @@
>    },
>  
> +  _dispatchToolboxEventToWindow: function(aEventType, aDetails, aWindow) {
> +    let evt = aWindow.document.createEvent("CustomEvent");
> +    evt.initCustomEvent(aEventType, true, true, aDetails);

createEvent() is deprecated - use the CustomEvent constructor:

  let event = new aWindow.defaultView.CustomEvent(aEventType, {
    bubbles: true,
    cancelable: true,
    details: aDetails
  });

Comment 9

[:Gijs (out for now; he/him)]

w/ nit fixed,

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7c7963536edb

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8378482 [details] [diff] [review]
adjust Australis' dispatching of customization events so customizationchange fires on all windows,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: bookmarks button breaks if it's moved about
Testing completed (on m-c, etc.): on m-c, has automated test
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

Comment 12

[:Gijs (out for now; he/him)]

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