Closed Bug 708190 Opened 13 years ago Closed 12 years ago

page-mod should be able to apply to existing tabs automatically

Categories

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

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jwkbugzilla, Assigned: ochameau)

References

Details

Attachments

(1 file, 1 obsolete file)

When an add-on using page-mod is installed mid-session its content scripts don't apply to existing tabs which was a bit surprising. So for the add-on to start working immediately you have to do something like:

  var include = "*.example.com";
  var { MatchPattern } = require("match-pattern");
  var pattern = new MatchPattern(include);

  var pageMod = require("page-mod");
  pageMod.PageMod({
    include: include,
    contentScriptWhen: 'end',
    contentScriptFile: contentScript
  });

  var tabs = require("tabs");
  for (var i = 0; i < tabs.length; i++)
    if (pattern.test(tabs[i].url))
      tabs[i].attach({contentScriptFile: contentScript});

Given that applying to existing tabs is a common requirement - the SDK should be able to do this itself. To keep backwards compatibility one could add a parameter "applyToExisting: true" that would trigger this behavior.
When we originally implemented page mods, we intentionally didn't apply them to existing tabs, mostly to reduce the scope of the implementation so we could get it done in a reasonable timeframe, since there's a tricky issue.

But now that page mods are done and in use, it's the right time to revisit such decisions.  And I agree that this is a common expectation, so we should apply page mods to existing tabs where possible.

It's a bit tricky, because content scripts attached at "start", which means document-element-inserted (i.e. after the <document> element has been created but before any scripts have run) are unlikely to work with existing tabs.

And those attached at "ready", which means DOMContentLoaded, are similarly less likely to work (although perhaps more likely than for "start").

But those attached at "end", which means the "load" event, will work just fine in most cases.

So we might be able to automatically attach content scripts to existing tabs when contentScriptWhen is "end" (the default value), while requiring authors to manually specify that content scripts attached on "start" or "ready" should nevertheless attach to existing tabs (because the author has tested them and knows they work).

