859216 - panel.show() should take a element id

Status: RESOLVED WONTFIX

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

Description

[Erik Vold [:erikvold] (please needinfo? me)]

panel.show(id) where id is a string should work.

Behind the scenes it should do require('sdk/window/utils').getMostRecentBrowserWindow().document.getElementId(id) and use that as the anchor (assuming the anchor is found).

Comment 1

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

Matteo, is this a dupe of something you're working on?

Comment 2

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

So, what we want to do is permit the panel to be displayed to a specific Widget / new UI Button as anchor; not to any XUL node. This is something low level that I don't think should be supported on high level API – as Erik said in the comment is still doable using low level API and it's fine IMHO.

And that's would be start to be fixed in bug 638142.

Comment 3

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

I would split the issue in two: high level API, and low level API.

1. Using High Level API, the dev should be able to anchor a panel to the High Level SDK UI components; without exposing DOM node.

2. Using High Level API, the dev should be able to anchor a panel to a content node of a content document – not Firefox UI – without using DOM node.

3. Using Low Level API, the dev should be able to anchor a panel to a node instance, like a Firefox UI XUL node.

The first point should be covered by https://github.com/mozilla/addon-sdk/wiki/JEP-Anchored-Panels.

The second point should be covered by `panel.show` method, passing a different `position` object – or directly a string, like:

    // the panel will be anchored to the first node of the
    // content document that matches the query; we have to
    // choose if the panel should be displayed using default
    // position if there isn't any nodes that matches the query
    // or it shouldn't displayed at all
    panel.show({position: "#my-node-id"});
    panel.show({position: ".button:not(:disabled)"});

    // we could be also more explicit using a 'function':
    panel.show({position: node("#my-node-id")}); // or something like that

And for the third point, we could have some low level function like `show` or similar:

    show(panel, {anchor: domNode});

Of course currently the 2nd point could be covered by 3rd point too, however the additional layer of abstraction for high level API in the 2nd point is needed in case of future change with e10s or similar, where the content node could run in a different thread / process of the add-on / chrome code.

Comment 4

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

With the new `position` API and anchored panels pull merged, I don't think this bug makes sense unless there is some common case we want cover in the SDK itself. This functionality can be added now by anyone in their add-on or 3rd party module, without touch the panel's code itself. The code would be something like:

  getNodeView.define(XULWrapper, wrapper =>
    getMostRecentBrowserWindow().document.querySelector(wrapper.selector));

  let panel = require('sdk/panel').Panel();

  let reload = XULWrapper("#urlbar-reload-button");

  panel.show({position: reload});

So now people can easily create a class object that redefine the `getNodeView` method, and implement it's own way to anchor a panel to any suitable target. We could probably document this in MDN, maybe in a tutorial, and link that to the `panel` documentation. 

What do you think, guys?

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #4)
> With the new `position` API and anchored panels pull merged, I don't think
> this bug makes sense unless there is some common case we want cover in the
> SDK itself. This functionality can be added now by anyone in their add-on or
> 3rd party module, without touch the panel's code itself. The code would be
> something like:
> 
>   getNodeView.define(XULWrapper, wrapper =>
>     getMostRecentBrowserWindow().document.querySelector(wrapper.selector));
> 
>   let panel = require('sdk/panel').Panel();
> 
>   let reload = XULWrapper("#urlbar-reload-button");
> 
>   panel.show({position: reload});
> 
> So now people can easily create a class object that redefine the
> `getNodeView` method, and implement it's own way to anchor a panel to any
> suitable target. We could probably document this in MDN, maybe in a
> tutorial, and link that to the `panel` documentation. 
> 
> What do you think, guys?

I'm think this would be useful, afaict the new api doesn't change much from what I said in comment 0.

Comment 6

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

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)

> >   getNodeView.define(XULWrapper, wrapper =>
> >     getMostRecentBrowserWindow().document.querySelector(wrapper.selector));
> > 
> >   let panel = require('sdk/panel').Panel();
> > 
> >   let reload = XULWrapper("#urlbar-reload-button");
> > 
> >   panel.show({position: reload});
> > 
> > So now people can easily create a class object that redefine the
> > `getNodeView` method, and implement it's own way to anchor a panel to any
> > suitable target. We could probably document this in MDN, maybe in a
> > tutorial, and link that to the `panel` documentation. 
> > 
> > What do you think, guys?
> 
> I'm think this would be useful, afaict the new api doesn't change much from
> what I said in comment 0.

The API are changed enough to permit such code without modifying panel or using unofficially supported features.
Before it wasn't possible pass in the `show` method and `constructor` an object as `position`'s object that would have a custom logic to define what the target was; now it's fully supported, and it's "officially" supported; see my comment 3: anchored panels bring some stuff to it – before it was an hack, see also passing a DOM node directly as second argument, that is deprecated.

I personally don't think we have enough common cases to make this as a feature of SDK, especially because can be easy done without modify a bit of the panel's code itself.
I would prefer keep the API narrow, implementing what prevents the devs to do what they want but they can't without modifying the code of SDK itself, or in a terrible complex way / hack, and I don't think this is the case.
This is also fall in the gray area where high level API seems mixed a bit to low level stuff.

I think it's enough simple to be just documented, or in the worst case can be a 3rd party module, that if we found is commonly used by different developers, we can implement it later on.

Let's see the rest of the opinions – I forgot to add jeff in the first batch, my bad – I can always be convinced otherwise. 

P.S.
In the code above I forgot the implementation of `XULWrapper` that would be just a `Class` with a `selector` property:

  const { XULWrapper } = Class({
    initialize: function(selector) {
      this.selector = selector;
    }
  });

Or something like that.

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I agree with quoted comment below that Matteo did earlier 
> 
> 2. Using High Level API, the dev should be able to anchor a panel to a
> content node of a content document – not Firefox UI – without using DOM node.
> 
> 3. Using Low Level API, the dev should be able to anchor a panel to a node
> instance, like a Firefox UI XUL node.

I do think they are different use cases and require different solutions. I do like XULWrapper example as a solution for 3rd use case. I don't think we need to do more than that as I don't wanna bind ourselfs to XUL.

I'd suggest someone (I'd propose Matteo since all the kudos for this needs to go to him) to write a blog post about this, maybe with a an associated third party library so ppl could just drop the file in and use it.

As of 2nd use case I think that's little trickier, for one e10s is happening so we can not assume content node's will be accessible from chrome. Other than that there is also behavioral differences, for example content scrolling should probably cause panel to hide (which I don't think is a constraint in other cases.

P.S.: I think we may wanna have an escape clause, for example if underlying panel target isn't visible maybe panel should not be displayed at all. It would be nice to embed it into panel.show somehow so that
getNodeView implementation could say return `null` in cases where target node isn't in a viewport and there for panel should not be displayed.

Comment 8

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I agree with Irakli, clearing the needinfo.

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Echoing Jeff and Irakli

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

Hey Matteo, do you still have time to work on this? can you un-assign yourself if not please?

Comment 11

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

As said in comment 7, and agreed by the rest of the team, we're not going on work on this – in a form of code – but instead writing a blog post. I'm keen to do so, if jeff or someone else can review it before publish it.

Jeff, are you OK with that?

Comment 12

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Sounds good to me.