1060579 - nsISelectionPrivate doesn't fire selection listener event when manipulating contenteditable from JS

Status: RESOLVED WONTFIX

(Core :: DOM: Selection, defect)

Description

[Jan Jongboom [:janjongboom] (inactive)]

We have a selection listener in forms.js (https://github.com/mozilla/gecko-dev/blob/master/dom/inputmethod/forms.js#L287). This works fine on contenteditable elements when moving the cursor around. However... There is no event fired when manipulating the innerHTML of the contenteditable from content process where the cursor position actually changes. E.g.:

<div id="text" contenteditable>Some text</div>

Then set the cursor to end of the div w/ JavaScript:

  var t = document.querySelector('#text');

  t.focus();
  var range = document.createRange();
  range.selectNodeContents(t);
  range.collapse(false);
  var selection = window.getSelection();
  selection.removeAllRanges();
  selection.addRange(range);

This works fine, I get notifySelectionChanged. Now we clear out the contenteditable via:

  setTimeout(function() {
    var t = document.getElementById('text');
    t.innerHTML = '';
  }, 20);

And now I don't get any event :-(

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

Ehsan, you were one of the last ones to touch the interface, you know which component this should be under?

Comment 2

[(no longer active)]

Why do you expect to get a selection change event in the test case in comment 0?  The selection is not changed when you set the innerHTML to an empty string...

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #2)
> Why do you expect to get a selection change event in the test case in
> comment 0?  The selection is not changed when you set the innerHTML to an
> empty string...

It is, the cursor was at position [9, 9], and after the innerHTML change it is at [0, 0].

Comment 5

[(no longer active)]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #3)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #2)
> > Why do you expect to get a selection change event in the test case in
> > comment 0?  The selection is not changed when you set the innerHTML to an
> > empty string...
> 
> It is, the cursor was at position [9, 9], and after the innerHTML change it
> is at [0, 0].

When I said the selection is not modified, I meant its ranges are not changed.  It is entirely possible for a selection's ranges to be changed under its nose when the content within those ranges mutates, which is what you are seeing here.  We have never dispatched any kind of selection change notification for that case AFAIK.

Comment 6

[(no longer active)]

So, I think what we can do here is to extend nsRange::SetInSelection to take a mozilla::dom::Selection* pointer, store that within the range so that the range knows what selection it is associated with, and then inside the nsRange mutation observers, notify the selection object of a change to the range if it is in a selection.

This worries me a little bit since that this changes the semantics of the selection listeners, by dispatching events on them where we would not before.  I think that is fine though.  Olli, what do you think?

Comment 7

[(no longer active)]

Or, we could expose these notifications through a new API if that is better...

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

I think comment 6 should be fine, especially if we notify selection listeners using script runners.

Comment 9

[Mats Palmgren (inactive)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> So, I think what we can do here is to extend nsRange::SetInSelection to take
> a mozilla::dom::Selection* pointer, store that within the range so that the
> range knows what selection it is associated with, and then inside the
> nsRange mutation observers, notify the selection object of a change to the
> range if it is in a selection.

I fear that this would be complex, brittle and slow.

Comment 10

[Mats Palmgren (inactive)]

If the use case is to observe DOM mutations under a single node -
why not use a MutationObserver?
https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Mats Palmgren (:mats) from comment #10)
> If the use case is to observe DOM mutations under a single node -
> why not use a MutationObserver?
> https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver

Because I think that would be the wrong way to fix this. We can create exceptions and handlers for everything like we had in the past, but it's going to break or have weird edge cases or something will break. That's why we abandoned our home grown solution for nsISelectionPrivate instead in bug 1026997. If we can't rely on this API to give us correct events whenever the cursor position changes I believe it's a bug.

Comment 12

[(no longer active)]

(In reply to Mats Palmgren (:mats) from comment #9)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #6)
> > So, I think what we can do here is to extend nsRange::SetInSelection to take
> > a mozilla::dom::Selection* pointer, store that within the range so that the
> > range knows what selection it is associated with, and then inside the
> > nsRange mutation observers, notify the selection object of a change to the
> > range if it is in a selection.
> 
> I fear that this would be complex, brittle and slow.

Can you please clarify?

Comment 13

[Mats Palmgren (inactive)]

The nsRange mutation handlers[1] are already complex and brittle and
I'm worried that adding code to handle additional cases will make
them substantially worse.  Note that currently we can ignore pretty
much all mutations that doesn't involve the start/end nodes of the
Range.  That won't be the case if we need to detect mutations that
"modifies the selection" (as I understand that concept).
Say we have a Range in the Selection from a:0 to c:0 in:
 <a>...<b>...<c>
then we currently ignore any mutations under <b> for example since it
doesn't affect the Range itself although the selection as the user
percieves it will likely change.  Detecting and notifying such changes
will make the code slower (and I supect it means *all* code will be
slower, since the UA likely has a Selection listener already so we
*will* notify all such mutations if they occur inside the selection).

IIUC, the use case in this bug only needs to observe mutations under
a single node and there is a simple solution for that already:

  var observer = new MutationObserver(function(mutations) {
        // update your selection dependent stuff here
      });
  });
  observer.observe(document.querySelector('#text'),
                   { childList: true, 
                     subtree: true, 
                     characterData: true });

I don't think complicated changes to nsRange are motivated if the
above serves the use case here - and if it doesn't, then let's
discuss what exactly is needed and how we can solve that.

[1]
http://hg.mozilla.org/mozilla-central/annotate/f27ff178807d/content/base/src/nsRange.cpp#l388

Comment 14

[Mats Palmgren (inactive)]

To clarify the markup I meant a,b,c to be subtrees, something like this:
<a>...</a>...<b>...</b>...<c>...</c>

Comment 15

[(no longer active)]

(In reply to Mats Palmgren (:mats) from comment #13)
> I don't think complicated changes to nsRange are motivated if the
> above serves the use case here - and if it doesn't, then let's
> discuss what exactly is needed and how we can solve that.

Needinfoing Jan.

Comment 16

[Jan Jongboom [:janjongboom] (inactive)]

pinging :yxl as well. You think this is acceptable solution?

Comment 17

[Yuan Xulei [:yxl] (Leave from May 18)]

I tend to use a content mutation observer instead of changing the selection listener.

It doesn't change the semantics of the selection listeners and isn't the nsIEditorObserver[1] we already have able to act as content mutation observer:

https://github.com/mozilla/gecko-dev/blob/45f4500903123d7142ca43fcedf81e1388fececf/dom/inputmethod/forms.js#L332

Comment 18

[Jan Jongboom [:janjongboom] (inactive)]

Alright, taking it.

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

Alright, so closing this as WONTFIX and moving the proposed fix into bug 1059163