Closed
Bug 876926
Opened 12 years ago
Closed 11 years ago
CustomizeMode.jsm should unwrap/re-wrap widgets based on an event sent from CustomizableUI
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
RESOLVED
FIXED
Firefox 28
People
(Reporter: Unfocused, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
(Whiteboard: [Australis:M9][Australis:P3])
Attachments
(3 files, 3 obsolete files)
4.26 KB,
patch
|
jaws
:
review+
Gijs
:
checkin+
|
Details | Diff | Splinter Review |
15.73 KB,
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
8.03 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
In CustomizeMode.jsm, we need to unwrap widgets whenever they're moved, so that CustomizableUI can safely interact with them. However, it makes assumptions as to when it needs to do that - naming, assuming widgets need unwrapped/re-wrapped only for an action performed by CustomizeMode.jsm - which is wrong. Any code can call CustomizableUI's APIs when one of the browser windows is in customize mode, two windows can be in customize mode, or an add-on can be added/removed.
So, CustomizeMode.jsm needs to listen for events for when to unwrap/re-wrap widgets. We have onWidgetMoved/onWidgetAdded/onWidgetRemoved events - CustomizableUI will need to also send events indicating that these are about to happen (ie, onWidgetMoving/onWidgetAdding/onWidgetRemoving), and CustomizeMode needs to unwrap when these events are received, and re-wrap when receiving the existing events.
Additionally, with bug 870011 with have Reset to Defaults working - which also assumes a reset will only happen via that instance of the UI. So that also needs to be triggered off some event (though, maybe it will just work with the events mentioned above).
Comment 1•12 years ago
|
||
Not taking this for Australis:M7.
Comment 3•11 years ago
|
||
Related to bug 885575?
Comment 4•11 years ago
|
||
I'm unclear what the impact of not doing this is. Sounds like something that is unlikely to happen frequently?
Whiteboard: [Australis:M?] → [Australis:P5]
Reporter | ||
Comment 5•11 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #4)
> I'm unclear what the impact of not doing this is. Sounds like something that
> is unlikely to happen frequently?
Shouldn't happen frequently, no. But if an add-on uses the CustomizableUI API while the browser is in customization mode (even if its just because the add-on was updated), the whole customization mode will break... potentially along with parts of the toolbars. So, uh, kinda serious side affects.
Assignee | ||
Comment 6•11 years ago
|
||
(In reply to Blair McBride [:Unfocused] from comment #5)
> (In reply to Justin Dolske [:Dolske] from comment #4)
> > I'm unclear what the impact of not doing this is. Sounds like something that
> > is unlikely to happen frequently?
>
> Shouldn't happen frequently, no. But if an add-on uses the CustomizableUI
> API while the browser is in customization mode (even if its just because the
> add-on was updated), the whole customization mode will break... potentially
> along with parts of the toolbars. So, uh, kinda serious side affects.
Yes. Going to be bold and go for P3 instead, also because this'll involve some core changes that we're going to need to test well, so doing it at the last minute would be very suboptimal.
(In reply to Justin Dolske [:Dolske] from comment #3)
> Related to bug 885575?
Yes.
Whiteboard: [Australis:P5] → [Australis:P3]
Assignee | ||
Comment 7•11 years ago
|
||
I'll poke at this tomorrow.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•11 years ago
|
||
This, essentially. The palette-specific drag code and our position adjusting stuff needed to be taught about all this. Otherwise this works. I'll file a bug on splitting off the positioning stuff into a singleton module tonight/tomorrow, as per some discussion on IRC.
Attachment #796779 -
Flags: review?(jaws)
Comment 9•11 years ago
|
||
Comment on attachment 796779 [details] [diff] [review]
CustomizeMode.jsm should unwrap/re-wrap widgets on an event-based basis,
Review of attachment 796779 [details] [diff] [review]:
-----------------------------------------------------------------
Can bug 885575 be resolved after this is fixed or is there more work to do?
::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +1053,5 @@
>
> +CustomizeMode.prototype.onWidgetAdded =
> +CustomizeMode.prototype.onWidgetMoved =
> +CustomizeMode.prototype.onWidgetRemoved =
> +CustomizeMode.prototype._onUIChange;
I don't think I've seen this style elsewhere. We should either just place CustomizeMode.prototype._onUIChange on each line separately, or indent all lines except the first one.
@@ +1058,5 @@
> +
> +CustomizeMode.prototype.onWidgetBeforeAdding =
> +CustomizeMode.prototype.onWidgetBeforeRemoving =
> +CustomizeMode.prototype.onWidgetBeforeMoving =
> +function _cmBeforeNodeTouch(aWidgetId, aTarget) {
Nit: The cm prefix isn't necessary here since all of these functions are limited to the CustomizeMode.jsm.
Attachment #796779 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #9)
> Comment on attachment 796779 [details] [diff] [review]
> CustomizeMode.jsm should unwrap/re-wrap widgets on an event-based basis,
>
> Review of attachment 796779 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Can bug 885575 be resolved after this is fixed or is there more work to do?
That bug doesn't have a description, so I'm not 100% sure. :-)
Taken literally, we'd need to teach CustomizableUI about toolbarpaletteitems and such. Which would be sadfaces. If that's not actually what that bug wants to be doing, then I think it can be resolved.
Note that a case that we weren't addressing before and aren't addressing now is what happens when an add-on adds (creates) a widget when in customize mode. We should probably add it to the visiblePalette, but that should be a separate bug IMO (we can do that from an onWidgetCreated handler).
Separately, when trying out this patch some more, I realized it's broken sometimes when dragging to/from the panel. Turns out that the culprit for 'sometimes' is here: http://mxr.mozilla.org/projects-central/source/ux/browser/components/customizableui/src/CustomizableUI.jsm?mark=606-615#606 . I'm inclined to just remove the specialcasing and never to reinsert back into the palette, but I'll check again whether that's a good idea when I wake up tomorrow.
Assignee | ||
Comment 11•11 years ago
|
||
Right. So this wasn't nearly enough, it turned out, so I split up my patches. This is the first. It addresses a small bug in getWidgetIdsInArea, and two tiny style/hardening issues.
Attachment #797204 -
Flags: review?(jaws)
Assignee | ||
Comment 12•11 years ago
|
||
This is the core patch. It does require (the functional parts of) the previous patch in order to work.
Attachment #797205 -
Flags: review?(jaws)
Assignee | ||
Comment 13•11 years ago
|
||
Then I wrote tests. They all pass with these patches, as do all the other CustomizableUI tests.
Attachment #797206 -
Flags: review?(jaws)
Assignee | ||
Updated•11 years ago
|
Attachment #796779 -
Attachment is obsolete: true
Comment 14•11 years ago
|
||
Comment on attachment 797204 [details] [diff] [review]
Fix placement id error, and accidental advantage had in toolbar.xml,
Review of attachment 797204 [details] [diff] [review]:
-----------------------------------------------------------------
r+ with the following issue addressed.
::: browser/components/customizableui/content/toolbar.xml
@@ +173,5 @@
> // Get a list of items only in the new list
> let newIds = [id for (id of newVal) if (oldIds.indexOf(id) == -1)];
> CustomizableUI.beginBatchUpdate();
> for (let newId of newIds) {
> + let oldIds = CustomizableUI.getWidgetIdsInArea(this.id);
I would prefer that we not introduce a new scoped variable for this. Just continue to use the previously defined oldIds variable (IOW, remove the 'let ' here).
Attachment #797204 -
Flags: review?(jaws) → review+
Comment 15•11 years ago
|
||
Comment on attachment 797205 [details] [diff] [review]
Use CustomizableUI events to wrap items for CustomizeMode,
Review of attachment 797205 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +612,5 @@
> + if (aTarget.ownerDocument.defaultView != this.window) {
> + return;
> + }
> + afterNodeTouch.call(this, aWidgetId, aTarget);
> + // If an API-based widget is removed while customizing, append it to the palette.
How can a widget be removed in such a way that it is not already destined for the palette?
@@ +1078,5 @@
> }
> };
>
> +function beforeNodeTouch(aWidgetId, aTarget) {
> + if (aTarget.ownerDocument.defaultView != this.window) {
When will the target's window not equal this.window? (same question for the usage below)
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #15)
> Comment on attachment 797205 [details] [diff] [review]
> Use CustomizableUI events to wrap items for CustomizeMode,
>
> Review of attachment 797205 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: browser/components/customizableui/src/CustomizeMode.jsm
> @@ +612,5 @@
> > + if (aTarget.ownerDocument.defaultView != this.window) {
> > + return;
> > + }
> > + afterNodeTouch.call(this, aWidgetId, aTarget);
> > + // If an API-based widget is removed while customizing, append it to the palette.
>
> How can a widget be removed in such a way that it is not already destined
> for the palette?
STR:
1. Open a second window.
2. Open customize mode in one window
3. In your first window, open the panel.
4. Right click on a widget and click "Remove from Menu"
Alternative step 4: have an add-on (install/uninstall) or other code call removeWidgetFromArea.
Per the link in comment 10, API widgets (and special widgets, which is OK) don't get put in the palette automatically when removed, so we have to do it 'manually' when in customize mode.
Thinking more about this, just like for the onWidgetCreated call, we should probably also handle onWidgetDestroyed in Customize Mode, and remove the (now empty) wrapper.
> @@ +1078,5 @@
> > }
> > };
> >
> > +function beforeNodeTouch(aWidgetId, aTarget) {
> > + if (aTarget.ownerDocument.defaultView != this.window) {
>
> When will the target's window not equal this.window? (same question for the
> usage below)
These will get called for each areaNode. So, if you open customize mode in one window, and have a second (or nth) window, all these functions will be called for all those windows.
Assignee | ||
Comment 17•11 years ago
|
||
Comment on attachment 797204 [details] [diff] [review]
Fix placement id error, and accidental advantage had in toolbar.xml,
Pushed this bit already: https://hg.mozilla.org/projects/ux/rev/761ef871425b
Attachment #797204 -
Flags: checkin+
Reporter | ||
Comment 18•11 years ago
|
||
Comment on attachment 797204 [details] [diff] [review]
Fix placement id error, and accidental advantage had in toolbar.xml,
Review of attachment 797204 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1960,5 @@
> throw new Error("Area not yet restored");
> }
>
> + // We need to clone this, as we don't want to let consumers muck with placements
> + return [].concat(gPlacements.get(aArea));
Late driveby nit: Now days we can more elegantly clone arrays, using the spread operator:
[...myArray];
Reporter | ||
Comment 19•11 years ago
|
||
Comment on attachment 797205 [details] [diff] [review]
Use CustomizableUI events to wrap items for CustomizeMode,
Review of attachment 797205 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +564,5 @@
>
> let container = areaNode.customizationTarget;
> let [provider, widgetNode] = this.getWidgetNode(aWidgetId, window);
>
> + this.notifyListeners("onWidgetBeforeAdding", aWidgetId, container);
Why do we need to send this event individually for every window? Doesn't seem like the listeners really need to care about that level of detail (all windows should be the same anyway, right?), but it means a lot more events and needing to filter them.
ie, currently, the order of events is:
* onWidgetBeforeAdding
* [add widget node]
* onWidgetAfterAdding
* onWidgetBeforeAdding
* [add widget node]
* onWidgetAfterAdding
* onWidgetBeforeAdding
* [add widget node]
* onWidgetAfterAdding
Instead of:
* onWidgetBeforeAdding
* [add widget node]
* [add widget node]
* [add widget node]
* onWidgetAfterAdding
Also, I think these events should be passing the same parameters as the existing events (onWidgetAdded, etc).
Assignee | ||
Comment 20•11 years ago
|
||
(In reply to Blair McBride [:Unfocused] from comment #19)
> Comment on attachment 797205 [details] [diff] [review]
> Use CustomizableUI events to wrap items for CustomizeMode,
>
> Review of attachment 797205 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +564,5 @@
> >
> > let container = areaNode.customizationTarget;
> > let [provider, widgetNode] = this.getWidgetNode(aWidgetId, window);
> >
> > + this.notifyListeners("onWidgetBeforeAdding", aWidgetId, container);
>
> Why do we need to send this event individually for every window? Doesn't
> seem like the listeners really need to care about that level of detail (all
> windows should be the same anyway, right?)
No, they aren't. This is also exactly why. One window can be in customization mode, others aren't (by definition, there are only 0-1 customizing windows). Then there are widgets which appear in some windows, but not others (showInPrivateBrowsing). Toolbar overflow is different in different windows, which can have different sizes.
Comment 21•11 years ago
|
||
Comment on attachment 797205 [details] [diff] [review]
Use CustomizableUI events to wrap items for CustomizeMode,
Review of attachment 797205 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +582,5 @@
> if (area.get("type") == CustomizableUI.TYPE_TOOLBAR) {
> areaNode.setAttribute("currentset", areaNode.currentSet);
> }
> +
> + this.notifyListeners("onWidgetAfterAdding", aWidgetId, container, widgetNode);
I agree with Blair that it would be nice if the these events all passed the same parameters.
::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +1084,5 @@
> + }
> + if (aTarget.id == CustomizableUI.AREA_PANEL) {
> + this._removePanelCustomizationPlaceholders();
> + }
> + this._unwrapItemsInArea(this.visiblePalette);
"// TODO(bug 885575): Remove once CustomizeUI can handle moving wrapped widgets." should be placed before this line and before the similar lines in afterNodeTouch.
Attachment #797205 -
Flags: review?(jaws) → review+
Updated•11 years ago
|
Attachment #797206 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 22•11 years ago
|
||
This uses just two events, and only unwraps the required items. Still passes the same tests. I think this is probably our best bet. I don't think moving the wrapping/unwrapping logic that is now in this patch in CustomizeMode.jsm to CustomizableUI.jsm is good for separation of concerns etc. (in effect, I think that we should fix this bug and then wontfix bug 885575). Does that sound agreeable?
Attachment #804291 -
Flags: review?(bmcbride)
Assignee | ||
Updated•11 years ago
|
Attachment #797205 -
Attachment is obsolete: true
Assignee | ||
Comment 23•11 years ago
|
||
The test does need to add the security-related prefs though.
Assignee | ||
Updated•11 years ago
|
Attachment #797206 -
Attachment is obsolete: true
Assignee | ||
Comment 24•11 years ago
|
||
Comment on attachment 804292 [details] [diff] [review]
Tests for CustomizableUI-event-based wrapping behaviour,
Carrying over r+
Attachment #804292 -
Flags: review+
Reporter | ||
Comment 25•11 years ago
|
||
Comment on attachment 804291 [details] [diff] [review]
Use CustomizableUI events to wrap items for CustomizeMode,
Review of attachment 804291 [details] [diff] [review]:
-----------------------------------------------------------------
+1 to comment 22
Attachment #804291 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 26•11 years ago
|
||
https://hg.mozilla.org/projects/ux/rev/80958a628e14
https://hg.mozilla.org/projects/ux/rev/d07c3d3fc083
Jared, per comment 22 / comment 25, and this patch, are you OK wontfixing bug 885575?
Flags: needinfo?(jaws)
Whiteboard: [Australis:P3] → [Australis:M9][Australis:P3][fixed-in-ux]
Comment 27•11 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #26)
> https://hg.mozilla.org/projects/ux/rev/80958a628e14
> https://hg.mozilla.org/projects/ux/rev/d07c3d3fc083
>
> Jared, per comment 22 / comment 25, and this patch, are you OK wontfixing
> bug 885575?
Yep, just closed it out.
Flags: needinfo?(jaws)
Assignee | ||
Comment 28•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/761ef871425b
https://hg.mozilla.org/mozilla-central/rev/d07c3d3fc083
https://hg.mozilla.org/mozilla-central/rev/80958a628e14
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P3][fixed-in-ux] → [Australis:M9][Australis:P3]
Target Milestone: --- → Firefox 28
You need to log in
before you can comment on or make changes to this bug.
Description
•