977150 - Position of PanelUI-popup is hard coded

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[M8R-p7]

The anchor position of PanelUI-popup is hard coded in PanelUI.show() as 'bottomcenter topright'.

An add-on to move the PanelUI button is very simple, but changing the position of the Panelmenu requires redefining the whole PanelUI.show function.

The position should be defined by a 'position' attribute, as with other panels and popups.

Comment 1

[:Gijs (he/him)]

Attachment: don't hardcode position of Australis panels in JS; unify overflow show code,

This should work fine, and unifies some code, and fixes a subtle bug in the overflow panel.

Comment 2

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

Comment on attachment 8383337 [details] [diff] [review]
don't hardcode position of Australis panels in JS; unify overflow show code,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +3491,5 @@
>      }
>      let doc = this._panel.ownerDocument;
>      this._panel.hidden = false;
> +    let contextMenu = doc.getElementById(this._panel.getAttribute("context"));
> +    gELS.addSystemEventListener(contextMenu, 'command', this, true);

I take it the subtle bug you are hinting at is that if the panel was opened via some path other than clicking on the chevron, then the event listener would never be added. In the future it may be nice to say what the subtle bug that you're fixing is ;)

Comment 3

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Comment on attachment 8383337 [details] [diff] [review]
> don't hardcode position of Australis panels in JS; unify overflow show code,
> 
> Review of attachment 8383337 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +3491,5 @@
> >      }
> >      let doc = this._panel.ownerDocument;
> >      this._panel.hidden = false;
> > +    let contextMenu = doc.getElementById(this._panel.getAttribute("context"));
> > +    gELS.addSystemEventListener(contextMenu, 'command', this, true);
> 
> I take it the subtle bug you are hinting at is that if the panel was opened
> via some path other than clicking on the chevron, then the event listener
> would never be added. In the future it may be nice to say what the subtle
> bug that you're fixing is ;)

Sorry, yes, that was what I meant. :-)

Comment 4

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/29aef9e5c32f

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/29aef9e5c32f

Comment 6

[:Gijs (he/him)]

Comment on attachment 8383337 [details] [diff] [review]
don't hardcode position of Australis panels in JS; unify overflow show code,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: add-ons can't easily control the appearance of the menu panel and/or overflow panel; small behavioural difference in closing behaviour of the overflow panel when it is programmatically opened
Testing completed (on m-c, etc.): on m-c 
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: low

Comment 7

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cb995c6b9ca0