(Not sure it's worth the complexity and uncertainty of having the behavior differ between contentScriptWhen values; thinking out loud here.)
(In reply to Myk Melez [:myk] [@mykmelez] from comment #1)

Wouldn't this cause issues with backwards compatibility? After all, mine is probably not the only add-on already attaching content scripts to existing tabs "manually". Having two identical content scripts in one tab might cause nasty side-effects.
Yes, it likely would!

So we'd either have to decide to make the breaking change (entraining lots of communication about what we're doing, why we're doing it, and how to update your addons to accommodate it, along with perhaps other measures to identify addons susceptible to bustage and help them get updated); or we can do what you suggest and make a backward-compatible change by adding an option that induces the behavior.

We have a pretty high bar for breaking changes.  This functionality probably isn't important enough to justify breaking the API, and we should make the backward-compatible change you suggest.
Severity: normal → enhancement
Priority: -- → P1
Attached patch Implement this feature (obsolete) — Splinter Review
Here is a way to implement this.
I'd say that this behavior should be the default one, because of startup issue with session history. Having said that, I'm wondering one thing. Shouldn't tabs be restored only when user click on "restore my tabs" button on about:home?
So that addons should already be loaded and page-mod registered?

Wladimir: Is there any issues around Firefox startup ? Or is this only about mid-session page-mod creation ?
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
(In reply to Alexandre Poirot (:ochameau) from comment #4)
> Created attachment 582871 [details] [diff] [review]
> Implement this feature
> 
> Here is a way to implement this.
> I'd say that this behavior should be the default one, because of startup
> issue with session history. Having said that, I'm wondering one thing.
> Shouldn't tabs be restored only when user click on "restore my tabs" button
> on about:home?
> So that addons should already be loaded and page-mod registered?
> 
> Wladimir: Is there any issues around Firefox startup ? Or is this only about
> mid-session page-mod creation ?

In my case, I automatically have my tabs restored on startup.
I think that Jetpack add-ons start up before session restore so that the issue is really about mid-session page-mod creation.
Blocks: 733582
Attached file Pull request 404
Irakli, could you give me your feedback on the public API so that I can write related documentation?
Attachment #582871 - Attachment is obsolete: true
Attachment #615734 - Flags: feedback?(rFobic)
Myk, Wladimir, you are more than welcome to give your feedback too as you started this discussion!

So I'm suggesting to add an `apply` attribute that acts like `allow` attribute of Page-Worker,Panel,Widget APIs:
https://addons.mozilla.org/en-US/developers/docs/sdk/latest/packages/addon-kit/page-worker.html#allow

This `apply` attribute is an object of this form:
apply: {
  existing: true, // For the purpose of this bug
  iframes: true   // For another bug, where we want to avoid attaching script to iframes.
  top: false
}

It will allow multiple configuration on attachment rules for page-mod.
I hope that this new attribute will allow to handle bug 684047 with the same pattern.
I will strongly suggest to avoid naming that property `apply`. Even if it's a property, it makes confusing because the native homonym method: when you look at first sight to a js code and you see `bind`, `apply`, `call` or `prototype` you know what they means. That's the same reason I guess in "heritage" module Irakli removed the `prototype` property because sounds confusing, and jQuery renamed its `bind`.

Also, have they been only boolean values? I think we could, as the bug 684047 seems to suggests, have one property (maybe `target` or similar) that contains strings like "top" or "frames".

It looks a bit weird mixing these two properties together, I mean if we looks in that way everything could be under "apply" (`include` as well). Maybe it could make more sense have `target` as top property (like `include`) and have a different property to apply to existing loaded pages – or even a method, maybe, to call explicitly? But maybe that makes the things more complex.
You are right, the conflict with Function.apply is unfortunate. We should choose something else. What about `target` ?

I choosed to use the nested configuration attribute for two reasons:
- we are already using this pattern with `allow` attribute,
- in order to avoid piling these contentScriptSomething attributes, we are speaking about adding two new such attributes.

Then, we should not put anything else than attributes with boolean values.
This pattern is just a more Javascripty way to define something like binary flags. Like this: pagemod.target = APPY_ON_EXISTING | APPLY_ON_TABS;

I wasn't sure we would like to end up with such list of attributes:
 contentScript
 contentScriptFile
 contentScriptWhen
 contentScriptTarget
 contentScriptApplyExisting (or whatever name we come up with)
But if you think it is better I can buy it.

Matteo, Do you have any concrete alternative suggestion for naming?
I totally agree that we shouldn't end up to have all these contentScriptSomething attributes. However, I do believe that `apply` or `applyExisting` or whatever are not in the domain of `contentScript` but in the domain of page-mod itself, like `include` for instance. So the property (or properties if we decide to split) shouldn't have `contentScript` as prefix, in my humble opinion.

I like bitmask operator in general, but in javascript they aren't really common, so I totally agree with you. However in JavaScript is also a common pattern use strings – see the event handlers, our properties like `contentScriptWhen`, etc – so something like:

    pagemod.target = ["frames", "top"];

Could works and it's more compact then:

    pagemod.target = {
        frames: true,
        top: true
    }

But I don't have strong feelings about it. 

IMVHO we should split the `target` and `apply`, and rename the last one in something else to avoid conflict with Function.apply. For the last one, I don't have a concrete alternative suggestion, because it feel unnatural that we need a property for that: it should be the normal behavior. So I guess any name will found for it will sound a little bit weird.

What about something with `attach` as prefix? At the end, that's what it does. Maybe could be also something like `contentScriptWhen`, instead of a Boolean: so we have different values like "new" and "all" and we have as default value "new" (like contentScriptWhen has its default).

I don't know, I just throw some ideas... As I said, everything I can think of it's a bit weird because it feels unnatural.
I have the same feeling, this should be the default behavior.
The main issue is about existing addons that have implemented a workaround, like:
PageMod({
  include: "*",
  contentScript: cs
});
for each(let tab in tabs)
  tab.attach({
    contentScript: cs
  });

There is some addons already doing this. If we apply to existing tabs, they are going to break. Here we have two options: 
 - consider that it should really be the only possible behavior and put extra warning in release note and mailing list about this breaking change in order to ensure such addons to be hopefully modified. [We won't have to introduce this attribute]
 - same thing but we consider that some addons may want to use the old behavior for some reasons. I don't see any? [We will have to introduce this attribute]
 - or we consider we should not ever break these addons and introduce this attribute and set it to default to the apply only on new documents

That may be something to discuss before pulling hairs on choosing a name for this flag!

TBH, I think page-mod should apply to existing and new documents by default, without having to set any particular flag. It will simplify the use of jetpack and its documentation. I don't see value in applying only to new documents. 
But what about compatiblity, should we prefer compatibility over easier/simplier jetpack API?
(We are most likely going to have same debate in bug 684047)

Daves, what your thoughts about this?
I agree with you Alex.

I can't imaging any user cases where you would apply the PageMod only to new tabs and not the one you already have opened. Maybe one thing we could do is asking in ML if there is any.

Another thing could be check in MXR how many add-ons are actually using PageMod and applying this workaround.

In my opinion we're going to fix a flaw in PageMod, not introducing a new feature. The proper behavior should be applied to all tabs not only new ones: keep in mind that `contentStyle*`, because is registering css as userstyle sheet, works in that way already. So for the new coming PageMod add-ons that are using both `contentStyle*` and `contentScript*` we could have a kind of discrepancy (where the style is applied to all tabs and the script only to the new ones).

Maybe as you said Alex, properly warning in docs, ML, and when developers repack their add-on with the new SDK could be good enough. I guess there are two kind of PageMod add-on: the ones that care about applied the contentScript to all tabs, and the ones they don't. If the first ones get the warning, they have to remove the manual loop code without adding anything else – if we add the new property, they have to add the new property as well. The second ones doesn't have to do anything.

We're not actually "broke" any promises about APIs, the APIs will be the same: we're fixing a wrong behavior, so the workaround won't needed anymore. It's like on web, when for cross-browsing stuff you need to create workarounds, and you remove them when the APIs properly works in all browsers.

I can imaging is a delicate matter, they are just my two (euro)cents.
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #14)
> We're not actually "broke" any promises about APIs, the APIs will be the
> same: we're fixing a wrong behavior, so the workaround won't needed anymore.
> It's like on web, when for cross-browsing stuff you need to create
> workarounds, and you remove them when the APIs properly works in all
> browsers.

APIs include the official documentation that we write about how those APIs are expected to behave and right now the docs say very early on "if your add-on is loaded while the user's browser is open, the user will have to reload any open pages that match the mod for the mod to affect them". I'd definitely consider this to be an API breaking change.

But it does feel like the correct default behaviour is to always apply. The only cases we're changing is when the add-on is installed/enabled after there are affected tabs, or if a page-mod is created sometime after startup. The latter seems an unlikely case, I bet every add-on always creates them as early as possible.

(In reply to Alexandre Poirot (:ochameau) from comment #13)
> I have the same feeling, this should be the default behavior.
> The main issue is about existing addons that have implemented a workaround,
> like:
> PageMod({
>   include: "*",
>   contentScript: cs
> });
> for each(let tab in tabs)
>   tab.attach({
>     contentScript: cs
>   });

A possible way to fix this case it to make it silently ignore attempts to attach the same script twice. That is an API breaking change too, but seems very unlikely to cause issues, also seems potentially useful. Thoughts?

> But what about compatiblity, should we prefer compatibility over
> easier/simplier jetpack API?
> (We are most likely going to have same debate in bug 684047)

There is no one answer to this question unfortunately, it's a judgement call everytime. Based on the things we've said publicly and the impressions that developers have though I think we have to err on the side of no-bustage in general.

What it comes down to is, how many people will we break with this change? Do we have any way to gather data on that? We can see all the source for add-ons on AMO and potentially email all the authors. We can blog about it and post in the mailing list. Does it make a difference depending on the contentScriptWhen property (I'd suspect that "start" scripts are more likely to be broken when run against existing pages)?

If we can be fairly confident that the amount of breakage is low then we should absolutely choose the best API. If we can't then we probably have to suck it up and take the additional complexity.
This is quite definitely a breaking change. In addition to two instances of the same content script being applied to the same tab I can see one more issue: this might break content script assumptions. Some extensions might be using contentScriptWhen: "ready" to get loaded before the page initialized (typically in the "load" event). If they get applied to an already loaded page things might break. Also, some pages change significantly after load (e.g. via AJAX-based navigation), so applying content scripts at a random point in time makes no sense. But estimating the amount of breakage will IMHO be very hard.
I think Wladimir makes an excellent point, `contentScriptWhen` declares when content script should be loaded. Non of the currently possible values matches the case of applying page-mod to the already loaded documents.

I also tempt to prefer API suggested by ZER0:
target: ["frames", "top", "existing" ]

Although, I think property name `handle` or `cover` is better for that. I don't like `apply` for the reasons already outlined.

I could also by into:

handleFrames: true
handleTop: false
handleExisting: true


I don't think breaking change pays off at this point. I like that this behavior is opt-in. If in a future we'll learn that this is a best default we'll have a good argument to make it so. I'd suggest to do incremental improvements, let's make it possible first without breaking anything. Then we can consider making it a default.
Comment on attachment 615734 [details]
Pull request 404

As I need this to improve addon builder helper, I'll go for the additional flag without changing default behavior:
{ target : ["existing"] }
I've updated the pull request with minimal documentation.
In page-mod, I've intentionally not used any tab API as it is important to be efficient here and we should not create costly helper Tabs object in order to achieve this. But we could extract this little tabs iterator if it is usefull for another module?
Attachment #615734 - Flags: review?(rFobic)
irakli: I received some feedback on irc, would `attachTo` work for you?

wbamberg | ochameau: 'attachTo: ["existing", "top", "frames"]'  or 'applyTo'? 
  Mossop | ochameau: attachTo seems good to me too.
Attachment #615734 - Flags: review?(rFobic) → review+
(In reply to Alexandre Poirot (:ochameau) from comment #19)
> irakli: I received some feedback on irc, would `attachTo` work for you?
> 
> wbamberg | ochameau: 'attachTo: ["existing", "top", "frames"]'  or
> 'applyTo'? 
>   Mossop | ochameau: attachTo seems good to me too.

This may conflict with https://bugzilla.mozilla.org/show_bug.cgi?id=755963
See Also: → 755963
I opened a scratchpad to take final decision on naming:
  https://etherpad.mozilla.org/new-page-mod-options
Blocks: 760853
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.11
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: