577709 - onSelect doesn't trigger for already open tabs

Status: RESOLVED FIXED

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

Description

[tct]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; ro; rv:1.9.2.3) Gecko/20100403 Fedora/3.6.3-4.fc13 Firefox/3.6.3
Build Identifier: Jetpack 0.5; Mozilla/5.0 (X11; U; Linux x86_64; ro; rv:1.9.2.3) Gecko/20100403 Fedora/3.6.3-4.fc13 Firefox/3.6.3

I wrote this code in Add-ons Builder:

let selection = require("selection");

selection.onSelect = function ()
{
  selection.text = "converting";
}

and tested it, but it changes the selection only for new tabs, not for the already open ones, although this example is presented in the Builder & SDK tutorials:

https://jetpack.mozillalabs.com/sdk/0.5/docs/#module/jetpack-core/selection

Reproducible: Always

Steps to Reproduce:
0. Open some web pages in tabs
1. Write the selection example code in Add-ons Builder
2. Press 'Test' button
3. Select text from previously open tabs
Actual Results:  
No change made.

Expected Results:  
Selected text to be converted to "converting" string.

Comment 1

[Dietrich Ayala (:dietrich)]

Eric, can you check this?

Comment 2

[Eric Jung [:ericjung]]

confirmed

Comment 3

[Eric Jung [:ericjung]]

Attachment: first fix attempt

This doesn't fix the problem, but I don't understand why at the moment. I removed the dependency on tab-browser in place of tabs. Anyone see any obvious mistakes?

Comment 4

[Myk Melez [:myk] [@mykmelez]]

I don't see anything.  Note, however, that it would be better to use tab-browser rather than tabs, since we're going to have to remove access to contentDocument from tabs in order for it to be e10s-compatible.  And it seems like it should be possible to make the same fix using tab-browser.

Comment 5

[Eric Jung [:ericjung]]

(In reply to comment #4)
> I don't see anything.  Note, however, that it would be better to use
> tab-browser rather than tabs

Are you sure? I was very highly encouraged to replace the use of tab-browser with tabs at the Summit.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #5)
> (In reply to comment #4)
> > I don't see anything.  Note, however, that it would be better to use
> > tab-browser rather than tabs
> 
> Are you sure? I was very highly encouraged to replace the use of tab-browser
> with tabs at the Summit.

I'm not positive, but `tabs` is a high-level module intended to be used by addon developers to create addons, while `tab-browser` is a low-level module intended to be used by high-level modules like `selection`.

So `tab-browser` exposes primitives such as DOM objects, while `tabs` will only exposes high-level `Tab` objects once bug 593908 gets fixed.  And it seems like `selection` needs those primitives.  So I think it will need to use `tab-browser`.

cc:ing our resident high vs. low-level modules and E10S guru for his thoughts, however.

Comment 7

[Dietrich Ayala (:dietrich)]

myk: will tab-browser work the same in e10s world?

i recommended that eric use the tabs module at the summit, figuring that the e10s version would still provide a mechanism (via remoted-script) to get at the content document.

at the time i was planning on replacing/merging tab-browser and tabs. but i'm clearly unclear on how e10s-ification will affect things, and the role of low-level vs high-level.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Dietrich: I'm not sure if tab-browser will work the same in the e10s world.

There should be a way for modules to track tabs that gives them access to the window and document DOM objects of the pages loaded into the tabs, and it seems like that could be accomplished by making tab-browser a module that only works in the chrome process and provides same-process access to those DOM objects.

But perhaps it makes more sense to merge tab-browser and tabs, then provide access to those DOM objects from only the chrome-process-facing part of the combined module.

We need Atul to provide input here.  Atul: what's the right thing to do here?

Comment 9

[Eric Jung [:ericjung]]

How do we get Atul's attention?

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 471600 [details] [diff] [review]
first fix attempt

How about we ask him to review this patch?

Comment 11

[Atul Varma [:atul]]

Comment on attachment 471600 [details] [diff] [review]
first fix attempt

Just to be clear, by 'e10s-ification' I assume you mean the process by which we're making Jetpack-based addon code run in its own process, rather than the process by which we adapt the SDK to work with content processes (e.g. Fennec), as the latter isn't on the roadmap for 1.0.

If that's the case, then I agree with Myk: we won't be able to provide direct access to DOM objects from the 'tabs' module, but we can from the 'tab-browser' module. Note, though, that once we add content process support post-SDK-1.0, we'll need to do something else, as DOM elements won't be accessible from the chrome process either. :)

As such, I'm going to r- this review since it uses 'tabs' instead of 'tab-browser'... Sorry about that confusion, Eric. :(

Other notes:

In general, I recommend keeping tab-browser separate from tabs for now: if anything, we'll be modularizing code even more when we add e10s support. For instance, the "front end" of an API that provides argument validation and other developer-ergonomic features may need to be split off into a separate module that imports the "back end", thus allowing the front end to be loaded in either the chrome or addon process, depending on what's importing it.

In other words, I'd prefer if we didn't coalesce any modules until after the e10s-ification process--and even then, we may want to consider the impacts of future content-process-e10sification before coalescing too.

This should all make more sense once we land the e10s framework in bug 567703, which should hopefully be sometime next week! (crossing my fingers.)

Comment 12

[Dietrich Ayala (:dietrich)]

Ugh, sorry for giving you wrong direction at the summit Eric. the future of high vs low level modules was not as clear then.

Comment 13

[Eric Jung [:ericjung]]

(In reply to comment #12)
> Ugh, sorry for giving you wrong direction at the summit Eric. the future of
> high vs low level modules was not as clear then.

You'd better polish your crystal ball :)
I'll work on a re-write.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Eric: are you still planning to work on this?

Comment 16

[Myk Melez [:myk] [@mykmelez]]

(automatic reprioritization of 1.0 bugs)

Comment 17

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

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 18

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

This appears to be working correctly on the current github master branch.

I used the following code:

let selection = require("selection");
selection.on("select", function ()
{
  selection.text = "converting";
});

I then cfx xpi'd it, then drag/drop'd the xpi file into a running Firefox session and chose to install it.

On the already active tab in that Firefox session, I selected some text, and the selection was converted to "converting".

This also works correctly on the SDK's stabilization branch. It is not working on the 1.12 release branch (which is also what Addon Builder has available).

I'd guess bug 803027 fixed this, then?