547092 - implement selection module for interacting with selection

Status: RESOLVED FIXED

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

Description

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

We should implement the selection module for interacting with the selection that is proposed in JEP 111.

Comment 1

[Eric Jung [:ericjung]]

Work being committed here: https://[e-mail]/ejung/jep-111-selection/

Comment 2

[Eric Jung [:ericjung]]

Attachment: release 0.1

1. I need help on how to correctly open a browser in test-selection.js. The current approach throws runtime exceptions which don't necessarily interfere with the tests, but nevertheless shouldn't be there. Please help.

2. I cannot figure out a way to write automated tests for onSelect. See two commented-out tests in test-selection.js. Any ideas?

3. I'm getting the following error when I run cfx testall against the cloned trunk:

  File "resource://testall-jetpack-core-lib/tab-browser.js", line 38, in 
    var errors = require("errors");

Even though the errors module exists. If I run this within my jetpack-sdk-61d81f5755b5 clone, it works fine. Any thoughts? Do you get the error, too?

Comment 3

[Eric Jung [:ericjung]]

Attachment: main.js for testing selection-0.1 interactively. cfx run -a firefox

After browser opens and loads tab, select text with mouse with 5 seconds to see selection change.

Comment 4

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

Attachment: release 0.1 of patch as plaintext file

This is "release 0.1" as a plaintext file for ease of reviewing.

Comment 5

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

Comment on attachment 439326 [details] [diff] [review]
release 0.1 of patch as plaintext file

Ok, I've reviewed the patch and identified the following issues...

selection.md:

> Gets or sets the current selection as html. Setting

Nit: html -> HTML (use all-caps when describing the type of the value of the "html" property via the acronym "HTML" and lower case when referencing the name of the property).


> <tt>selection.**onSelect**</tt>
> 
> Gets a singleton object with two functions for managing selection event notifications.
> 
> ### onSelect Functions ###
> 
> <tt>selection.onSelect.**add**(*callback*)</tt>
> 
> Adds *callback* to a list of callbacks which are called when text/html is selected.
> 
> <tt>selection.onSelect.**remove**(*callback*)</tt>
> 
> Removes *callback* from a list of callbacks which are called when text/html is selected.

onSelect is actually an event callback property that should behave a bit differently.  The add/remove methods are correct, but the common case for registering one or more callbacks is to simply set the property, i.e.:

  selection.onSelect = function() {};
  selection.onSelect = [function() {}, function() {}];

The best way to implement this is to use the collection module that Drew has created over in bug 559169.


> <tt>selection.**iterator**</tt>
> 
> Discontiguous selections can be accessed by iterating over <tt>selection</tt>. Each iteration
> instance returns a <tt>Selection</tt> object on which <tt>text</tt>, <tt>html</tt>, or
> <tt>contiguous<tt> may be called.

The title of this section is confusing, as it suggests that consumers will iterate over an "iterator" property. Perhaps it can be something like "Iterating Discontiguous Selections" instead?  I'm not yet very familiar with the Markdown format used to create these docs, so perhaps this suggestion is uninformed; if so, Atul'll have a better one when he reviews this code.


> Log the current discontiguous selections as hmtl:

hmtl -> HTML



selection.js:

> * Portions created by the Initial Developer are Copyright (C) 2007

2010!


>    set contiguous() { /* no-op */}

Omit the setter, as this property should be read-only, such that attempting to set it throws an exception, and that will happen automatically if the property has a getter but no setter.


>    let tmp = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);

