Closed Bug 972267 Opened 6 years ago Closed 6 years ago

Australis: StarUI not responding after toggle position

Categories

(Firefox :: Toolbars and Customization, defect)

30 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: alice0775, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P2])

Attachments

(1 file, 1 obsolete file)

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
Whiteboard: [Australis:P2]
My fix in bug 969780 seems to fix this.
Depends on: 969780
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.
This still needs a test, but other than that, what do you think, Jared?
Attachment #8376572 - Flags: review?(jaws)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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?
(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 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
Attachment #8376572 - Flags: review?(jaws)
Attachment #8376572 - Attachment is obsolete: true
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
  });
Attachment #8378482 - Flags: review?(jaws) → review+
w/ nit fixed,

remote:   https://hg.mozilla.org/integration/fx-team/rev/7c7963536edb
Flags: in-testsuite+
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7c7963536edb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 30
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
Attachment #8378482 - Flags: approval-mozilla-aurora?
Attachment #8378482 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.