Closed Bug 638142 Opened 13 years ago Closed 11 years ago

Add ability to anchor a panel to a widget, button, or any browser DOM element

Categories

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

Product:
Add-on SDK Graveyard
Component:
General
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: msucan, Unassigned)

References

Details

(Whiteboard: [papercuts][triage-followup]) 

We currently can associate widgets to panels, such that when the widget is clicked, the associated panel is opened and anchored at the widget element. However, if we define our own onClick handler for the widget, we loose the ability to open the panel anchored to the widget element.

Even if panel.show(anchor) allows us to give it an anchor element, the widget object doesn't give us any reference to the widget element.

Proposed solutions:

- the widget object should give us a reference to its addon bar element.
- if that's not acceptable, then give us a widget.panelShow() method that calls panel.show(widgetElement) for us.
I think getter/setter for the panel sounds fine, and very flexible.

You could then do things like easily hide and show the panel from within event handlers, and could swap out panels.
Ignore comment #1, we already have that :)

Option #1 to expose the DOM element is a non-starter for e10s.

Option #2 is feasible, but I'd rather not expand the core widget API for this if possible.

There's an option #3, which is to have panel.show() use an anchor provided to the ctor (or previously passed to show()).

This means that for anchored panels you don't have to keep passing in the anchor every time you show it, which is kind of nice. And also means we can do this in widgets:

widget.panel.show()/hide()

which is intuitive.

The only disconcerting part is that sometimes show() w/o an anchor uses and anchor, and sometimes it doesn't.

Myk, what are you thoughts on resolving this?
(In reply to comment #2)
> There's an option #3, which is to have panel.show() use an anchor provided to
> the ctor (or previously passed to show()).
> 
> This means that for anchored panels you don't have to keep passing in the
> anchor every time you show it, which is kind of nice. And also means we can do
> this in widgets:
> 
> widget.panel.show()/hide()
> 
> which is intuitive.
> 
> The only disconcerting part is that sometimes show() w/o an anchor uses and
> anchor, and sometimes it doesn't.

The other problem is that widgets have multiple anchors, one per browser window, and the anchor to which the panel should be anchored may not be the same one to which it was anchored last time (or the first anchor created when the widget was first instantiated).

Option #4 is to make panel.show() accept a widget object to which it should be anchored:

  widget.panel.show(widget)

This still runs into the multiple anchor problem, so the panel would have to query the widget for the anchor for, say, the widget in the topmost window (which gets into the issue of how to share private information between the widget and the panel without exposing it to API consumers).

But at least it would avoid the problem of show() without an anchor sometimes using an anchor.

Hmm, I need to ponder the API design issues here some more...

Also cc:ing Irakli for his thoughts regarding how we can implement the sharing of private information between these two APIs.
> The other problem is that widgets have multiple anchors, one per browser
> window, and the anchor to which the panel should be anchored may not be the
> same one to which it was anchored last time (or the first anchor created when
> the widget was first instantiated).

Right now we mirror widgets across windows for the caller, so this fact of multiple anchors already existing is never exposed to add-on devs, it's handled inside the widget implementation.

This mirroring feature could be removed. However, for the common case, I think that's highly undesirable, as it creates a whole bunch of work for the add-on developer.

While I'd prefer not to make widgets' API even bigger, widget.show/hidePanel() seems to be the solution that addresses all issues in a way that's still syntactically clearest to the caller.
The possibility of sharing private information between APIs/modules is quite important. We'd need a common solution across jetpack modules. Is there any bug or discussion going on about this? To find a solution.
I don't want to conflate these two issues. Anchoring a panel to a widget is one (simple) thing that could be nicely addressed by Dietrich's solution in comment #4. widget.show/hidePanel()

For cross module API, I think we're going to want and need something in the devtools sdk, possibly residing in chrome that acts as a broker -- god help me if we reinvent COM again, but I'm hoping we don't need something that heavy.

I'll file that bug as soon as a hit the save changes button.
(In reply to comment #6)
> I don't want to conflate these two issues. Anchoring a panel to a widget is one
> (simple) thing that could be nicely addressed by Dietrich's solution in comment
> #4. widget.show/hidePanel()

Yup, I agree.  Let it be widget.show/hidePanel().
created bug 639518 to talk about cross-addon communications.
Priority: -- → P2
Target Milestone: --- → 1.0
Whiteboard: [papercuts]
(In reply to comment #5)
> The possibility of sharing private information between APIs/modules is quite
> important. We'd need a common solution across jetpack modules. Is there any
> bug or discussion going on about this? To find a solution.

Mihai I have been exploring different ways to approach that and for the moment solution I find most flexible is the one I blogged about: http://jeditoolkit.com/2011/04/11/shareable-private-properties.html#post

Also here is code demonstrating how to use it:
https://github.com/Gozala/jetpack-io/tree/master/lib

I believe that could be good way to go forward about this
(automatic reprioritization of 1.0 bugs)
Priority: P2 → P1
Target Milestone: 1.0 → 1.1
Assignee: nobody → myk
Severity: normal → enhancement
Re-prioritizing all 1.1-targeted feature requests to 1.2.
Target Milestone: 1.1 → 1.2
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: 1.2 → ---
Unassigning myself from bugs I am not actively working on.
Assignee: myk → nobody
Whiteboard: [papercuts] → [papercuts][triage-followup]
Assignee: nobody → rFobic
Assignee: rFobic → jsantell
Jordan, are you actively working on that?
I was wondering if it makes sense to add this functionality, now that we want to deprecate Widget. Because the current code of Widget "encapsulate" the XUL node created per view, it probably requires some changes to the Widget API too in order to implements this feature (unless we do some "hack" using the id).

If the feature is not implemented yet, I would suggest to don't add enhancement on Widget API side – we want to deprecate it - and focus on new UI components instead, and add this capability for the upcoming `Button`.
Flags: needinfo?(jsantell)
No, I'm not currently working on this -- but agreed, lets revisit this regarding all the new UI components
Flags: needinfo?(jsantell)
Can we close this?
Flags: needinfo?(dtownsend+bugmail)
If we don't implement this, we will stop other legit uses of passing anchor element to the panel.

See my IME add-on [1]. You can get a sense of how the panel anchors to the element with image on [2].

[1] https://github.com/timdream/jszhuyin-firefox
[2] http://blog.timc.idv.tw/posts/firefox-addon-jszhuyin-ime/

I *could* re-implement that maybe top/left/height/width... but I will lost the arrow and will have to dup a lot of positioning code to my add-on.
Assignee: jsantell → nobody
Summary: Add ability to anchor a panel to a widget → Add ability to anchor a panel to a widget, button, or any browser DOM element
Erik, it seems to me that this bug is overlaps with bug 859216; I think we should close one of them.

About how to handle this feature, see also my comment there: https://bugzilla.mozilla.org/show_bug.cgi?id=859216#c3
Or maybe we could keep one for high level API implementation (SDK objects, content document); and one for low level API implementation (display the panel anchored to any DOM node instance).

In that case, I would suggest to keep this one for high level, and the other one for low level. What do you think?
When Australis lands widget will be deprecated so we won't be working on this.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
Flags: needinfo?(dtownsend+bugmail)
Info for the triage-followup:
The new button API implementation for Australis is bug 907374 which blocks
bug 907450 "Anchored Panels" that looks like the successor to this bug.

On IRC Mossop pointed to bug 787390 as a related bug of interest to follow and
bug 695913 is the tracking bug blocked both by that one and Australis.

https://forums.mozilla.org/addons/viewforum.php?f=27 seems ideal for further discussion.
You need to log in before you can comment on or make changes to this bug.