Use the XPCOM utility function defineLazyServiceGetter to cache a global reference to this service so the context function doesn't have to cross the expensive XPCOM boundary each time it is called, i.e.:

  let xpcom = require("xpcom");
  xpcom.utils.defineLazyServiceGetter(this,
                                      "windowMediator",
                                      "@mozilla.org/appshell/window-mediator;1",
                                      "nsIWindowMediator");
  ...
  function context() {
    return (windowMediator.getMostRecentWindow("navigator:browser") ||


>    return (tmp.getMostRecentWindow("navigator:browser") ||
>        tmp.getMostRecentWindow("mail:3pane")).document.commandDispatcher.focusedWindow;

Use Atul's xul-app.js module from Atul's personal packages repository to determine which application is being run and branch accordingly.  (tab-browser also requires xul-app, so it's needed anyway.)  I have filed bug 559638 on reviewing and landing that module in jetpack-core.


> function setSelection(type, val, rangeNumber) {
> ...
>     try {
>       window = context();
>       range = window.getSelection().getRangeAt(rangeNumber);
>     }
>     catch(e) {
>       return undefined;
>     }

This should actually throw if there is no selection.


>    node["innerHTML"] = val;

Is there a reason to use the square brackets form for dereferencing a property here instead of the more conventional dot notation?  If so, it would be good to document it with a code comment, as it's not clear why it's being used.


>        // We only look for certain types of selection reasons

Can you expand this comment, explaining why the code only looks for these types of reasons?


>        this.listeners.forEach(function(listener) {
>            let safeCallback = function() {
>                try {
>                    callback();
>                }

callback() -> listener(), I think.


>            setTimeout(safeCallback, 0);

setTimeout needs to be imported from the timer module, as module contexts don't come with it by default.


These last two issues suggest the listening code isn't being exercised by the tests, and a glance at the test code confirms that.  We'll need to figure out how to fix that.  Would handling additional reasons in notifySelectionChanged help?  Alternately, perhaps you could synthesize DOM events that emulate user selection.



test-selection.js:

Include a license block at the top of this file.


            test.assertEqual(true, [DIV1, DIV2].some(function(t) t == i.html), "get multiple selection html works");

This is fine.  However, it reminds me to figure out and specify whether iteration is ordered.  If so, we'll want to change this test in the future to test that the discontiguous selections are returned in the appropriate order.  In the meantime, add a comment to that effect here.


        test.assertEqual(selection.html, "<span>" + REPLACEMENT_HTML + "</span>", "selection html works");

Is it possible to implement selection setting such that doing so doesn't wrap the selection in <span></span> tags?


        for (let i in selection) {

for...in happens to work here, but all these loops should actually be for each...in, since they iterate over values, not names.



tab-browser.js:

I haven't reviewed this thoroughly yet, but it should get reviewed and landed separately.  I have filed bug 559641 on that and made this one depend on that one.

Also, it looks like errors, window-utils, and unload-2 are required by tab-browser.  Fortunately, they've just landed per bug 556940.  However, unload-2's functionality has been merged with the unload module, so tab-browser will need to be updated accordingly.



Two general issues that apply to multiple files:

1. Most variable declarations use let, but a few use var unnecessarily.  They should all use let.
2. Indentation seems to be a mix of two- and four-space.  Use two-space indentation consistently.



Finally, when I tested this using main.js from the earlier version you emailed me a couple days ago, I noticed that making two discontiguous selections and then modifying main.js to change both of them didn't work, as only the first was changed.  And it looks like setting a selection unselects it, whereas it should replace the selection with its new value without unselecting it.

Comment 6

[Eric Jung [:ericjung]]

What is the deadline for this to be completed?

Comment 7

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

(In reply to comment #6)
> What is the deadline for this to be completed?

Ideally, it would land in time for 0.3, whose freeze has recently been extended from this evening to Tuesday, April 20 at 11:59pm PDT.  If it doesn't land by then, though, then it should land in time for 0.4, whose freeze is Tuesday, May 18 at 11:59pm PDT.

Comment 8

[Eric Jung [:ericjung]]

(In reply to comment #5)

> Would handling additional reasons in notifySelectionChanged
> help?  

According to my tests, no.

> Alternately, perhaps you could synthesize DOM events that emulate user
> selection.

I've tried a variety of ways to do this using the functions at http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js (For example: left mouse button down, page down keypress, then left mouse button up). I was not able to get text to be selected. AFAIK, the only approach I haven't tried yet is nsIDOMWindowUtils.sendSelectionSetEvent(), but that is Firefox 3.7+ only. Please let me know if you'd like me to try that. Otherwise, I'm at a loss of what to do next. Any other ideas?

>         test.assertEqual(selection.html, "<span>" + REPLACEMENT_HTML +
> "</span>", "selection html works");
> 
> Is it possible to implement selection setting such that doing so doesn't wrap
> the selection in <span></span> tags?

After a *lot* of testing, I've concluded that the answer is no--at least not with Javascript. There may be a [noscript] means to do it, but I haven't considered that. FWIW, the JetPack pre-reboot selection API used the same approach.

What *is* possible is to modify the currently selected text with something else. Consider the following HTML page:

<html><body><p>There is radiance and glory in the darkness would we but see it</p></body></html>

with the word "darkness" selected by the user. I can change "darkness" to new text. Unfortunately, changing the currently selected text to something else results in the selection being removed. So, in order to re-select the (new) text, Selection.extend() must be used. But selecting *only* that new text isn't possible with Selection.extend(). This is because extend() works by selecting from a DOM node onwards (i.e., the <p> node or the beginning of the #text node ("There...") which is a child of <p>).

let window = context();
let selection = window.getSelection();
let currentRange = selection.getRangeAt(rangeNumber);

let domNode = Ci.nsIDOMNode;
let newRange = window.document.createRange();
let newNode = newRange.createContextualFragment(val);
  let anchorNode = selection.anchorNode;
  if (anchorNode.nodeType == domNode.TEXT_NODE || anchorNode.nodeType == domNode.COMMENT_NODE ||
      anchorNode.nodeType == domNode.CDATA_SECTION_NODE) {
    // Get anchorNode's contents and replace the selected portion with newNode contents
    let startOffset = currentRange.startOffset, endOffset = currentRange.endOffset;
    let selectedContents = anchorNode.textContent.substring(startOffset, endOffset);
    anchorNode.textContent = anchorNode.textContent.substring(0, startOffset - 1) + newNode.textContent +
      anchorNode.textContent.substring(endOffset);
    // Selection is now collapsed. Select the new content.
    selection.extend(anchorNode, startOffset);
  }
  else {
  }
  if (newNode.nodeType == domNode.TEXT_NODE || newNode.nodeType == domNode.COMMENT_NODE ||
    newNode.nodeType == domNode.CDATA_SECTION_NODE) {
  }
  else {
  }

If there is another way to select text programmatically with Javascript, I wasn't able to find it. Please confirm/deny if it is OK to proceed with the <span/> approach used by the pre-reboot jetpack selection API. 

> Also, it looks like errors, window-utils, and unload-2 are required by
> tab-browser.  Fortunately, they've just landed per bug 556940.  However,
> unload-2's functionality has been merged with the unload module, so > > tab-browser will need to be updated accordingly.

If tab-browser has landed, then I don't need to include it in my patches for this bug anymore, right? And, if so, then I don't need to do any tab-browser updates since they presumably landed with the changes you mention?

> I noticed that making two discontiguous selections and
> then modifying main.js to change both of them didn't work, as only the first
> was changed.  

I didn't realize that is the expected behavior. I wrote it so that setting the selection when there are discontiguous selections only changes the first selection. Should it instead change *all* selections?

> And it looks like setting a selection unselects it, whereas it
> should replace the selection with its new value without unselecting it.

After a *lot* of testing, I don't think this is possible--at least not through Javascript. There may be a [noscript] means to do it, but I haven't considered that. FWIW, the JetPack pre-reboot selection API unselected first.

Comment 9

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

(In reply to comment #8)
> AFAIK, the only approach
> I haven't tried yet is nsIDOMWindowUtils.sendSelectionSetEvent(), but that is
> Firefox 3.7+ only. Please let me know if you'd like me to try that. Otherwise,
> I'm at a loss of what to do next. Any other ideas?

Yes, please try nsIDOMWindowUtils.sendSelectionSetEvent, as only being able to run it on 3.7+ is a reasonable fallback if we can't get anything working on 3.6.


> If there is another way to select text programmatically with Javascript, I
> wasn't able to find it. Please confirm/deny if it is OK to proceed with the
> <span/> approach used by the pre-reboot jetpack selection API. 

Yes, it's fine to proceed with the <span/> approach.


> > Also, it looks like errors, window-utils, and unload-2 are required by
> > tab-browser.  Fortunately, they've just landed per bug 556940.  However,
> > unload-2's functionality has been merged with the unload module, so > > tab-browser will need to be updated accordingly.
> 
> If tab-browser has landed, then I don't need to include it in my patches for
> this bug anymore, right? And, if so, then I don't need to do any tab-browser
> updates since they presumably landed with the changes you mention?

Yes, you no longer need to include it in the patches for this bug.  However, your changes to tab-browser have not yet landed.  They're being worked on over in bug 560467, although the specific implementation has changed, as Atul is implementing the functionality you need via a different path.


> > I noticed that making two discontiguous selections and
> > then modifying main.js to change both of them didn't work, as only the first
> > was changed.  
> 
> I didn't realize that is the expected behavior. I wrote it so that setting the
> selection when there are discontiguous selections only changes the first
> selection. Should it instead change *all* selections?

Setting the overall selection object should only change the first selection, but setting a selection object returned by the iterator should change that particular selection.


> > And it looks like setting a selection unselects it, whereas it
> > should replace the selection with its new value without unselecting it.
> 
> After a *lot* of testing, I don't think this is possible--at least not through
> Javascript. There may be a [noscript] means to do it, but I haven't considered
> that. FWIW, the JetPack pre-reboot selection API unselected first.

Ok, that's fine.  We can leave the behavior as-is, then.

Comment 10

[Dietrich Ayala (:dietrich)]

Hi Eric, do have an ETA for a new patch on this? We're <2 weeks out from code freeze for 0.4, so would like to make sure this lands as soon as possible.

Comment 11

[Eric Jung [:ericjung]]

I'm shooting for another patch this week.

Comment 12

[Dietrich Ayala (:dietrich)]

Hey again :) It's been a week. Do you think you'll have time to get a new patch and a review cycle before code freeze for 0.4 tomorrow night? Is there anything we can do to help move this forward?

(I've started on the tabs work, but I'm not clear on whether it's hard-blocking this or not)

Comment 13

[Eric Jung [:ericjung]]

(In reply to comment #5)

> > function setSelection(type, val, rangeNumber) {
> > ...
> >     try {
> >       window = context();
> >       range = window.getSelection().getRangeAt(rangeNumber);
> >     }
> >     catch(e) {
> >       return undefined;
> >     }
> 
> This should actually throw if there is no selection.

Are you sure that's what you want? The reason I ask is because the wiki at https://wiki.mozilla.org/Labs/Jetpack/Reboot/JEP/111 says

"If there is not a selection:
* getting either property returns undefined;
* setting either property has no effect."

"No effect" doesn't really say anything about the return value and/or exceptions, but shouldn't the getter and setter behave the same way when there's no selection? That was my reasoning when returning undefined for the setter. If we throw from the setter instead, shouldn't we throw from the getter, too (when there's no selection to get)?
 
> These last two issues suggest the listening code isn't being exercised by the
> tests, and a glance at the test code confirms that.  We'll need to figure out
> how to fix that.  Would handling additional reasons in notifySelectionChanged
> help?  Alternately, perhaps you could synthesize DOM events that emulate user
> selection.

As previously discussed, I'm now using nsIDOMWindowUtils.sendSelectionSetEvent() to do this (Firefox 3.7+ only). However, that method is returning false; i.e., it's failing, and I'm not sure why. Could you possibly take a look? New patches momentarily.

Comment 14

[Eric Jung [:ericjung]]

Attachment: 0.2

Requires http://hg.mozilla.org/labs/jetpack-sdk/ with this patch https://bug560467.bugzilla.mozilla.org/attachment.cgi?id=447623
However, that patch has a bug in timer.js, so after applying the patch, revert timer.js

Comment 15

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

Comment on attachment 447929 [details]
0.2

(In reply to comment #13)
> (In reply to comment #5)
> > This should actually throw if there is no selection.
> 
> Are you sure that's what you want? The reason I ask is because the wiki at
> https://wiki.mozilla.org/Labs/Jetpack/Reboot/JEP/111 says
> 
> "If there is not a selection:
> * getting either property returns undefined;
> * setting either property has no effect."
> 
> "No effect" doesn't really say anything about the return value and/or
> exceptions, but shouldn't the getter and setter behave the same way when
> there's no selection? That was my reasoning when returning undefined for the
> setter. If we throw from the setter instead, shouldn't we throw from the
> getter, too (when there's no selection to get)?

The getters shouldn't throw when there isn't a selection, because retrieving and testing the selection is how an API consumer determines whether or not there is a selection, and requiring consumers to catch an exception is unnecessary when it is possible to convey the lack of a selection with a return value (although now that I look at this, I realize that the return value should be "null", which is preferable to "undefined" in cases where a valid value is never "null", because the use of "undefined" to mean "this property has no value" can be conflated with the use of "undefined" to mean "this property is not defined").

The setters are a different situation, however.  For them, it's important to be able to convey information about whether or not the operation was successful, so consumers can react accordingly (try again, notify their users, etc.).  But it's not possible to do that using return values, because setters are called via assignment operators, and assignment operators ignore the return values of setters and instead return their right operands, i.e.:

  var foo = { set bar() { return 1 } };
  var baz = (foo.bar = 2);
  // baz is 2, not 1

So in order to convey information like "there is no selection, so we can't set it" (or "there is a selection, and we tried to set it, but we failed to do so", for that matter), the setters need to throw exceptions.


> As previously discussed, I'm now using
> nsIDOMWindowUtils.sendSelectionSetEvent() to do this (Firefox 3.7+ only).
> However, that method is returning false; i.e., it's failing, and I'm not sure
> why. Could you possibly take a look? New patches momentarily.

I started to look at this, but I'm blocked on that at the moment by this exception when I try to run the tests:

  Traceback (most recent call last):
    File "resource://jetpack-core-jetpack-core-lib/selection.js", line 220, in 
      let tabTracker = require("tabs").TabTracker(SelectionListenerManager);
    File "resource://jetpack-core-jetpack-core-lib/tabs.js", line 130, in TabTracker
      this._tracker = new Tracker(this);
    File "resource://jetpack-core-jetpack-core-lib/tabs.js", line 87, in Tracker
      this._windowTracker = new windowUtils.WindowTracker(this);
    File "resource://jetpack-core-jetpack-core-lib/window-utils.js", line 58, in WindowTracker
      this._regWindow(window);
    File "resource://jetpack-core-jetpack-core-lib/window-utils.js", line 81, in _regWindow
      this.delegate.onTrack(window);
    File "resource://jetpack-core-jetpack-core-lib/tabs.js", line 105, in onTrack
      this._delegate.onTrack(browser);
  TypeError: this._delegate.onTrack is not a function

I'm using the version of tabs.js you referenced (bug 560467, attachment 447623 [details] [diff] [review]), although I didn't have to revert timer.js, because the patch didn't make any changes to it, so perhaps that's actually a different tabs patch than the one with which you tested?



>diff --git a/packages/jetpack-core/docs/selection.md b/packages/jetpack-core/docs/selection.md

>+<tt>selection.**contiguous**</tt>
>+
>+Getter which returns <tt>true</tt> if the current selection is a single,
>+contiguous selection.  Returns <tt>false</tt> if there are one or more discrete

one or more -> two or more, no?


>+current selection, <tt>undefined</tt> is returned.  Discontiguous selections
>+can be created interactively with <tt>Ctrl+double mouse click</tt>.

Isn't it Ctrl+click-and-drag?


>+Log the current discontiguous selections as hmtl:

Nit: hmtl -> HTML



>diff --git a/packages/jetpack-core/lib/selection.js b/packages/jetpack-core/lib/selection.js

>+function Selection(rangeNumber) {
>+  this._rangeNumber = rangeNumber;
>+}
>+
>+Selection.prototype = {
>+
>+  _rangeNumber: 0,
>+
>+  get text() {
>+    return getSelection(TEXT, this._rangeNumber);
>+  },
>+
>+  set text(str) {
>+    setSelection(TEXT, str, this._rangeNumber);
>+  },
>+
>+  get html() {
>+    return getSelection(HTML, this._rangeNumber);
>+  },
>+
>+  set html(str) {
>+    setSelection(HTML, str, this._rangeNumber);
>+  },
>+
>+  get contiguous() {
>+    let sel = getSelection(DOM, this._rangeNumber);
>+    // It isn't enough to check that rangeCount is zero. If one or more ranges
>+    // are selected and then unselected, rangeCount is set to one, not zero.
>+    // Therefore, if rangeCount is one, we also check if the selection is
>+    // collapsed.
>+    if (sel.rangeCount == 0) {
>+      return undefined;
>+    }
>+    if (sel.rangeCount == 1) {
>+      if (safeGetRange(sel, 0).collapsed) {
>+        return undefined;
>+      }
>+      else return true;
>+    }
>+    return false;
>+  }
>+};

In order to hide the private rangeNumber argument from API consumers while still enabling Selection getters/setters to access it, the getters/setters should be defined as lexical closures (i.e. functions that have access to the scope in which they are defined even after the function call that creates the scope has returned) in the Selector constructor, i.e.:

  function Selection(rangeNumber) {
    this.__defineGetter__("text", function () getSelection(TEXT, rangeNumber));
    this.__defineSetter__("text", function (str) setSelection(TEXT, str, rangeNumber));
    ...
  }


>+function context() {
>+  // Overlay names should probably go into the xul-app module instead of here
>+  let overlayName = xulApp.is("Firefox") ? "navigator:browser" :
>+    xulApp.is("Thunderbird") ? "mail:3pane" : null;
>+  if (!overlayName) {
>+    console.log("Unknown Mozilla Application");
>+  }
>+  return windowMediator.getMostRecentWindow(overlayName).document.
>+    commandDispatcher.focusedWindow;
>+}

Have you tested Thunderbird and confirmed that the module works with it?  If not, it would be better to explicitly support only Firefox until we can test and confirm Thunderbird support (which doesn't have to happen before this patch gets checked in).

Also, this should check at the beginning of the module and throw if it's in an unsupported app, just like context-menu does, which for context-menu looks like this:

  if (!require("xul-app").is("Firefox")) {
    throw new Error([
      "The context-menu module currently supports only Firefox.  In the future ",
      "we would like it to support other applications, however.  Please see ",
      "https://bugzilla.mozilla.org/show_bug.cgi?id=560716 for more information."
    ].join(""));
  }


>+  // Get the selected content as the specified type
>+  if (type == DOM) {
>+    return selection;
>+  }

This is a great idea and would be a welcome addition to the API!  However, after we integrate electrolysis (e10s), addon code is going to run in a separate process from the process(es) in which pages are loaded, and cross-process object passing is complicated and limited.

For modules that can't avoid it (widget, page worker, page mods, panel, perhaps context menu), we're going to have to revise the APIs (and their implementations) significantly in order to deal with the implications of e10s.  For this API, I think we should avoid that complexity by only supporting access to plain text and strings of serialized HTML, at least in the initial version of the API.


>+  /** nsISelectionListener implementation */
>+  notifySelectionChanged: function notifySelectionChanged(document, selection,
>+  reason) {

Nit: line up this parameter with the ones on the line above it, i.e.:

  notifySelectionChanged: function notifySelectionChanged(document, selection,
                                                          reason) {


>+/*********************** exports *************************/
>+
>+exports.selection = new Selection(0);
>+exports.selection.__iterator__ = function __iterator__() {
>+  let sel = getSelection(DOM);
>+  for (let i = 0; i < sel.rangeCount; i++) {
>+    yield new Selection(i);
>+  }
>+};
>+
>+require("collection").addCollectionProperty(exports.selection, "onSelect",
>+  SelectionListenerManager.listeners);
>+

Over the course of the 0.4 cycle, we standardized on exporting singletons such that require("module-name") returns the singleton rather than returning an object with a property that contains the singleton.  For this API, that means it should be possible to do:

  let selection = require("selection");
  console.log(selection.text);

instead of:

  let selection = require("selection").selection;
  console.log(selection.text);

However, this can't just do:

  exports = new Selection(0);

because the "exports" object is owned by the harness, and assigning it to a different object is not guaranteed to work in this fashion.  So instead, this needs to do (in conjunction with defining getters/setters as lexical closures in the constructor, as described earlier in this review):

  Selection.call(exports, 0);

Then the Selection constructor will define the getters/setters on the "exports" object directly.

The custom iterator and onSelect property also need to be defined on the "exports" object directly, but those are simple changes:

  exports.__iterator__ = function __iterator__() { ... }
  require("collection").addCollectionProperty(exports, "onSelect",
    SelectionListenerManager.listeners);


Finally, it would be preferable for the code to follow the Jetpack <https://wiki.mozilla.org/Labs/Jetpack/Reboot/Style_Guide> and Mozilla <https://developer.mozilla.org/En/Developer_Guide/Coding_Style> code style guidelines (with the former taking precedence over the latter where they conflict).

Comment 16

[Eric Jung [:ericjung]]

(In reply to comment #15)
> value (although now that I look at this, I realize that the return value
> should be "null", which is preferable to "undefined" in cases where a valid
> value is never "null"

I've changed the getters to return "null" instead of "undefined".

> make any changes to it, so perhaps that's actually a different tabs patch than
> the one with which you tested?

v3 now uses the tip + attachment 448121 [details] [diff] [review]. Please try again?

> >+current selection, <tt>undefined</tt> is returned.  Discontiguous selections
> >+can be created interactively with <tt>Ctrl+double mouse click</tt>.
> 
> Isn't it Ctrl+click-and-drag?

That appears to work as well. On my platform, double-clicking a word selects it. I can then hold the Ctrl key and double-click another word to create a discontiguous selection. Anyway, I've changed the docs to your method.

> >+  // Get the selected content as the specified type
> >+  if (type == DOM) {
> >+    return selection;
> >+  }
> 
> This is a great idea and would be a welcome addition to the API!  However,
> after we integrate electrolysis (e10s), addon code is going to run in a
> separate process from the process(es) in which pages are loaded, and
> cross-process object passing is complicated and limited.

This wasn't intended to add to the API. The |DOM| constant is only used internally and not intended for export. I've documented that in the source and hopefully you'll approve its internal use.

> Finally, it would be preferable for the code to follow the Jetpack
> <https://wiki.mozilla.org/Labs/Jetpack/Reboot/Style_Guide> and Mozilla
> <https://developer.mozilla.org/En/Developer_Guide/Coding_Style> code style
> guidelines (with the former taking precedence over the latter where they
> conflict).

I've spent a lot more time on v3 with formatting, code style, documentation, comments, proper spacing, whitespacing, braces (or no), etc. I hope there are now few if any style problems.

Comment 17

[Eric Jung [:ericjung]]

Attachment: v3

Requires tip of http://hg.mozilla.org/labs/jetpack-sdk/ and attachment 448121 [details] [diff] [review]. Please note that testOnSelect() and testOnSelectExceptionNoBubble() still fail due to the failure of nsIDOMWindowUtils.sendSelectionSetEvent() to select

Comment 18

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

Comment on attachment 448442 [details] [diff] [review]
v3

(In reply to comment #16)
> v3 now uses the tip + attachment 448121 [details] [diff] [review]. Please try again?

Yup, the tests run and work now (except for the one that is known to be failing).


> > This is a great idea and would be a welcome addition to the API!  However,
> > after we integrate electrolysis (e10s), addon code is going to run in a
> > separate process from the process(es) in which pages are loaded, and
> > cross-process object passing is complicated and limited.
> 
> This wasn't intended to add to the API. The |DOM| constant is only used
> internally and not intended for export. I've documented that in the source and
> hopefully you'll approve its internal use.

Ah, right, sorry, I missed that.  Yes, it's perfectly fine to use internally.


>diff --git a/packages/jetpack-core/docs/selection.md b/packages/jetpack-core/docs/selection.md

>+    let selection = require("selection").selection; if (selection.text)
>+      console.log(selection.text);
>+
>+Log the current discontiguous selections as HTML:
>+
>+    let selection = require("selection").selection; if (!selection.contiguous)
>+    {
>+      for each (let subselection in selection) {
>+         console.log(subselection.html);
>+      }
>+    }

Nit: make the |if| statements start on a new line, make the second one's opening brace cuddle it, and leave out braces around the single-line |for| block, i.e.:

    let selection = require("selection").selection;
    if (selection.text)
      console.log(selection.text);
...
    let selection = require("selection").selection;
    if (!selection.contiguous) {
      for each (let subselection in selection)
         console.log(subselection.html);
    }


Also, it was recently suggested to me that code examples for addon developers should use |var| rather |let| in order to be more familiar to developers with a web development background.  I think that's a reasonable policy (although we should continue to use |let| in implementation code), and these should use var.


>diff --git a/packages/jetpack-core/lib/selection.js b/packages/jetpack-core/lib/selection.js

>+  this.__defineGetter__("contiguous", function () {
>+    let sel = getSelection(DOM, rangeNumber);
>+    // It isn't enough to check that rangeCount is zero. If one or more ranges
>+    // are selected and then unselected, rangeCount is set to one, not zero.
>+    // Therefore, if rangeCount is one, we also check if the selection is
>+    // collapsed.
>+    if (sel.rangeCount == 0)
>+      return null;
>+    if (sel.rangeCount == 1) {
>+      if (safeGetRange(sel, 0).collapsed)
>+        return null;
>+      else
>+        return true;
>+    }

If there's a single range, but its toString() result is the empty string (or retrieving it throws an exception), then safeGetRange() will return null, in which case |safeGetRange(sel, 0).collapsed| will throw "null has no properties".  To avoid that, this needs to do:

    if (sel.rangeCount == 1) {
      let range = safeGetRange(sel, 0);
      if (range && range.collapsed)
        return null;
      else
        return true;
    }


>+  throw new Error("Type " + type + "is unrecognized.");

Nit: "is -> " is


>+      throw e; // TODO: should this throw a new error or reuse |e| ? if we
>+               // reuse |e|, we don't even need a try/catch block here.

The emerging best practice is to throw new Error("a more developer-friendly error message than the ones typically thrown by XPCOM components").


>+        try {
>+          listener();
>+        }
>+        catch (e) {
>+          // Catch exceptions so that other listeners, if any, are still called.
>+          console.exception(e);
>+        }

Nit: this is best done via:

  require("errors").catchAndLog(function () listener())();


>+  onTrack: function(tab) {
>+    let selection = tab.ownerDocument.defaultView.getSelection();
>+    if ((selection instanceof Ci.nsISelectionPrivate))
>+      selection.addSelectionListener(SelectionListenerManager);
>+  },

This is tracking selection in chrome windows.  In order to track selections in web pages, it needs to add listeners to every page after it's loaded (which is quite unfortunate, but as far as I can tell there is no better API).  Here's a code snippet showing the necessary modifications (note: this uses Tracker rather than TabTracker):

http://pastebin.mozilla.org/730700


>diff --git a/packages/jetpack-core/tests/test-selection.js b/packages/jetpack-core/tests/test-selection.js

>+function sendSelectionSetEvent(window) {
>+  const Ci = Components.interfaces;
>+  let utils = window.QueryInterface(Ci.nsIInterfaceRequestor).
>+                                    getInterface(Ci.nsIDOMWindowUtils);
>+  if (!utils.sendSelectionSetEvent(0, 1, false))
>+    dump("**** sendSelectionSetEvent did not select anything\n");
>+  else
>+    dump("**** sendSelectionSetEvent succeeded\n");
>+}
>+
>+// testOnSelect() requires nsIDOMWindowUtils, which is only available in
>+// Firefox 3.7+.
>+exports.testOnSelect = function testOnSelect(test) {
>+  let callbackCount = 0;
>+  primeTestCase(TEXT_SINGLE, test, function(window, test) {
>+    selection.onSelect = function() {callbackCount++};
>+    // Now simulate the user selecting stuff
>+    sendSelectionSetEvent(window);
>+    selection.text = REPLACEMENT_TEXT;
>+    test.assertEqual(1, callbackCount, "onSelect text listener works.");
>+    //test.pass();
>+    //test.done();
>+  });
>+
>+  test.waitUntilDone(5000);
>+};

I've done some digging, but I can't figure out why this is failing either.  In theory we should be able to do something similar to the test selection expanding test <http://mxr.mozilla.org/mozilla-central/source/layout/generic/test/test_selection_expanding.html>, but that's what you tried originally, and it didn't work either.

At this point I think we should punt on this and make the test do |test.pass("needs a reliable way to trigger selection")|, then file a bug on the test failing and look for someone more familiar with the event system to figure this out.

Comment 19

[Eric Jung [:ericjung]]

(In reply to comment #18)
> Nit: make the |if| statements start on a new line, make the second one's
> opening brace cuddle it, and leave out braces around the single-line |for|
> block, i.e.:

Sorry, that was the result of my automated end-of-line whitespace stripper that didn't work 100%.

> http://pastebin.mozilla.org/730700
>
> function onLoad(event) {
>   SelectionListenerManager.onLoad(event);
> }

> function onUnload(event) {
>   SelectionListenerManager.onUnload(event);
> }

Are these two functions really needed? Can't I instead write:

onTrack: function onTrack(browser) {
  // TODO: is this.onLoad and this.onUnload more appropriate?
  browser.addEventListener("load", SelectionListenerManager.onLoad, true);
  browser.addEventListener("unload", SelectionListenerManager.onUnload, true);
},

and

onUntrack: function onUntrack(browser) {
  // TODO: is this.onLoad and this.onUnload more appropriate?
  browser.removeEventListener("load", SelectionListenerManager.onLoad, true);
  browser.removeEventListener("unload", SelectionListenerManager.onUnload, true);
}

> onLoad: function onLoad(event) {
>   // Nothing to do without a useful window
>   let window = event.target.defaultView;
>   if (window == null)
>     return;

Nit: I'd prefer to write:

  if (!window)
    return;

is that OK? Ditto for:

> onUnload: function onUnload(event) {
>   // Nothing to do without a useful window
>   let window = event.target.defaultView;
>   if (window == null)
>     return;

Comment 20

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

(In reply to comment #19)
> Are these two functions really needed? Can't I instead write:
> 
> onTrack: function onTrack(browser) {
>   // TODO: is this.onLoad and this.onUnload more appropriate?
>   browser.addEventListener("load", SelectionListenerManager.onLoad, true);
>   browser.addEventListener("unload", SelectionListenerManager.onUnload, true);
> },
> 
> and
> 
> onUntrack: function onUntrack(browser) {
>   // TODO: is this.onLoad and this.onUnload more appropriate?
>   browser.removeEventListener("load", SelectionListenerManager.onLoad, true);
>   browser.removeEventListener("unload", SelectionListenerManager.onUnload,
> true);
> }

You can do that as long as onLoad and onUnload don't refer to |this|, since |this| will not be SelectionListenerManager when you register those functions as event listeners in this manner.

Since SelectionListenerManager is a singleton defined in the global scope, you can replace references to |this| in those functions with SelectionListenerManager.

Just keep in mind that there is potential for confusion during future changes to this code, as the functions are defined as properties of an object, so they appear as if they are going to be invoked as methods, with |this| being SelectionListenerManager inside them, whereas in fact they are invoked as functions, and |this| is the global object inside them.


> > onLoad: function onLoad(event) {
> >   // Nothing to do without a useful window
> >   let window = event.target.defaultView;
> >   if (window == null)
> >     return;
> 
> Nit: I'd prefer to write:
> 
>   if (!window)
>     return;
> 
> is that OK?

Yes, in fact it's better!

Comment 21

[Eric Jung [:ericjung]]

Attachment: 0.4

1. I've commented out the two tests which make use of Ci.nsIDOMWindowUtils.sendSelectionSetEvent(). Let me know if you'd rather they be removed entirely.

2. Test testSetContiguous() results in this error, even though I'm using a try/catch block:

(firefox-bin:8193): GLib-WARNING **: g_set_prgname() called multiple times
....console: [JavaScript Warning: "setting a property that has only a getter" {file: "file:///home/fidel/dev/jetpack-head/jetpack-sdk/packages/jetpack-core/lib/securable-module.js -> resource://jetpack-core-jetpack-core-tests/test-selection.js" line: 139}]

Since tests that fail shouldn't be checked in, do you know how to fix this, or should I remove it? It's not obvious to me why the try/catch isn't working.

Comment 22

[Eric Jung [:ericjung]]

(In reply to comment #21)
> 2. Test testSetContiguous() results in this error, 

FYI, the error is expected and anticipated; it's just that I can't get the test harness to respond properly

Comment 23

[Eric Jung [:ericjung]]

Attachment: main.js for testing selection-0.4 interactively.

1. Copy main.js into jetpack-sdk/packages/jetpack-core/lib
2. cd to jetpack-sdk/packages/jetpack-core/lib
3. cfx run -a firefox
4. Page will load. Wait 5 seconds minimum, even if page loads before 5 secds
5. Select something on page. Look for the text of what you've selected in stdout
6. For some reason, non-contiguous selections aren't dumping the correct output to stdout. This is probably a bug in the code which needs to be addressed.

Comment 24

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

Comment on attachment 450944 [details] [diff] [review]
0.4

> 1. I've commented out the two tests which make use of
> Ci.nsIDOMWindowUtils.sendSelectionSetEvent(). Let me know if you'd rather they
> be removed entirely.

Nope, the commenting out is great.  And zpao has said he can take a look at this, so I'm going to point him at it.


> 2. Test testSetContiguous() results in this error, even though I'm using a
> try/catch block:
> 
> (firefox-bin:8193): GLib-WARNING **: g_set_prgname() called multiple times
> ....console: [JavaScript Warning: "setting a property that has only a getter"
> {file:
> "file:///home/fidel/dev/jetpack-head/jetpack-sdk/packages/jetpack-core/lib/securable-module.js
> -> resource://jetpack-core-jetpack-core-tests/test-selection.js" line: 139}]
> 
> Since tests that fail shouldn't be checked in, do you know how to fix this, or
> should I remove it? It's not obvious to me why the try/catch isn't working.

I think it isn't working because the code isn't throwing an exception, it's just generating a warning (which is what JS does when you try to set a property that only has a getter, for some reason).  

The test harness has an assertRaises, but it doesn't have an assertWarns, and I don't know any way to catch a warning, so comment out this test as well, and I'll find someone to look at it.



>+    var selection = require("selection").selection;
>+    if (selection.text)
>+      console.log(selection.text);

Here and elsewhere, require("selection").selection -> require("selection")


>+      for each (var subselection in selection) {
>+         console.log(subselection.html);
>+      }

Nit: I previously recommended "for each...in" statements when iterating an object with a custom iterator, but rereading the docs on custom iterators reveals that "for...in" statements are actually more appropriate ("for each...in" seems to be a special case iteration statement for arrays), so make this:

      for (var subselection in selection)
         console.log(subselection.html);


>+      // Rethrow with a more developer-friendly message than the caught
>+      // exception.
>+      throw new Error("");

This should throw an actual message, f.e.: It isn't possible to change the selection, as there isn't currently a selection."


>+        // Catch exceptions so that other listeners, if any, are still called.
>+        require("errors").catchAndLog(function() listener())();

Erm, I just realized that we need to set the listener's |this| object to avoid exposing the global state, i.e.:

        require("errors").catchAndLog(function() listener.call(exports))();


> 6. For some reason, non-contiguous selections aren't dumping the correct output
> to stdout. This is probably a bug in the code which needs to be addressed.

It's actually because the program uses a "for" statement to iterate the selection, but "for" statements don't work with custom iterators.  To iterate the non-contiguous selection, do:

  for (let sel in selection)
    dump("selected :" + sel.text + "\n");


Finally, nsIDOMWindow::getSelection seems to be causing a leak.  As far as I can tell, it has nothing to do with this code, although it affects test results.  I'm not sure what to do about this.

Comment 25

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

(In reply to comment #24)
> Finally, nsIDOMWindow::getSelection seems to be causing a leak.  As far as I
> can tell, it has nothing to do with this code, although it affects test
> results.  I'm not sure what to do about this.

Hmm, although I can't duplicate by calling that method in the tab-browser tests, so maybe it is related to this code.

Comment 26

[Drew Willcoxon :adw]

(In reply to comment #24)
> Finally, nsIDOMWindow::getSelection seems to be causing a leak.  As far as I
> can tell, it has nothing to do with this code, although it affects test
> results.  I'm not sure what to do about this.

I haven't been following this bug, but looking briefly at the test it appears that primeTestCase() opens a tab but doesn't close it.  That's a problem for a couple of reasons: 1) tests should always clean up after themselves, and 2) there's some funnybusiness with windows, tabs, Jetpack, and memory leaks.  See bug 566351 for details, but the upshot is if you close the new tab after every test, the leaks (might) go away.

Comment 27

[Eric Jung [:ericjung]]

Attachment: v5

Comment 28

[Eric Jung [:ericjung]]

Attachment: main.js for testing selection-0.5 interactively.

1. Copy main.js into jetpack-sdk/packages/jetpack-core/lib
2. cd to jetpack-sdk/packages/jetpack-core/lib
3. cfx run -a firefox
4. Page will load. Wait 5 seconds minimum, even if page loads before 5 secds
5. Select something on page. Look for the text of what you've selected in
stdout

Comment 29

[Dietrich Ayala (:dietrich)]

I'm not seeing any obvious leak cause. Closing tabs like Drew mentioned would be good for reducing leak potential.

I'm a bit concerned about the super long timeouts. See the tests in my patch on bug 549317 for approaches to making the window/tab tests more deterministic. Not something we need to fix in these tests now, but if we start having problems with random failures we should remove arbitrary setTimeout stuff first.

Comment 30

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

Note: the main.js manual test requires the following change for testing discontiguous selections:

        for (let sel in selection)
-          dump("selected :" + selection.text + "\n");
+          dump("selected :" + sel.text + "\n");

(Otherwise, you see the first range repeated as many times as there is a selection rather than seeing each individual range.)

Comment 31

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

Comment on attachment 451450 [details] [diff] [review]
v5

Looks great, and the tests no longer leak now that you're closing the tabs. r=myk!

Comment 32

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

Landed as changeset https://hg.mozilla.org/labs/jetpack-sdk/rev/faded6fd8d7b.

Comment 33

[Dietrich Ayala (:dietrich)]

Attachment: tests followup

Comment 34

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

Comment on attachment 451519 [details] [diff] [review]
tests followup

>+    test.pass("the selection module does not support this application.");

Nit: the -> The

Good catch, r=myk!

Comment 35

[Dietrich Ayala (:dietrich)]

fixed, pushed: http://hg.mozilla.org/labs/jetpack-sdk/rev/b459207c5fbf

Comment 36

[Eric Jung [:ericjung]]

Attachment: main.js for testing selection-0.5 interactively.

1. Copy main.js into jetpack-sdk/packages/jetpack-core/lib
2. cd to jetpack-sdk/packages/jetpack-core/lib
3. cfx run -a firefox
4. Page will load. Wait 5 seconds minimum, even if page loads before 5 secds
5. Select something on page. Look for the text of what you've selected in
stdout

Comment 37

[Eric Jung [:ericjung]]

One thing that bothers me:

1. Run the interactive test (main.js)
2. Wait 5 seconds then select something on the page with double-click of the left mouse button. You'll see the selection in stdout.
3. Hold the cntl key and single-click the left mouse button on another word. You'll see both the original selection and null in stdout.

This pushes the liability of null checking onto selection observer.

Comment 38

[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".