Our new widget definition mechanism allows developers to specify the areas that a widget is allowed to be in. We aren't currently enforcing this. We should do that - and maybe make some reasonable assumptions for XUL toolbar items that don't get this list of areas.

I think we should consider changing that to area *types*, rather than specific areas. Like AREATYPE_TOOLBAR, AREATYPE_PANEL, etc. (I think I mentioned this in another bug recently, but I can't remember where.)

Am I doing the right thing here? I'm making the assumption that XUL-provided widgets are only allowed in toolbars, and not in other areas. Is that a safe assumption to make? If so, we'll need to implement the New Window, Print, and Full Screen buttons with the new createWidget API to get them into the menu panel.

Mike and Gijs, you too might be interested in my question in comment 2.

(In reply to Mike Conley (:mconley) from comment #2) > Created attachment 757456 [details] [diff] [review] > WIP Patch 1 > > Am I doing the right thing here? I'm making the assumption that XUL-provided > widgets are only allowed in toolbars, and not in other areas. > > Is that a safe assumption to make? If so, we'll need to implement the New > Window, Print, and Full Screen buttons with the new createWidget API to get > them into the menu panel. I'm already reimplementing the feed button to do this. However, this will mean the downloads button can't go into the panel, either. Strictly speaking, I see no reason eg. the home button can't be in the panel, to make space for other stuff. I personally never use it, and I imagine I'm not alone (danger: extrapolating from personal experience!). So, not entirely sure this is a great idea, sounds like we'd need to reimplement everything? Also, add-on buttons coming from XUL wouldn't be allowed to go into the panel, or, in a way, in the nav-bar's overflow panel, and we're still discussing using that as a way to migrate stuff out of the add-ons bar...

(In reply to :Gijs Kruitbosch from comment #4) > I'm already reimplementing the feed button to do this. However, this will > mean the downloads button can't go into the panel, either. Strictly > speaking, I see no reason eg. the home button can't be in the panel, to make > space for other stuff. I personally never use it, and I imagine I'm not > alone (danger: extrapolating from personal experience!). So, not entirely > sure this is a great idea, sounds like we'd need to reimplement everything? Yeah, it'd mean re-implementing anything that wants to go into the menu panel (but _not_ the overflow panel, which is not technically an "area" distinct from its toolbar). > Also, add-on buttons coming from XUL wouldn't be allowed to go into the > panel, or, in a way, in the nav-bar's overflow panel They should still be able to go into the overflow panel - I'm talking strictly about the menu panel here. Supposing XUL widgets *should* be allowed to go into the panel, how might we allow them to indicate this? Via an attribute? Or do we just blanket-assume that XUL widgets are allowed in toolbars and the menu panel?

(In reply to Mike Conley (:mconley) from comment #5) > Supposing XUL widgets *should* be allowed to go into the panel, how might we > allow them to indicate this? Via an attribute? Or do we just blanket-assume > that XUL widgets are allowed in toolbars and the menu panel? TBH, I've never really understood why we (said we) had the allowedArea stuff to begin with. I'm sure this is just me being clueless though, but I welcome cluesticks, so: which widgets do we really not want to be shown anywhere other than in <some area> ?

(In reply to :Gijs Kruitbosch from comment #6) > (In reply to Mike Conley (:mconley) from comment #5) > > Supposing XUL widgets *should* be allowed to go into the panel, how might we > > allow them to indicate this? Via an attribute? Or do we just blanket-assume > > that XUL widgets are allowed in toolbars and the menu panel? > > > TBH, I've never really understood why we (said we) had the allowedArea stuff > to begin with. I'm sure this is just me being clueless though, but I welcome > cluesticks, so: which widgets do we really not want to be shown anywhere > other than in <some area> ? I think the idea is that this helps to reduce the number of cases certain widgets need to consider in terms of their placements. The Gecko Profiler, for example, probably doesn't want to go into the menu panel, and the devs behind it probably don't want to waste effort making it open a subview or something when it's in the menu panel. So that's what kinda shaped my reasoning here - assume widgets will work properly in toolbars, and have them opt-in to the other areas iff they're created via the createWidget API. But those are only my reasons - this allowedAreas stuff was around before I started. Maybe there were other intentions as well? Blair?

Comment on attachment 757456 [details] [diff] [review] WIP Patch 1 Review of attachment 757456 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me, but I don't have an answer for the XUL widgets question.

(In reply to Mike Conley (:mconley) from comment #7) > I think the idea is that this helps to reduce the number of cases certain > widgets need to consider in terms of their placements. The Gecko Profiler, > for example, probably doesn't want to go into the menu panel, and the devs > behind it probably don't want to waste effort making it open a subview or > something when it's in the menu panel. > > So that's what kinda shaped my reasoning here - assume widgets will work > properly in toolbars, and have them opt-in to the other areas iff they're > created via the createWidget API. > > But those are only my reasons - this allowedAreas stuff was around before I > started. Maybe there were other intentions as well? Blair? Pretty much what you said. Though it was more clear-cut at the time, as we didn't have subviews like we do now, and removable=false wasn't supported. As for XUL widgets: I think its a grey area. There are no hard reasons to allow/disallow XUL widgets in the panel, but there are some fuzzy ones that could be argued either way: * Add-on compatibility - if we allow XUL widgets in the panel, how many add-ons will be broken when people move such widgets there? If we disallow it, how many add-ons are we artificially limiting, resulting in sub-par UX for people who want the widget in the panel? * API equality - disallowing XUL widgets in the panel puts every existing add-on at a disadvantage (which currently includes Jetpack add-ons). On the other hand, it will quicken the adoption of the new API. Personally, I'm in favour of allowing XUL widgets in the panel. But it depends on how many add-ons that ends up breaking, and I don't know where to put the line in the sand. We could support an API for XUL widgets to define areas where they want to be allowed, along the lines of the "customizableui-areatype" etc attributes (only this would be set by the add-on, not us).

Comment on attachment 757456 [details] [diff] [review] WIP Patch 1 Review of attachment 757456 [details] [diff] [review]: ----------------------------------------------------------------- While you're at it, want to remove CustomizableUI.TYPE_BUTTON? IIRC, that was added so it would eventually map to gSupportedWidgetTypes, but we're not using it yet. Should also rename TYPE_MENU_PANEL and TYPE_TOOLBAR to AREATYPE_MENU_PANEL and AREATYPE_TOOLBAR, since TYPE_* is ambiguous and will clash with TYPE_BUTTON if we ever get around to bringing that back and using that naming scheme for widget types. ::: browser/components/customizableui/src/CustomizableUI.jsm @@ +1699,5 @@ > */ > function WidgetGroupWrapper(aWidget) { > this.isGroup = true; > > + const kBareProps = ["id", "source", "type", "disabled", "name", "description", "allowedAreaTypes"]; Interesting side-effect of both of these wrappers: allowedAreaTypes can be modified. For this wrapper, it will update the underlying widget object and persist for the lifetime of the application. But for the XUL wrapper, the modifications will persist only until the wrapper is garbage-collected. At the very least, we need to fix it so the behaviour isn't dependant on GC. One solution is to create a wrapper for Set, that only allows access to accessor functions. Here's such a wrapper I wrote for something else: http://www.pastebin.mozilla.org/2482999 I had been wondering if it would be useful to have that available in a standard toolkit module, along with a read-only wrapper for Map and Array. (I assume it still works - there had been talk of possibly changing the JS spec for how the iterator could be accessed.)

(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #9) > (In reply to Mike Conley (:mconley) from comment #7) > > I think the idea is that this helps to reduce the number of cases certain > > widgets need to consider in terms of their placements. The Gecko Profiler, > > for example, probably doesn't want to go into the menu panel, and the devs > > behind it probably don't want to waste effort making it open a subview or > > something when it's in the menu panel. > > > > So that's what kinda shaped my reasoning here - assume widgets will work > > properly in toolbars, and have them opt-in to the other areas iff they're > > created via the createWidget API. > > > > But those are only my reasons - this allowedAreas stuff was around before I > > started. Maybe there were other intentions as well? Blair? > > Pretty much what you said. Though it was more clear-cut at the time, as we > didn't have subviews like we do now, and removable=false wasn't supported. > > As for XUL widgets: I think its a grey area. There are no hard reasons to > allow/disallow XUL widgets in the panel, but there are some fuzzy ones that > could be argued either way: > * Add-on compatibility - if we allow XUL widgets in the panel, how many > add-ons will be broken when people move such widgets there? If we disallow > it, how many add-ons are we artificially limiting, resulting in sub-par UX > for people who want the widget in the panel? > * API equality - disallowing XUL widgets in the panel puts every existing > add-on at a disadvantage (which currently includes Jetpack add-ons). On the > other hand, it will quicken the adoption of the new API. > > Personally, I'm in favour of allowing XUL widgets in the panel. But it > depends on how many add-ons that ends up breaking, and I don't know where to > put the line in the sand. > Talking to Stephen, the two items we *don't* want to go into the menu panel are the new tab button and the list all tabs buttons. > We could support an API for XUL widgets to define areas where they want to > be allowed, along the lines of the "customizableui-areatype" etc attributes > (only this would be set by the add-on, not us). Hm. This means that I have to grab the node from some window and examine its attributes in order to know what area types it's allowed in. I can think of a few ways of doing that: 1) Making allowedAreaTypes a function like getAllowedAreaTypes that takes a window as a parameter. Bleh. 2) Let allowedAreaTypes fish around for any existing instance of a node in any window. Alternatively, we can expose an API for XUL widgets to specify certain properties of themselves. Something like: CustomizableUI.defineXULWidget("new-tab-button", { allowedAreaTypes: [blah, blah] }); Other options? Anything I missed?

(In reply to Mike Conley (:mconley) from comment #11) > Other options? Anything I missed? We already do something similar for the "removable" attribute - see CustomizableUIInternal.isWidgetRemovable(). That doesn't need a window as a parameter - it just assumes the same widget in each window has the same value for the attribute, so looks in the first window we know about. Which is a fair assumption, as we don't want to support these types of attributes/properties having different values between different windows.' But a CustomizableUI.defineXULWidget() would work too. Not sure which I prefer :\ The first feels more XUL-ly, but the second fits better with the overall API direction. Not sure which is likely to be used more.

Adding Gijs as a reviewer for my tests. Probably the strangest thing I had to do in here was to account for the case where areas are being registered at initialization. During that phase, widgets are restored into their areas, and we use canWidgetMoveToArea to determine if they in fact can be there. At that point, however, we've not registered any build windows. My solution here was to just allow all areas if we seem to be in the initialization state - though, Blair, if you have an idea of how we could possibly reorganize initialization such that the build windows are available at that point, that could work too.

(In reply to Mike Conley (:mconley) from comment #13) > if you have an idea of how we could > possibly reorganize initialization such that the build windows are available > at that point, that could work too. Hmm, could it be as simple as moving the call to registerBuildArea() to the start of registerMenuPanel()/registerToolbar() ? Or at least, so its before the calls to buildArea() and restoreStateForArea(). Assuming that works, it would get us in a better state. However, we'd still need the code to handle the case where there are no windows - mostly due to OSX when there are no browser windows open, although it'd be possible to hit it in other ways too (even earlier in initialization, or when there's an add-on window open but no browser window).

(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #14) > However, we'd still > need the code to handle the case where there are no windows - mostly due to > OSX when there are no browser windows open, although it'd be possible to hit > it in other ways too (even earlier in initialization, or when there's an > add-on window open but no browser window). Come to think of it, we should check nothing else breaks in this case - filed bug 880022. Though, maybe we don't need the fallback code that gives defaults. If there are no browser windows, no instances of XUL widgets, then maybe we should just purposefully throw/do nothing.

Comment on attachment 758689 [details] [diff] [review] Patch v1 with tests Review of attachment 758689 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/src/CustomizableUI.jsm @@ +1302,5 @@ > instances: new Map(), > currentArea: null, > removable: false, > defaultArea: null, > + allowedAreaTypes: new ReadOnlySet(new Set([CustomizableUI.AREATYPE_TOOLBAR])), Hm, maybe the constructor for ReadOnlySet should also accept an array (automatically constructing its own Set, in that case). Though, I'd prefer if the underlying widget object didn't have read-only properties (make it read-only only in the wrappers). @@ +1610,5 @@ > return false; > } > + > + let widget = this.wrapWidget(aWidgetId); > + if (!widget.allowedAreaTypes.has(areaProps.get("type"))) { Hmm, should make registerArea() ensure that the 'type' property has a value (either by throwing, or allowing a default if omitted). ::: browser/components/customizableui/src/CustomizableWidgets.jsm @@ +1,1 @@ > +/* tHis Source Code Form is subject to the terms of the Mozilla Public Erm? @@ +38,5 @@ > viewId: "PanelUI-history", > removable: true, > defaultArea: CustomizableUI.AREA_PANEL, > + allowedAreaTypes: [CustomizableUI.AREATYPE_MENU_PANEL, > + CustomizableUI.AREATYPE_TOOLBAR], Hmm... we should probably allow a CustomizableUI.AREATYPE_ANY

Comment on attachment 758689 [details] [diff] [review] Patch v1 with tests Review of attachment 758689 [details] [diff] [review]: ----------------------------------------------------------------- Small nit, other than that I hope that Blair's suggestion is doable so that we can have this work properly for XUL widgets, too... ::: browser/components/customizableui/test/head.js @@ +148,5 @@ > + } > + return node; > +} > + > +function addToToolbox(...aNodes) { Can we call this addToPalette?

(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #14) > (In reply to Mike Conley (:mconley) from comment #13) > > if you have an idea of how we could > > possibly reorganize initialization such that the build windows are available > > at that point, that could work too. > > Hmm, could it be as simple as moving the call to registerBuildArea() to the > start of registerMenuPanel()/registerToolbar() ? Or at least, so its before > the calls to buildArea() and restoreStateForArea(). > Doesn't seem to be that simple - the problem is that registerArea calls restoreStateForArea for non-legacy areas, in this case the menu panel. restoreStateForArea then tries to add widgets to the area based on the saved (or default) placements. During this adding process, each widget is checked to ensure that the widget can in fact be added to the area. For XUL widgets, this involves checking the build windows for the nodes attributes. But, as we've said, the build windows haven't been registered yet because no areas have technically been built. Is it worth making the panel a lazy area as well? And then restore state for it only when it's first made visible? Let me know. Until then, I've still got my hack here, but using the AREATYPE_ANY value that you suggested. (In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #16) > Comment on attachment 758689 [details] [diff] [review] > Patch v1 with tests > > Review of attachment 758689 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/customizableui/src/CustomizableUI.jsm > @@ +1302,5 @@ > > instances: new Map(), > > currentArea: null, > > removable: false, > > defaultArea: null, > > + allowedAreaTypes: new ReadOnlySet(new Set([CustomizableUI.AREATYPE_TOOLBAR])), > > Hm, maybe the constructor for ReadOnlySet should also accept an array > (automatically constructing its own Set, in that case). > Good idea. Done. > Though, I'd prefer if the underlying widget object didn't have read-only > properties (make it read-only only in the wrappers). > Yep, done. > @@ +1610,5 @@ > > return false; > > } > > + > > + let widget = this.wrapWidget(aWidgetId); > > + if (!widget.allowedAreaTypes.has(areaProps.get("type"))) { > > Hmm, should make registerArea() ensure that the 'type' property has a value > (either by throwing, or allowing a default if omitted). > Ok, registerArea now defaults to AREATYPE_TOOLBAR if one has not been provided. > ::: browser/components/customizableui/src/CustomizableWidgets.jsm > @@ +1,1 @@ > > +/* tHis Source Code Form is subject to the terms of the Mozilla Public > > Erm? > Bah, editor fail. Fixed. > @@ +38,5 @@ > > viewId: "PanelUI-history", > > removable: true, > > defaultArea: CustomizableUI.AREA_PANEL, > > + allowedAreaTypes: [CustomizableUI.AREATYPE_MENU_PANEL, > > + CustomizableUI.AREATYPE_TOOLBAR], > > Hmm... we should probably allow a CustomizableUI.AREATYPE_ANY Done. (In reply to :Gijs Kruitbosch from comment #17) > Comment on attachment 758689 [details] [diff] [review] > Patch v1 with tests > > Review of attachment 758689 [details] [diff] [review]: > ----------------------------------------------------------------- > > Small nit, other than that I hope that Blair's suggestion is doable so that > we can have this work properly for XUL widgets, too... > > ::: browser/components/customizableui/test/head.js > @@ +148,5 @@ > > + } > > + return node; > > +} > > + > > +function addToToolbox(...aNodes) { > > Can we call this addToPalette? Done.

Gijs suggested we might try registering the most recent browser window on CustomizableUI initialization to guarantee the presence of a window within gBuildWindows. Seems to work. This means I can take out my workaround. Blair - sorry to toss more on your review plate. :/ But what do you think of this?

Comment on attachment 759849 [details] [diff] [review] Patch v1.3 Whoops - leftover dump.

Let's try that again.

Comment on attachment 759851 [details] [diff] [review] Patch v1.4 Review of attachment 759851 [details] [diff] [review]: ----------------------------------------------------------------- Yay! Glad this worked out. Overall this looks good, but I have a few suggestions: ::: browser/components/customizableui/src/CustomizableUI.jsm @@ +117,5 @@ > + // Before we register these areas, we need to ensure at least one > + // browser window is registered, otherwise, we won't be able to get > + // a read on XUL widget attributes to determine if items are allowed > + // in these areas, or if they're removable, etc. > + this.registerBuildWindow(RecentWindow.getMostRecentBrowserWindow()); OK, call me paranoid, but I'd say that if this call returns null (which I guess is what it does if there isn't a window?), we're going to error when initializing. We probably shouldn't do that; can you update registerBuildWindow to return early when passed null? Alternatively, we could decide that this should always happen and that we're OK with test failures if people break us this way. @@ +1940,5 @@ > this.type = "custom"; > this.provider = CustomizableUI.PROVIDER_XUL; > > + Object.defineProperty(this, "allowedAreaTypes", { > + get: function() { Given my previous comment, I think this should keep a fallback in case there are no gBuildWindows. :-(

Comment on attachment 759851 [details] [diff] [review] Patch v1.4 Thanks Gijs - I'll pop this off of Blair's review queue - though I still want to make sure he's all good with registering the first browser window on init...

Hey Blair - don't need a full-blown review here, just want to make sure what I'm doing with registering the first available browser window on init is acceptable. Any potential problems with this idea? -Mike

(In reply to Mike Conley (:mconley) from comment #25) > Hey Blair - don't need a full-blown review here, just want to make sure what > I'm doing with registering the first available browser window on init is > acceptable. Any potential problems with this idea? *sigh* Wish I could give a simple yes/no answer to this. On one side, I don't think doing this will immediately break anything. But... * It could become an issue if/when we want to move this to Toolkit. We're building in assumptions that may not hold outside of Firefox (eg, that a customizable window will always be the one to initialize the module). That said, Toolkit is a non-goal at the moment - just something we need to keep in mind. * It still doesn't solve bug 880022. Initialization in Firefox is fine if we assume browser.js is causing the module to initialize the module, but that APIs can still be called when there are no browser windows open. But I *think* that assumption means we don't need to worry about getMostRecentBrowserWindow() returning null in Firefox (Gijs' comment 23). I think its reasonable to leave those problems unsolved for now, in the name of getting this thing landed. Bug 880022 probably needs to be addressed before shipping, since we want add-ons to use these APIs - but it should be enough to just purposefully throw an exception, rather than solving the root issue (leaving that for later).

Ok, I've marked bug 880022 as blocking M7, and I've gone ahead and landed this on UX: https://hg.mozilla.org/projects/ux/rev/f2fec3b20abe

This seems to have turned the WinXP mochitest-bc tests perma-orange. :-( 11:27:23 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug422590.js | Test timed out Bug 606099 - Intermittent browser_bug422590.js | Test timed out 11:27:24 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug422590.js | Found a tab after previous test timed out: about:customizing Bug 606099 - Intermittent browser_bug422590.js | Test timed out 11:27:24 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug427559.js | content window is focused - Got [object ChromeWindow], expected [object XrayWrapper [object Window]] 11:27:24 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug432599.js | uncaught exception - TypeError: aTarget is undefined at chrome://mochikit/content/tests/SimpleTest/EventUtils.js:287 11:27:54 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug432599.js | Test timed out 11:27:54 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug432599.js | Found a tab after previous test timed out: data:text/plain,Content 11:27:55 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug462289.js | tab key to selected tab1 activates tab - Got [object XULElement], expected [object XULElement] (I actually suspect the stray tab is what causes all the following orange... we should fix the thing that makes 422590 go orange and that should take care of this...)

Ugh. I'll just go ahead and back this out. :/ https://hg.mozilla.org/projects/ux/rev/556df196ed9a

Removing the items from M7 that do not block us from landing on m-c.

I think P1 based on having addons in areas they're not wanting to be in is bad?

Unassigning myself, just in case anybody wants to take this over the line while I'm on vacation.

I'll take a stab at this.

Unbitrotted, carrying over review, investigating whether this can now reland or not after my build has finished.

I screwed up my unbitrotting. This is better. Try run: https://tbpl.mozilla.org/?tree=Try&rev=34d3e238ca8b AFAICT this should be landable, but I'll wait on the try run before actually doing it.

(In reply to :Gijs Kruitbosch from comment #35) > Created attachment 770755 [details] [diff] [review] > Patch v1.4, better unbitrotted > > I screwed up my unbitrotting. This is better. Try run: > https://tbpl.mozilla.org/?tree=Try&rev=34d3e238ca8b > > AFAICT this should be landable, but I'll wait on the try run before actually > doing it. Test failures all over Windows. I'll try to reproduce, but can't guarantee I'll still get to this tonight. Looks like this might still offer a slight improvement of our Windows numbers, though.

So at least right now, failures happen because something does actually call into CustomizableUI before anything else happens and we don't have a browser window yet - getMostRecentWindow returns null. Which doesn't really make sense to me at this point.

(In reply to :Gijs Kruitbosch from comment #37) > So at least right now, failures happen because something does actually call > into CustomizableUI before anything else happens and we don't have a browser > window yet - getMostRecentWindow returns null. Which doesn't really make > sense to me at this point. Try push with just delaying stuff until we do have build windows: https://tbpl.mozilla.org/?tree=Try&rev=e021dbece131

The try run for this was green. I'm not sure I like it much, though. Mike, what do you think?

Comment on attachment 771319 [details] [diff] [review] Patch v1.4.1 Review of attachment 771319 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/content/toolbar.xml @@ +35,5 @@ > ]]></constructor> > > <method name="_init"> > <body><![CDATA[ > let scope = {}; Looks like this empty scope can go. ::: browser/components/customizableui/test/browser_873066_allowedAreaTypes.js @@ +216,5 @@ > +]; > + > +function cleanup() { > + removeCustomToolbars(); > + resetCustomization(); This is an async call now - we should use the async cleanup function thing like the other tests.

So with this original approach, we make it so that add-ons have to opt-in to being able to be in the menu panel... I seem to recall a discussion with dolske during the work week where he mentioned that this might be weird, since we'd then have some items that *can* go in the panel, and some items that *can't*, and that this kind of makes the 20 part of the 80/20/2 thing a little more difficult to achieve. Do I have that right, dolske? If that's the case, then do we: 1) still need to enforce allowedAreaTypes? Is it true that there are some items that cannot go into the menu panel? 2) want to default widgets to be allowed in all areas, as opposed to just toolbars? Thereby, switching to an opt-out approach for the menu panel.

(In reply to Mike Conley (:mconley) - mostly gone until July 8 from comment #41) > So with this original approach, we make it so that add-ons have to opt-in to > being able to be in the menu panel... > > I seem to recall a discussion with dolske during the work week where he > mentioned that this might be weird, since we'd then have some items that > *can* go in the panel, and some items that *can't*, and that this kind of > makes the 20 part of the 80/20/2 thing a little more difficult to achieve. > > Do I have that right, dolske? > > If that's the case, then do we: > > 1) still need to enforce allowedAreaTypes? Is it true that there are some > items that cannot go into the menu panel? I've tried putting the bookmarks toolbar item in the panel. It, err, isn't a great experience, to put it mildly. I'd like to not support that, at least initially. I suspect the tabs element itself, if it's not already removable="false", goes in that category as well. Don't know if there are others, offhand. > 2) want to default widgets to be allowed in all areas, as opposed to just > toolbars? Thereby, switching to an opt-out approach for the menu panel. That sounds reasonable to me. Also, if we do implement this support, changing the default is a relatively painless operation that could improve add-on compat should we find problems either way (too many people complain "it won't go into the panel" OR too many people complain "I put adblockplus in the panel and that totally broke the world because it can't cope"). Either way, I'll hold off on landing until dolske clarifies.

(In reply to Mike Conley (:mconley) from comment #41) > I seem to recall a discussion with dolske during the work week where he > mentioned that this might be weird, since we'd then have some items that > *can* go in the panel, and some items that *can't*, and that this kind of > makes the 20 part of the 80/20/2 thing a little more difficult to achieve. > > Do I have that right, dolske? Yes. My baseline assumption is that unless there is some compelling reason why there's a class of widgets that are crazy to have in the panel, then we really don't want to support that (and if we did, it should be an opt-out). For the reason you noted -- it would be in general really weird for an addon to not allow the user to place its widget in the panel. (In reply to :Gijs Kruitbosch from comment #42) > I've tried putting the bookmarks toolbar item in the panel. It, err, isn't a > great experience, to put it mildly. Because of its popup? Yeah, it either shouldn't offer the popup (one-click starring, that's it), or the panel should anchor to the 'burger button.

(In reply to Justin Dolske [:Dolske] from comment #43) > (In reply to Mike Conley (:mconley) from comment #41) > > > I seem to recall a discussion with dolske during the work week where he > > mentioned that this might be weird, since we'd then have some items that > > *can* go in the panel, and some items that *can't*, and that this kind of > > makes the 20 part of the 80/20/2 thing a little more difficult to achieve. > > > > Do I have that right, dolske? > > Yes. My baseline assumption is that unless there is some compelling reason > why there's a class of widgets that are crazy to have in the panel, then we > really don't want to support that (and if we did, it should be an opt-out). > For the reason you noted -- it would be in general really weird for an addon > to not allow the user to place its widget in the panel. So does that make this wontfix? > (In reply to :Gijs Kruitbosch from comment #42) > > > I've tried putting the bookmarks toolbar item in the panel. It, err, isn't a > > great experience, to put it mildly. > > Because of its popup? Yeah, it either shouldn't offer the popup (one-click > starring, that's it), or the panel should anchor to the 'burger button. Sorry, I was unclear. I meant the "bookmarks toolbar" item, id est, the bookmarks as shown in the bookmarks toolbar. And no, not so much about popups, but about complete brokenness. I drag it to the panel, and it disappears completely (!?). I exit customization mode, and single bookmark text/labels are misaligned to the top of their panel row if there is no icon. Ones with icons (eg. folders) look sort of OK except the text is all bold. The items all keep the bookmark toolbar hover state, too, which on Mac OS X features rounded corners which seem to be defined relative to the element height, which means you get 24px-radius rounding on the button which looks... really weird? :-) Anyway, I'll file a new bug for that, then...

Hey Blair, It's starting to sound as if area restrictions is not a thing we necessarily want... do you have a position on all of this? -Mike

(In reply to Mike Conley (:mconley) from comment #45) > It's starting to sound as if area restrictions is not a thing we necessarily > want... do you have a position on all of this? I'd hope that it wouldn't be necessary for built-in widgets (except in the case where we run out of time to fix bugs), but I think it's important for add-ons. There are plenty of add-ons that add a dropdown panel, or something else that aren't compatible with our new panel UI - I'd rather have a slightly less awesome customization experience where you can't move such a widget, compared to being able to move such a widget but it breaks. I'd expect that getting those add-ons updated is going to be a rather long tail.

(In reply to Blair McBride [:Unfocused] from comment #46) > > I'd rather have a > slightly less awesome customization experience where you can't move such a > widget, compared to being able to move such a widget but it breaks. I'd > expect that getting those add-ons updated is going to be a rather long tail. (In reply to Justin Dolske [:Dolske] from comment #43) > Yes. My baseline assumption is that unless there is some compelling reason > why there's a class of widgets that are crazy to have in the panel, then we > really don't want to support that (and if we did, it should be an opt-out). > For the reason you noted -- it would be in general really weird for an addon > to not allow the user to place its widget in the panel. > I think we need to reconcile these two points. So, to summarize, it sounds like there are two options we're considering here: 1) Allow widgets to opt-out of being able to be included in the panel. This is good for widgets that do things that aren't really compatible with the panel - like open menu popups. This means that the user can be in the position where certain items will refuse to go into the menu panel. 2) All removable widgets are allowed in the panel. The user can move things wherever they'd like, but the onus is now on add-on authors to do something reasonable when their widget is in the menu panel. We're currently in the state of #2, and the question is whether to switch to the state of #1. #2 puts more demands on add-on authors, and #1 introduces a little bit of inconsistency while customizing for the user. So which state do we prefer?

An opt-out is probably ok, since that removes the concern of addons forgetting to include an opt-in flag (this making a customizable menu less useful). OTOH, even if add-ons opt-out of the menu panel, they'll still have to deal with possibly ending up in the overflow panel, which would have exactly the same issue. So it sorta seems like add-ons need to be able to deal with such cases anyway. Instead of providing mechanisms for limiting placement, how about mechanisms for making it easier for things to just-work no matter where they are?

(In reply to Justin Dolske [:Dolske] from comment #48) > An opt-out is probably ok, since that removes the concern of addons > forgetting to include an opt-in flag (this making a customizable menu less > useful). > > OTOH, even if add-ons opt-out of the menu panel, they'll still have to deal > with possibly ending up in the overflow panel, which would have exactly the > same issue. So it sorta seems like add-ons need to be able to deal with such > cases anyway. They can specify that their button isn't overflowable, so they don't need to, need to, but I agree that this is somewhat inevitable. Still not sure if I should update + land this now! Sounds like 'no'? :-) > Instead of providing mechanisms for limiting placement, how about mechanisms > for making it easier for things to just-work no matter where they are? I agree with this sentiment, but I'm not too sure about what we could do, unfortunately. I guess we could try to come up with a way that menubuttons automatically create subviews? But that's a fair amount of work, and I'm not sure if it'd be worth the effort - as we saw in bug 877450, submenus cause issues. Maybe there are other, more low-hanging-fruit-type things we could do? Mike or Mike or Blair, have you got ideas? :-)

(In reply to :Gijs Kruitbosch from comment #49) > (In reply to Justin Dolske [:Dolske] from comment #48) > > An opt-out is probably ok, since that removes the concern of addons > > forgetting to include an opt-in flag (this making a customizable menu less > > useful). > > > > OTOH, even if add-ons opt-out of the menu panel, they'll still have to deal > > with possibly ending up in the overflow panel, which would have exactly the > > same issue. So it sorta seems like add-ons need to be able to deal with such > > cases anyway. > > They can specify that their button isn't overflowable, so they don't need > to, need to, but I agree that this is somewhat inevitable. > > Still not sure if I should update + land this now! Sounds like 'no'? :-) > That's a no, I believe. > > Instead of providing mechanisms for limiting placement, how about mechanisms > > for making it easier for things to just-work no matter where they are? > > I agree with this sentiment, but I'm not too sure about what we could do, > unfortunately. I also agree that this would be ideal, but there are some cases we don't have solutions for. Probably the one that comes to mind first is buttons that open multi-level popups. We don't really have a nice way for those to work in the menu panel right now, except to ask the add-on developer to design it differently (or just to act differently when in the panel). We've got mechanisms in place to help out widgets that open panels - though the behaviour for what those widgets should do in the menu panel is unclear. There was an idea at one point of having them anchor to the menu button, but I think it got the People's Eyebrow from UX. We could attempt to jam the panel in as a subview, but wide-panels would be pretty unwieldy. Also, moving items around in the DOM on an add-on might, again, break that add-on in unpredictable ways. So while it'd be ideal to be fowards-compatible here, I honestly don't think we can do it for the cases I've mentioned with the panel behaviour spec that we've got.

(In reply to Mike Conley (:mconley) from comment #50) > > > Instead of providing mechanisms for limiting placement, how about mechanisms > > > for making it easier for things to just-work no matter where they are? > > > > I agree with this sentiment, but I'm not too sure about what we could do, > > unfortunately. > > I also agree that this would be ideal, but there are some cases we don't > have solutions for. Probably the one that comes to mind first is buttons > that open multi-level popups. Why is it a problem to just let items open popups no matter where they're placed? I guess it's not the kind of UI envisioned by UX, but that's true for lots of things add-ons can do. (One could argue that with context menus for items in the panel, we've already broken the no-popups-in-the-menu-panel rule ourselves.)

How do you feel about Dao's implied suggestion, Stephen? For add-on widgets in the panel, we could show the first popup for a widget in a subview, and then child popups could open off of it, like old-style menus. I know that's not ideal, and the Bookmark widget design, for instance, explicitly chose to avoid this approach. But this might ease the burden on add-on developers somewhat, since we could (theoretically) just make old-style menu popups forward compatible in the panel. Thoughts?

I'm going to produce a demo build for fang and shorlander to try that uses menuitems spawning menupopups from within a subview.

Hey fang - I only caught part of your feedback about the build I sent you before I had to log off. Would you mind summarizing your thoughts here? Thanks, -Mike

As discussed in our weekly meeting, we're going to WONTFIX this until we find out if this is a thing we actually really need. We have no evidence yet that this is a major problem (let alone a P1), so we're going to WONTFIX this until we have enough evidence to necessitate re-opening.