Closed Bug 732294 Opened 8 years ago Closed 7 years ago

Panel constructor lacks a way to set position of panel other than "centered"

Categories

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

defect

Tracking

(firefox23-)

RESOLVED FIXED
Tracking Status
firefox23 - ---

People

(Reporter: jono, Assigned: zer0)

References

Details

Attachments

(2 files)

Going off of the documentation at https://addons.mozilla.org/en-US/developers/docs/sdk/1.3/packages/addon-kit/docs/panel.html

I can pass in width and height attributes as options to the panel constructor...

var panel = require("panel").Panel({
  width: 500,
  height: 400
});

but there are no options to set the position of the panel within the browser window. It always appears centered (if opened with panel.show()) or anchored to the widget (if opened from an addon-bar widget).

I'm working on a requested feature for the Collusion add-on, to provide a view of the Collusion tracking graph in a panel overlaying a corner of the browser window. Unfortunately it seems my only choices at the moment are to have it centered (obscuring the user's prime web content) or anchor it to the addon-bar widget.

I propose an API where you could specify halign and valign properties in the panel constructor. halign could be "left", "right", or "center", and valign could be "top", "bottom", or "center". The Add-on SDK would use these attributes and the width and height to position the panel in a location appropriate for the screen size on the user's device. (If the screen is too small, e.g. on mobile, they might just be ignored.)

So I would be able to do:

var panel = require("panel").Panel({
  width: 300,
  height: 200,
  halign: "right",
  valign: "top"
});

Please consider it. Thanks!

(Note: this is similar to bug https://bugzilla.mozilla.org/show_bug.cgi?id=669523 but that one deals with specifying position of panel relative to an anchor; this is for when panel is opened without an anchor.)
Assignee: nobody → zer0
Priority: P2 → P1
Here the JEP related to Panel Position APIs, that should cover this scenario too: https://github.com/mozilla/addon-sdk/wiki/JEP-Panel-positioning
I made some comments!
Attachment #719792 - Flags: review?(rFobic)
Irakli, I created the pull request in order to have some concrete code where to discuss, after the comments had in the etherpad, to check if the APIs are fine.
Attachment #719792 - Flags: review?(rFobic) → review-
Attachment #719792 - Flags: review?(rFobic)
Comment on attachment 719792 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/826

Please see my overview comment in the pull before landing!

Wohoo panel positioning \o/
Attachment #719792 - Flags: review?(rFobic) → review+
Comment on attachment 735523 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/826

This is the implementation after the "detraitification", therefore it needs a completely new review I'm afraid.

I will do some unit test ASAP, but in the meantime I'd like if you could take a look to the current implementation.
Attachment #735523 - Flags: review?(rFobic)
Attachment #735523 - Flags: review?(rFobic) → review+
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/dbd07b38531d48745d295370b9ca2a182da6ab26
Bug 732294 - Panel constructor lacks a way to set position of panel other than "centered"

Implemented the API described in [JEP Panel Positioning](https://github.com/mozilla/addon-sdk/wiki/JEP-Panel-positioning) and discussed in etherpad.

https://github.com/mozilla/addon-sdk/commit/a2e18bc37e8eff9b563fec695bea73b07922b036
Merge pull request #826 from ZER0/panel-align/732294

fix Bug 732294 - Panel constructor lacks a way to set position of panel other than "centered" r=@gozala
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
According to bug 878877 we're going to back this out of beta so please prepare a backout patch and nominate for uplift.
Flags: needinfo?(zer0)
We're not going to back this out, this patch won't be affected by bug 878877, we'll still have the capability to set the position of the panel even when bug 878877 will land.
Flags: needinfo?(zer0)
Oh sorry, I didn't read the recent comment in bug 878877. I think it could be dangerous back this out because I recall that at least one other fix is built on top of it; so we should trace all the fixes are relying on these changes before back it out.
Ok, will take the fix in bug 878877 in that case since backout is too risky.
You need to log in before you can comment on or make changes to this bug.