636448 - "arrow panel" appearance for panels

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Alexandre Poirot [:ochameau]]

Improve panel API to provide a way to use arrow panels:
  https://developer.mozilla.org/en/XUL/Attribute/panel.type

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

I'm asking for a feedback before writing the documentation, so something like a quick review on choices I took.

So here is the new API:
let panel = Panel({ type: 'arrow', position: 'bottomcenter topleft', ... });
panel.type = 'arrow';
panel.position = 'before_end';
panel.show(anchor);

You may give type and position in option dict or change them afterward via attributes on Panel objects. But these parameters won't change anything immeadiatly, you'll have to call show to have a panel with these new settings.

Here is for the public API, now let's see what new hacks needed to use arrow panel!

-> On windows arrow panels XBL has an anonymous node that has a big padding and this is really bad with our iframes because we expect it to have the full panel size. It's even worse when there is scrollbars on this iframe!
So I use another magic hack to remove this padding properly.
I use an inlined XBL containing an inlined CSS that remove this padding on anonymous content. Then I have to wait for attachment of the XBL (waitForBinding function), because the XBL I added is applied asynchronously (because it contains a stylesheet, I think that all xbl with stylesheet are doing the same).
 
-> First alternative would be to add this, in onShow method:
let box = this._xulPanel.ownerDocument.getAnonymousElementByAttribute(this._xulPanel, "class", "panel-inner-arrowcontent");
box.style.padding = "0";
But there is a clunky effect when the panel is displayed because we change the padding when the panel is already visible.
(we can't do this on panel contruction because anonymous content is not ready)

Another alternative would be to add a stylesheet in browser document. That works but we would have to be carefull to not apply this style on others browser panel, nor having conflicts between multiple panel module instances.

Comment 2

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

Great start! It made me realize that I didn't describe the issue very well, however, as I actually don't want to make arrow panels be an option that addon authors can specify; rather, I would like all panels to be arrow panels, and I want our implementation to choose the positioning of panels automatically based on the position of their anchor on the screen.

In other words, I want to abstract away the details of panel appearance and positioning, so the only thing addon authors have to worry about is the content of their panel, and we take care of putting it in the right place and making it look good!

The lower-level XUL API is useful for developers who want a lot of control over the appearance and behavior of their addon (and are willing to put in the extra work), but the SDK API is intended to be a higher-level API for folks who just want a simple, attractive panel that they don't have to configure much.

The hard part of the task will be choosing a positioning. But it should be possible to measure the distance of the anchor relative to the four sides of the screen and choose a positioning that automatically puts the panel more towards the center of the screen, i.e. if the anchor is towards the bottom right corner of the screen, put the panel above and to the left of it; if the anchor is near the top left corner, on the other hand, put the panel below and to the right of it; etc..

Comment 3

[Alexandre Poirot [:ochameau]]

Enabling arrow by default make sense!
Handling position automaticaly is really cool too, because position options is quite hard and there is so many (undocumented) values to define positioning that it's really hard to use. But I think that at some point, users may still want to have some control on it. May be not on relative position, "on top, left, right, bottom" but more on alignement: left, center or right.

First I'll try to propose a patch that do all automatically, then I would like to propose a simple option to control panel alignement relative to the anchor.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Patch v2 - arrow enabled by default, automatic positioning, no more extra attributes

Myk: here is something that should fullfill your previous comment. No more extra attributes on Panel, very simple automatic computation of position and all Panels are arrow panels.

Here panels can only be on top or on bottom of the anchor. And position is computed with screenX/screenY of anchor node. One alternative would be to compute position with windowX/windowY (ie anchor position relative to the top window) in order to keep panel displayed inside browser window. But its more complicated to do and doesn't work well with small windows.

Finally, I would like to add one or more properties to allow :
- left/right positions (start_*/end_*)
- to setup panel alignment, left or right (*_start/*_end).

Comment 5

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

Comment on attachment 515649 [details] [diff] [review]
Patch v2 - arrow enabled by default, automatic positioning, no more extra attributes

> Myk: here is something that should fullfill your previous comment. No more
> extra attributes on Panel, very simple automatic computation of position and
> all Panels are arrow panels.

This looks great!


> Here panels can only be on top or on bottom of the anchor. And position is
> computed with screenX/screenY of anchor node. One alternative would be to
> compute position with windowX/windowY (ie anchor position relative to the top
> window) in order to keep panel displayed inside browser window. But its more
> complicated to do and doesn't work well with small windows.

screenX/screenY seems fine.  There's no need for panels to remain inside the browser window, as long as their position relative to the anchor is sensible.


> Finally, I would like to add one or more properties to allow :
> - left/right positions (start_*/end_*)
> - to setup panel alignment, left or right (*_start/*_end).

Could you explain the use cases for these?  It isn't clear to me why they are needed.

One final note: the patch removes padding around the iframe on Windows, but there still seems to be a bunch of padding around the iframe on Mac and Linux.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: v3 - fix padding on linux, ready to review

I fixed the padding on linux too, but I can't verify that it is fixed on mac.
Then I found and kill a bug in symbiont. An undefined window in onStart function. When there is one XML document loading, it's window is null and symbiont receive an event for any document, like the XBL we use in panel!

I think that this patch is ready to review as there is no option added, we do not need any new documentation.

And I let additional positioning options for a future release. I think that the most important one is choosing between vertical (top or bottom) or horizontal (left or right). For now, they are all vertical. For example we can't display a thin sidebar on left of the browser with icons that will display panels on their right.

Comment 7

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

Comment on attachment 519232 [details] [diff] [review]
v3 - fix padding on linux, ready to review

diff --git a/packages/addon-kit/lib/panel.js b/packages/addon-kit/lib/panel.js

+      // One anonymous node has a big padding that doesn't work well jetpack 
+      // as we would like to display an iframe with full panel size
+      // -> Use a XBL wrapper with inner stylesheet to remove this padding.

  // One anonymous node has a big padding that doesn't work well with Jetpack,
  // as we would like to display an iframe that completely fills the panel.
  // -> Use a XBL wrapper with inner stylesheet to remove this padding.


+      // Setup the vertical position of the popup relative to the anchor
+      // (always display the arrow on anchor center)

Nit: Setup -> Set up


+      position = vertical + "center " +  verticalInverse + horizontal;

Nit: there's an extra space after second plus sign.


+      // Allow panel to flip itself if the panel can't be displayed at the
+      // specified position (usefull if we compute a bad position or if the 
+      // user move the window and panel is kept visible)

Nits:
  usefull -> useful
  move -> moves
  and panel is kept -> and the panel remains


+    // Resize the iframe instead of using panel.sizeTo
+    // because sizeTo doesn't work arrowpanels

Nit: work arrowpanels -> work with arrow panels


+    // Wait for the XBL binding to be contructed

Nit: contructed -> constructed



diff --git a/packages/addon-kit/tests/test-panel.js b/packages/addon-kit/tests/test-panel.js

+      contentURL: "data:text/html,<html><body style='padding:0; margin:0; background: gray; text-align: center;'>Anchor: "+anchor.id+"</body></html>",
...
+        if (count==5) {

Nit: add space around + and == operators and after colons, and wrap lines to 80 characters, breaking up strings as needed, i.e.:

      contentURL: "data:text/html,<html><body style='padding: 0; margin: 0; " +
                  "background: gray; text-align: center;'>Anchor: " +
                  anchor.id + "</body></html>",
...
        if (count == 5) {

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Final patch that landed

landed:
https://github.com/mozilla/addon-sdk/commit/31a80933c768c6bd4a434b276c5ca0965946c24f