If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

onSelect doesn't trigger for already open tabs

RESOLVED FIXED

Status

Add-on SDK
General
P2
normal
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Tiberiu C. Turbureanu, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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.
Eric, can you check this?
Version: unspecified → Trunk
confirmed
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

7 years ago
Assignee: nobody → eric.jung
Created attachment 471600 [details] [diff] [review]
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?
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.
(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.
(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.
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.
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?
How do we get Atul's attention?
Comment on attachment 471600 [details] [diff] [review]
first fix attempt

How about we ask him to review this patch?
Attachment #471600 - Flags: review?(avarma)

Comment 11

7 years ago
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.)
Attachment #471600 - Flags: review?(avarma) → review-
Ugh, sorry for giving you wrong direction at the summit Eric. the future of high vs low level modules was not as clear then.
(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.
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".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
Eric: are you still planning to work on this?
Priority: -- → P2
Target Milestone: --- → 1.0
(automatic reprioritization of 1.0 bugs)
Priority: P2 → P1
Target Milestone: 1.0 → 1.1
Assignee: eric.jung → nobody
Priority: P1 → P2
Target Milestone: 1.1 → 1.2
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: 1.2 → ---
Blocks: 821779
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?
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.