882823 - contentStyleFile not working for tab.attach

Status: RESOLVED FIXED

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

Description

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

Reported here: http://stackoverflow.com/questions/16987722/firefox-addon-sdk-launching-pagemod-when-pressing-a-button

Comment 1

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

Matteo, were you already working on this?

Comment 2

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

I can see a main issue here. Now that we can register stylesheet per document, is not a problem adding both `contentStyle` and `contentStyleFile` to `attach` options; however `tab.attach` was intended only for script, in fact it returns a `Worker`.

In first place, then, I don't think is formerly correct add options to `attach` that are not related to the object returned – `contentStyle*` has nothing to do with a `Worker`. As consequence of that, currently a dev can "remove" the script attached by `tab.attach` calling `worker.destroy()`, but the `contentStyle` cannot be removed in the same way – as said, worker and style are unrelated.

Of course we could listen for the destroy event of the worker returned, and remove the content style too, however it sounds to me a bit hacky.

We could also pass a different object to `tab.attach` and returns a different thing than `Worker` but I honestly think we're going to overcomplicate that stuff.

Even if we found a way to workaround that, my feeling is that devs want to have a sort of "page-mod" that is applied only to a specific document. So even if we fix the contentStyle thing somehow using `attach`, then they could demand `attachTo` for instance – and why not?

We can definitely support the style feature easily using low level API: thanks to the new contentStyle approach, we actually have `attach` and `detach` generic methods, so it's really easy have:

    let style = Style({url: './mystyle.css'});

    attach(style, tab); // `tab` could be both SDK and XUL tab
    
    // detach the style from a specific tab
    detach(style, tab)

The only thing to do is register our Tab class as "target", and that's all, the rest of the code is already in our code base.
 
So my suggestion is implement this functionality on low level API now; and think more carefully about how we want to expose that on high level API, because as I exposed, it's less trivial that it seems.
Maybe it could be worthy having a totally different method for attaching a stylesheet and detach it; or maybe we could thinking to have a way to apply a page-mod to a Tab.

Irakli, because we're talking about high level API I'd like to have your opinion about that.

Comment 3

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

> (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #2)
> I can see a main issue here. Now that we can register stylesheet per
> document, is not a problem adding both `contentStyle` and `contentStyleFile`
> to `attach` options; however `tab.attach` was intended only for script, in
> fact it returns a `Worker`.
> 
> In first place, then, I don't think is formerly correct add options to
> `attach` that are not related to the object returned – `contentStyle*` has
> nothing to do with a `Worker`. As consequence of that, currently a dev can
> "remove" the script attached by `tab.attach` calling `worker.destroy()`, but
> the `contentStyle` cannot be removed in the same way – as said, worker and
> style are unrelated.
> 
> Of course we could listen for the destroy event of the worker returned, and
> remove the content style too, however it sounds to me a bit hacky.
> 
> We could also pass a different object to `tab.attach` and returns a
> different thing than `Worker` but I honestly think we're going to
> overcomplicate that stuff.
> 
> Even if we found a way to workaround that, my feeling is that devs want to
> have a sort of "page-mod" that is applied only to a specific document. So
> even if we fix the contentStyle thing somehow using `attach`, then they
> could demand `attachTo` for instance – and why not?
> 
> We can definitely support the style feature easily using low level API:
> thanks to the new contentStyle approach, we actually have `attach` and
> `detach` generic methods, so it's really easy have:
> 
>     let style = Style({url: './mystyle.css'});
> 
>     attach(style, tab); // `tab` could be both SDK and XUL tab
>     
>     // detach the style from a specific tab
>     detach(style, tab)
> 
> The only thing to do is register our Tab class as "target", and that's all,
> the rest of the code is already in our code base.
>  
> So my suggestion is implement this functionality on low level API now; and
> think more carefully about how we want to expose that on high level API,
> because as I exposed, it's less trivial that it seems.
> Maybe it could be worthy having a totally different method for attaching a
> stylesheet and detach it; or maybe we could thinking to have a way to apply
> a page-mod to a Tab.
> 
> Irakli, because we're talking about high level API I'd like to have your
> opinion about that.

I do think having example from your code working would be a good start with documentation somewhere showing it.

I would also add `console.warn` in `tab.attach` if options contain `contentStyleFile` option that would point to the documentation mentioned above.

I think it would also make sense to implement `attach(mod, tab)` in the next iteration which will just decompose `mod` into `style` and `contentScript` and will attach them to a tab with just `attach`. For this I would create a new bug though, cause I think more work will have to be done on the content scripts side to make it work.

As of high level API I think I'd rather point people interested to an `attach` function, then based on feedback we get, we can either promote it to high level API or make one that would meet users requirements.

Comment 4

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

In order to add the code I described I need to slightly change `tab` module, Irakli. If your refactoring is near to be land, I will wait. Otherwise I will do that now, just let me know your estimation.

Comment 5

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

Now that I'm back my no 1 priority task is to finish my window / tab refactoring. If changes you'll have to make are small I don't mind you doing them without waiting on my patch. Just let me review them so I can include changes to my refactoring code.

I think I would also like some general JEP concerning `attach(mod, target)` and `detach(mod, [target])`
so we can have a better integrated picture on this.

Ultimately I'd like to deprecate `tab.attach` and change it's implementation to:

Tab.prototype.attach = function(mod) attach(mod, this)

Where current implementation of .attach can be moved to

attach.define(Object, ...)

Comment 6

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

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #5)
> Now that I'm back my no 1 priority task is to finish my window / tab
> refactoring. If changes you'll have to make are small I don't mind you doing
> them without waiting on my patch. Just let me review them so I can include
> changes to my refactoring code.

Is there any update on this, Irakli?

> I think I would also like some general JEP concerning `attach(mod, target)`
> and `detach(mod, [target])`
> so we can have a better integrated picture on this.

Agreed. I felt also that the worker's attach/detach are actually a bit sort of dupes, but they work slightly differently: I would love to merge them, but I'm not sure would be possible.

Also, I was wondering if you still want to implement something like that: https://github.com/mozilla/addon-sdk/wiki/JEP-Content-scripts

Because we could basically reuse the same `attach` / `detach` thing – the `Style` was inspired by that – so that we could have something like:

  let script = Script({
    uri: ...,
    source: ...
  });

  attach(script, PageMod);

That, if you want to have the `spawn` method in the object itself, could be implement as:

  Script.prototype.spawn = function(target) { attach(this, target) };

Comment 7

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

Yes I'd like to get eventually have page-mod 2.0 or something alike which would decompose content scripts content styles and page patterns into it's own components that can be composed to work together. I like where we got with stylesheets and I do hope we can do the same for content scripts. Maybe we could also have something like a myMod = Mod([ script, style, ... ]) that is basically composition of scripts and styles that also implements same attach / detach logic. Finally I don't know how yet but I also hope to be able to get to point where you can attach all this things to a tab or to a pattern of pages like mod.attach(pattern("http://google.com/*")).

That being said there is no one actively working or thinking about this.

Comment 8

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

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #7)

> That being said there is no one actively working or thinking about this.

Thanks. And what about the windows / tabs refactoring? If it is still pending, I'd rather to add this feature.

Comment 9

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

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #8)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #7)
> 
> > That being said there is no one actively working or thinking about this.
> 
> Thanks. And what about the windows / tabs refactoring? If it is still
> pending, I'd rather to add this feature.

I'm not sure I got what you're saying here, but in regards to your question I had
to move to other stuff that had a higher priority so not much going on there.

I'm also little less motivated to do the window / tab work refactoring at this point
as I'm pretty sure with E10S we'll have to change them. For all the same reasons why
I believe chrome has async methods to obtain lists of open tabs / windows:
https://developer.chrome.com/extensions/tabs#method-getAllInWindow

Comment 10

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

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #9)

> > Thanks. And what about the windows / tabs refactoring? If it is still
> > pending, I'd rather to add this feature.

> I'm not sure I got what you're saying here, but in regards to your question
> I had
> to move to other stuff that had a higher priority so not much going on there.

Okay, that what I wanted to know: I basically paused this bug 'cause you said that you were finish the refactoring of window / tab, but because it wasn't landed yet I wanted to be sure that you weren't actively working on them and in such case, I could actually implement this bug without waiting your patch.

> I'm also little less motivated to do the window / tab work refactoring at
> this point
> as I'm pretty sure with E10S we'll have to change them. For all the same
> reasons why
> I believe chrome has async methods to obtain lists of open tabs / windows:
> https://developer.chrome.com/extensions/tabs#method-getAllInWindow

Indeed, I agree with you.

Thanks!

Comment 11

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

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1566

Comment 12

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

Comment on attachment 8462826 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1566

Looks good to me, thanks!

Comment 13

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/63466463b0dec99b2d576de15b7db3addcc3b5de
Bug 882823 - contentStyleFile not working for tab.attach

 - added tabs as valid target for both `attach` and `detach`

https://github.com/mozilla/addon-sdk/commit/80391cda74f95f03ec624bdca45a336ec24a96d9
Merge pull request #1566 from ZER0/tab-style/882823

fix Bug 882823 - contentStyleFile not working for tab.attach r=@gozala

Comment 14

[willmarquardt]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #12)
> Comment on attachment 8462826 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1566
> 
> Looks good to me, thanks!

This is still not working either for contentStyle or contentStyleFile. Made the following extension:

const tabs = require('sdk/tabs');
const { url } = require('sdk/self').data;

main.js
-------

tabs.on('ready', function(tab) {
  tab.attach({
    contentStyle: "body { border: 5px solid blue; }",
    contentStyleFile: url('content.css'),
    contentScriptFile: url('content.js')
  });
});

content.css
-----------

body {
    border: 5px solid red;
}

content.js
----------

console.log('attached');

The script is getting attached but the style aren't.

From a dev's perspective, not knowing what's going on on the browser level, it would be very useful if the contructor options were consistent across PageMod, tab.attach, Panel, and PageWorker.

Comment 15

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

(In reply to willmarquardt from comment #14)

> This is still not working either for contentStyle or contentStyleFile.

We didn't add `contentStyle*` to `tab.attach` for the reason explained in comment 2.
Instead, we make attach a style to a tab possible, using low level APIs – see again comment 2.
> 
> const tabs = require('sdk/tabs');
> const { url } = require('sdk/self').data;
> 
> main.js
> -------
> 
> tabs.on('ready', function(tab) {
>   tab.attach({
>     contentStyle: "body { border: 5px solid blue; }",
>     contentStyleFile: url('content.css'),
>     contentScriptFile: url('content.js')
>   });
> });

Considering that only as a test – otherwise a PageMod would be more suitable for such thing – it should be rewritten as follow:

  const tabs = require('sdk/tabs');
  const { attach } = require('sdk/content/mod');
  const { Style } = require('sdk/stylesheet/style');

  let style = Style({
     source: 'body { border: 5px solid blue }',
     uri: './content.css'
   });

  tabs.on('ready', tab => {
    tab.attach({
      contentScriptFile: './content.js'
    });

    attach(style, tab);
  });

> From a dev's perspective, not knowing what's going on on the browser level,
> it would be very useful if the contructor options were consistent across
> PageMod, tab.attach, Panel, and PageWorker.

Unfortunately, there was some bad design decision at the beginning, so that `tab.attach` returns a worker, and therefore is not suitable to use it for different things like styles. Changing now what is the value returned would be bad; the best would be probably having something like what Irakli said in comment 7, in the future.

Comment 16

[willmarquardt]

Thanks for the detailed response. I'll look into the style module. I agree with comment 7 as well. 

> Considering that only as a test – otherwise a PageMod would be more suitable for such thing
Agreed, just wanted to make a simple example.