612676 - consider alternate Selection API

Status: RESOLVED WONTFIX

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

Description

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

We can't guarantee that the selection won't change between successive calls to the Selection API.  An alternate API that would mitigate the problem would be to make the API expose a getSelection() method that returns an object representing the current selection and then disable that object when the selection changes.

For example:

  let selection = require("selection").getSelection();

  // do some work, perhaps asynchronously

  // This only works if the selection hasn't changed in the meantime.
  selection.text = "a new value";

The "select" event would similarly pass its handlers an object representing the current selection.

We should consider changing the Selection API to work in this way.

Comment 1

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

Currently tab-browser is only used by selection API tests. Would be nice to remove this dependency so that we can remove tab-browser.

Comment 2

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

(In reply to comment #0)
> We can't guarantee that the selection won't change between successive calls to
> the Selection API.  An alternate API that would mitigate the problem would be
> to make the API expose a getSelection() method that returns an object
> representing the current selection and then disable that object when the
> selection changes.
> 
> For example:
> 
>   let selection = require("selection").getSelection();
> 
>   // do some work, perhaps asynchronously
> 
>   // This only works if the selection hasn't changed in the meantime.
>   selection.text = "a new value";
> 
> The "select" event would similarly pass its handlers an object representing the
> current selection.
> 
> We should consider changing the Selection API to work in this 

Assuming we have following scenario:

let selection = require("selection");
let s1 = selection.getSelection();
.....
// user changed selection and s1 becomes obsolete 
let s2 = selection.getSelection();

I have some questions:

1. What does access to s1.text / s1.html returns ?
(Deitrich mentioned it should throw, I believe it would be very ugly to require try / catch property accesses, neither it's something one would expect) I would assume null or undefined will be returned.
2. What happens if you try to set s1.text or s1.html. From my understanding of the proposal assignment will be just ignored. Dietrich suggested that it will throw, which is fine way to notify users, but I would than add some additional property like  `obsolete` for instance if (!s1.obsolete) s1.text = 'bla'. Also if (s1.text) s1.text = 'bla' may work. I personally think we should just ignore because we can't guarantee that selection have not changed on the other process.
3. What is going to happen with s2.html if user changes s2.text ? I guess one would assume that s2.html will automatically update itself but still with E10S it will be async in best case. This may be very confusing as well in scenario like this:

let selection = require("selection").getSelection();
selection.text = .....
let changedHTML = selection.html // this will be out of date information.

I guess we can throw in this case to prevent bugs, but it's ugly :(

4. And the final one. When user does selection.text = ... that will emit 'select' event in next turn of event loop. One would assume that object passed to a listener will be the same selection object but I don't really see a way selection can be associated. If we do create new selection object is previous one becomes obsolete ?

All this being said I'm not sure that new API is better.

Comment 3

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

(In reply to comment #2)
> All this being said I'm not sure that new API is better.

Yup, you make good points, and this does seem more and more like an ambiguous win.  So let's stick with the current API.