669523 - Allow panels to explicitly specify their position

Status: RESOLVED INVALID

(Add-on SDK Graveyard :: General, enhancement, P3)

Description

[Anant Narayanan [:anant]]

Attachment: Patch to allow panels to specify their position

Panels sometimes place themselves in arbitrary positions depending on screen space. Developers should be able to specify where the panel draws with respect to an anchor (if any are provided).

This patch adds support to pass in a 'position' string to the panel.show() function as a second argument. Possible values are: "(top|bottom)(left|center|right) (bottom|top)(left|center|right)".

A panel may also specify its default position as an option in its constructor.

The patch also fixes a minor bug where a wrong comparison between screenY and panel.width is performed (it should be screenX).

Comment 1

[Anant Narayanan [:anant]]

Comment on attachment 544103 [details] [diff] [review]
Patch to allow panels to specify their position

>diff --git a/packages/addon-kit/lib/panel.js b/packages/addon-kit/lib/panel.js
>index 0d6d223..ea618aa 100644
>--- a/packages/addon-kit/lib/panel.js
>+++ b/packages/addon-kit/lib/panel.js
>@@ -113,7 +113,9 @@ const Panel = Symbiont.resolve({
>       this.height = options.height;
>     if ('contentURL' in options)
>       this.contentURL = options.contentURL;
>-
>+    if ('position' in options)
>+      this.preferredPosition = options.position;
>+    
>     this._init(options);
>   },
>   _destructor: function _destructor() {
>@@ -142,7 +144,7 @@ const Panel = Symbiont.resolve({
>   get isShowing() !!this._xulPanel && this._xulPanel.state == "open",
> 
>   /* Public API: Panel.show */
>-  show: function show(anchor) {
>+  show: function show(anchor, position) {
>     anchor = anchor || null;
>     let document = getWindow(anchor).document;
>     let xulPanel = this._xulPanel;
>@@ -180,15 +182,16 @@ const Panel = Symbiont.resolve({
>       xulPanel.appendChild(frame);
>       document.getElementById("mainPopupSet").appendChild(xulPanel);
>     }
>-    let { width, height } = this, x, y, position;
>+
>+    let x, y;
>+    let { width, height } = this;
>     
>     if (!anchor) {
>       // Open the popup in the middle of the window.
>       x = document.documentElement.clientWidth / 2 - width / 2;
>       y = document.documentElement.clientHeight / 2 - height / 2;
>       position = null;
>-    } 
>-    else {
>+    } else if (!position && !this.preferredPosition) {
>       // Open the popup by the anchor.
>       let rect = anchor.getBoundingClientRect();
>       
>@@ -206,11 +209,11 @@ const Panel = Symbiont.resolve({
>       else
>         vertical = "bottom";
>       
>-      if (screenY > window.screen.availWidth / 2 + width)
>+      if (screenX > window.screen.availWidth / 2 + width)
>         horizontal = "left";
>       else
>         horizontal = "right";
>-      
>+
>       let verticalInverse = vertical == "top" ? "bottom" : "top";
>       position = vertical + "center " + verticalInverse + horizontal;
>       
>@@ -218,8 +221,10 @@ const Panel = Symbiont.resolve({
>       // specified position (useful if we compute a bad position or if the 
>       // user moves the window and panel remains visible)
>       xulPanel.setAttribute("flip","both");
>+    } else if (!position) {
>+      position = this.preferredPosition;
>     }
>-    
>+
>     // Resize the iframe instead of using panel.sizeTo
>     // because sizeTo doesn't work with arrow panels
>     xulPanel.firstChild.style.width = width + "px";

Comment 2

[Myk Melez [:myk] [@mykmelez]]

16:42:08 - myk: anant: sorry about the delay; i'm looking at your patch for positioning panels now; one question: which positions did you want to specify manually that prompted the patch?
16:43:39 - anant: myk: no worries, tyt! we wanted to position the openwebapps dashboard panel to be always "topcenter, bottomright"
16:44:02 - anant: sometimes the panel would position itself below the top-level window, which looked weird
16:44:52 - anant: mostly we wanted consistent placement, the current behavior makes the panel position change based on where the window was located w.r.t. the screen
16:46:17 - myk: anant: ah, so the dashboard panel is activated via a widget at the bottom right corner of the screen, and you want the panel to always appear above it and offset to the left?
16:47:15 - anant: myk: exactly!
16:47:27 - anant: such that the panel is always living inside the browser window
16:48:41 - myk: anant: hmm, i wonder if we should just make that the default behavior
16:49:20 - myk: anant: i.e. make the algorithm that determines panel placement take into account the edges of the browser window and display the panel within them if at all possible
16:49:44 - myk: anant: (the second thing i'm wondering is whether there's a simpler syntax than the XUL one for specifying panel placement)
16:50:38 - anant: myk: yes, we could invent our own syntax and map it to the XUL ones (an earlier version of the patch actually did that). but then we thought add-on developers would already know what the XUL ones meant so we reverted
16:50:46 - anant: I suppose that isn't true for new add-on developers :)
16:51:17 - myk: anant: yeah; of course, we aim to serve both new and existing developers; at the same time, we'd like to provide higher-level abstractions where possible
16:51:21 - anant: and re: default behavior, yes I support that we should always display the panel inside the browser window (in which cases would this not be possible?)
16:51:32 - myk: anant: and the XUL syntax in this case is particularly complex
16:52:23 - myk: anant: it also supports some unuseful cases (like "topleft topleft")
16:53:20 - myk: anant: there are some edge cases where a browser window is stuffed in a corner such that a panel that opens into the browser would be mostly obscured offscreen
16:54:02 - myk: anant: f.e. imagine that you stuff your browser window into the bottom left corner of your screen such that virtually the only thing visible is a widget in the top right corner of the browser window that notifies you when something interesting happens
16:54:13 - myk: anant: and when it happens, you want to click the widget and see the panel associated with it
16:54:33 - myk: anant: if it were to open into the browser window, it would be mostly obscured, whereas if it opened outside the window, it would be visible
16:55:23 - myk: anant: but that's a real edge case; we could start by always preferring to open towards the browser and fix that edge case in a second phase
16:56:52 - myk: anant: one thought i had about manual positioning options is to allow developers to specify a compass direction (N, S, E, W, NE, NW, SE, SW) that we map to a placement; that gives developers fewer options for positioning, and we would have to make some decisions about exactly where these placements appear...
16:57:13 - myk: anant: but it might be sufficient for the common cases that aren't satisfied by our automatic placement algorithm


Matteo, Alex: what do y'all think?

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Note: the syntax for XUL panel positioning is different for arrow panels, and I can't actually find documentation about them anywhere, but it appears to be in the format "(point on edge of anchor) (point on edge of panel)" where each part is (top|bottom)(left|center|right).

So "topcenter bottomright", which Anant wants, opens the panel such that a point near the bottom right corner of the panel is next to a point near the top center of the anchor, i.e. the panel opens above and to the left of the anchor, the equivalent to the compass direction northwest (NW), if compass directions specify the position of the panel relative to the anchor.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 544103 [details] [diff] [review]
Patch to allow panels to specify their position

(Cancelling this review request while we figure out the right thing to do here.)

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

In my opinion we should improve the algorithm logic rather than specify the panel's position. As Myk said, we could display the panel in the browser window as soon as there is enough space, otherwise we fallback to the current logic.

I'd like to avoid to give the capability to the addon developers to "force" the panel's position, especially for a panel associated to a widget. We put the widget in the addon bar for a reason, to have a standard behavior that we can internally handle across platform and future changes; and to provide a good, uniform, user experience.

For instance, if an Addon Developer force the panel to be displayed as "topcenter bottomright", what happens when we will move the addon bar from the bottom to the top, near the address bar? And how we should map these several positions in other devices, like mobile or tablets?

If we can cover the issue Anant has, just improving the logic of the algorithm, I think is the better, and more changes-proof and platform independent, approach.

Comment 6

[Dan Walkowski]

This is my original complaint, we wrote the patch together, and Anant was kind enough to file the bug. :)  (thanks Anant!)

There is no 'forcing' in our patch.  Only 'requesting'.  We request that that panel be displayed in position X.  If it does not fit, the original logic runs and puts it elsewhere.

This allows developers to have their panel outside the window if they wish, (for status info that shouldn't obscure content), or inside the window if, as in our case, it is a semi-modal picker that appears over content to indicate to users that it is frontmost and should be dealt with first.

I am not wedded to the particular details of the patch, but do wish to give developers some control over placement.  No hard-coded placement algorithm is going to please everyone.  (Ex: I think the current code is bonkers  :)

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

From what I can see from the patch, the position is 'forced' – the current logic is not executed if the developer specified a position for the panel.

However, even if the developer 'requesting' a position, is doing that in according with the current widget's addon bar position, and for a specific platform, but: the widget's addon bar position will change – and could change again in the future – so we can end up with the same problem we have at the moment, panel opened in wrong places or with unexpected behavior. Having the panel positioned to "topcenter bottomright" makes sense only if the widget's bar is on the bottom, and only in Firefox desktop.
We should also map this behavior on different platform – and of course with different result, and it adds an unnecessary complexity in my opinion, removing the benefit to have an uniform an good user experience, across addons and platforms.

I agree that no hard-coded placement algorithm is going to please everyone, in the same way as adding a widget in a predetermined location like the widget's addon bar, but we did it for reasons.

I totally agree that the current code has to be improved a lot, despite what we're going to do here, I took a quick look yesterday, but that can be done without affect the reasons above.

That's my understanding, Myk can probably confirm or not what I'm saying. Of course if the premises are not correct, my thoughts will be different: I'm a relative newer in jetpack. :)

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Marking anything that potentially looks like a feature request as "enhancement", sorry for the bugspam. :)

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Re-prioritizing all 1.1-targeted feature requests to 1.2.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

After chatting in IRC with Alex and jono, it could be worthy enable custom position for *non* anchored panel.
The scenario of jono it's more likely to be fixed by:

https://bugzilla.mozilla.org/show_bug.cgi?id=638142

But could be a good enhancement in any case.

Comment 13

[WM]

So I think we can close this bug now?

Comment 14

[Matteo Ferretti [:zer0] [:matteo]]

I think we can close it. The fix for panel position also fixes the original bug (where the panel was displayed in an unexpected way), fixing and improving the algorithm in accordance to UX about how an anchored panel should be displayed.

This is so far what we can do on high level API – from UX perspective, an anchored panel has strictly rules about how it should be displayed, and that have to be respected also to be consistent with Firefox UI.

On other hand, a non anchored panel now can be displayed anywhere thanks to the panel position stuff.

I will close as "INVALID", if anyone thinks about a better resolved reason, feel free to change it; or reopen it.