991576 - Allow override of widget panel placement

Status: RESOLVED WONTFIX

(Firefox :: Toolbars and Customization, defect)

Description

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

Follow-up to bug 991356. By itself, that bug is barely problematic at all, but consider an add-on like mine, that adds a toolbar to the bottom of the browser, essentially restoring the add-on bar.

Like I mention in 991356, most times the panel will open in the right position, but because the bar's placement is on the bottom, bug 991356 will be much more evidenced (I have a user that says this will happen after every browser restart, although personally I can only reproduce following similar steps to what I posted in bug 991356).

However, on the add-on's side there's nothing we can do, it's the widgets (CustomizableUI/panelUI) that command the panel and we can't access it or change it or override it.

My proposal: default PanelUI.showSubView() (http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.js#296) to open the popup in "bottomcenter topright", but check the area where the trigger node is for some kind of "PanelUIPosition" attribute and allow that to override the default. This might not fix bug 991356, but it would make a huge difference in the meantime, as I guess that won't be very easy to fix in a short time.

Comment 1

[:Gijs (he/him)]

Hrm. I think we should actually prio this, even if the prio is very low. It'll need something similar to bug 977150, but then again we create the panel dynamically, so that wouldn't quite work. The simplest would be to add a property to the registerArea call's options that we check (and default to the current default). Luís, do you have time to look at this? I think we're not going to get to it ourselves before we ship. :-(

Comment 2

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

I actually have very little time lately, far less than I'd like, and not even close enough to get the build tools installed (and working). So this is the best I can do right now, even though I realize it's not exactly what you expected. I hope it helps none-the-less.

I wouldn't go with adding a property to registerArea's options. If for some reason the area node needs to move (which is my case actually) and the panel position should be changed with it, then we'd have to unregisterArea, move the node, then registerArea again just to update this position value. That's a lot of browser work, which I think is greatly unnecessary and easily avoidable. Instead, I'd go with adding an attribute to the actual area node "panelposition" (similar to bug 977150, except in the area node, rather than in the panel node). This also has the added advantage (or maybe it's a disadvantage?) of not having to track this information throughout CUI either, so it is as easily changeable as doing a single setAttribute() call.

Since this would be repeated in a lot of places, this could all be done by a simple method in CustomizableUIInternal, maybe before CUI.handleWidgetCommand() in http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/src/CustomizableUI.jsm#1208:
> getPanelPositionForWidget: function(aAnchor, aArea) {
>   // try to find if the area provided has a panelposition attribute
>   // that should override the default panel position
>   if (aAnchor) {
>     let placement = aArea ? { area: aArea } : null;
>     // if no area was provided, see if aAnchor is a known widget and
>     // find its placement
>     if (!placement && aAnchor.id)
>       placement = CustomizableUI.getPlacementOfWidget(aAnchor.id);
>     if (placement) {
>       let document = aAnchor.ownerDocument;
>       let areaNode = document.getElementById(placement.area);
>       if (areaNode && areaNode.getAttribute('panelposition'))
>         return areaNode.getAttribute('panelposition');
>     }
>   }
>   
>   // default position for the panel
>   return "bottomcenter topright";
> },
with the obvious CustomizableUI redirect so it can be accessed from the outside of course, maybe before CUI.hidePanelForNode() in http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/src/CustomizableUI.jsm#3184:
> /**
>  * Given a widget node, find the correct position to open a panel
>  * that should be anchored to this widget.
>  *
>  * @param aAnchor a (widget?) node that is, or contains, the panel's anchor.
>  * @param aArea optional, the id of the area where aAnchor is placed.
>  * @return a string value to be used for the panel's position.
>  */
> getPanelPositionForWidget: function(aAnchor, aArea) {
>   return CustomizableUIInternal.getPanelPositionForWidget(aAnchor, aArea);
> },

In http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.js#357, the change is simple, from:
> tempPanel.openPopup(iconAnchor || aAnchor, "bottomcenter topright");
to something like:
> let position = CustomizableUI.getPanelPositionForWidget(aAnchor, aPlacementArea);
> tempPanel.openPopup(iconAnchor || aAnchor, position);

This won't interfere with panelview's at all, and will still default any type="view" widgets to open the panel in the current defined position if the area node doesn't have the attribute to override it.

Social widgets also need a little something in http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-social.js#1329:
> let anchor = document.getAnonymousElementByAttribute(aToolbarButton, "class", "toolbarbutton-badge-container");
> // Bug 849216 - open the popup in a setTimeout so we avoid the auto-rollup
> // handling from preventing it being opened in some cases.
> setTimeout(function() {
>   panel.openPopup(anchor, "bottomcenter topright", 0, 0, false, false);
> }, 0);
something similar as above:
> let anchor = document.getAnonymousElementByAttribute(aToolbarButton, "class", "toolbarbutton-badge-container");
> let position = CustomizableUI.getPanelPositionForWidget(aToolbarButton);
> // Bug 849216 - open the popup in a setTimeout so we avoid the auto-rollup
> // handling from preventing it being opened in some cases.
> setTimeout(function() {
>   panel.openPopup(anchor, position, 0, 0, false, false);
> }, 0);

And for social marks widgets in http://mxr.mozilla.org/mozilla-central/source/browser/base/content/socialmarks.xml#218:
> panel.openPopup(anchor, "bottomcenter topright", 0, 0, false, false);
would become
> let position = CustomizableUI.getPanelPositionForWidget(this);
> panel.openPopup(anchor, position, 0, 0, false, false);

The bookmarks menu could also use this attribute, but only when it is called from the button; when anchored to the identity box or the browser the current default should always be used. In http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#313:
> StarUI.showEditBookmarkPopup(itemId, BookmarkingUI.anchor,
>                              "bottomcenter topright");
would become
> // if the anchor exists, the button has to as well
> let position = CustomizableUI.getPanelPositionForWidget(BookmarkingUI.button);
> StarUI.showEditBookmarkPopup(itemId, BookmarkingUI.anchor, position);

And finally the downloads panel in http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/downloads.js#577:
> this.panel.openPopup(aAnchor, "bottomcenter topright", 0, 0, false,
>                      null);
changes into
> let position = CustomizableUI.getPanelPositionForWidget(DownloadsButton._placeholder);
> this.panel.openPopup(aAnchor, position, 0, 0, false, null);

I think those are all the panels that would (or should) be affected by a panelposition override.

Comment 3

[:Gijs (he/him)]

Won't do this for webextensions because it's unclear how it would work or why it would make sense. Fixing bug 991356 or bug 974389 would make more sense.