1133155 - Selection Handler closes leaving handles on screen

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Mark Capella [:capella]]

0) Navigate to:
   http://www.w3schools.com/html/tryit.asp?filename=tryhtml_formatting_b

1) Long press to selectWORD the word "bold" in the second <p> element
2) Observe the expected (word is selected, selection handles appear, actionbar opens, keyboard opens)
3) Tap the actionbar "check"/close icon in the upper left
4) Observe the actionbar close

5) Observe the selection and handles remain as they are, instead of closing.
   https://www.dropbox.com/s/fro2wri3gna28ug/bugLostSelNoteListener.png?dl=0


Selectionhandler.s is properly receiving a "TextSelection:End" from the actionbar in TextSelection.java, but failing in it's _clearSelection() routine when it tries to remove the SeelctionListener that it expects to exist !
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=9ef34ca40f79&mark=1064-1064#1056

Logging through nsSelection.cpp's Add/Remove/Notify SelectionListener() methods, I see that the listener is indeed being be removed, but *externally* to SelectionHandler.js.

This issue exists in Release all the way down to Nightly.

I'm assuming it's an oddness of this particular page, but I can't yet explain how it grabs a reference to SelectionHandler to basically beat us to the removeSelectionListener(this) request  :-/

Comment 1

[Mark Capella [:capella]]

Attachment: bug1133155.diff

This fixes it :-)

Comment 2

[Mark Capella [:capella]]

Note re: original comment above ... the listener isn't actually being removed ... the editor associated with the textarea seems to change on the fly, so I need to hold a ref to the original.

Comment 3

[:Margaret Leibovic]

Comment on attachment 8564710 [details] [diff] [review]
bug1133155.diff

Review of attachment 8564710 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good, I'm just a bit concerned about us failing to clear a reference in some cases.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +359,5 @@
>        return this.START_ERROR_NO_SELECTION;
>      }
>  
>      // Add a listener to end the selection if it's removed programatically
> +    this._selectionPrivate = selection.QueryInterface(Ci.nsISelectionPrivate);

Can you add a comment here explaining why we need to hold onto this _selectionPrivate reference?

@@ +1078,5 @@
>        // Remove our listener before we clear the selection
> +      try {
> +        this._selectionPrivate.removeSelectionListener(this);
> +      } catch (e) {
> +        throw "Failed to remove expected selection listener.";

If you're just going to throw another exception in this catch statement, I think it would make sense to just remove the try/catch altogether and let the original exception propagate.

@@ +1080,5 @@
> +        this._selectionPrivate.removeSelectionListener(this);
> +      } catch (e) {
> +        throw "Failed to remove expected selection listener.";
> +      }
> +      this._selectionPrivate = null;

We should probably do this somewhere else, so that we don't end up holding onto a reference if there is no selection when we get here. I would move this to _deactivate (or at least below this if statement).

Comment 4

[Mark Capella [:capella]]

note: I found this similar code in the existing base, so this isn't entirely unexpected to someone ...
http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js?rev=bcf39cf75424&mark=263-264,293-294#245

Think I'll ping ehsan on return from pto

Comment 5

[(no longer active)]

I'm not really sure what information you're requesting from me.  Can you please clarify?

Comment 6

[Mark Capella [:capella]]

Ah sorry, my issue is that in mobile SelectionHandler, to observe selection changes in editable elements, we normally add a listener:
element.QueryInterface(Ci.nsIDOMNSEditableElement).editor.selection.QueryInterface(Ci.nsISelectionPrivate).addSelectionListener(this);

later we remove the listener with:
element.QueryInterface(Ci.nsIDOMNSEditableElement).editor.selection.QueryInterface(Ci.nsISelectionPrivate).removeSelectionListener(this);

On nightly, this was appearing to be able to fail. Even though our reference to |element| wasn't changing, subsequent reference from |.editor.| in that chain was indeed changing. (STR case listed above).

I figured it was an oddness on that page, and the solution I planned was simply to save the entire resolved reference that the listener was added to. That fixed my problem of how to remove the listener, but didn't explain why it was necessary, or what selection changes I might not be observing as a result.

It seemed at the time that |editor| was "floating" in value, as logging was showing it changing, and sometimes reverting / re-establishing to the original value intermittently.

On re-testing just now before explaining to you, however, the bug seems to have "gone away". Since I can no longer repro, I'll probably WONT FIX this, and assume something on the core side got tweaked then corrected. Maybe even spend some time bisecting to see if I can find it :-)

Comment 7

[Mark Capella [:capella]]

ni? for any final thoughts   -- mark

Comment 8

[(no longer active)]

There is nothing to guarantee that the editor.selection object you call removeSelectionListener on is the same one that addSelectionListener was called on, as you have found out, so the change in your patch seems correct to me.  Even though the bug may not happen on a specific Web page, based on code inspection I think we still need to fix this.  The presence of the bug depends on what the Web page does.  For example, if the document's designMode attribute changes while the selection is shown, I expect the bug to occur.

Comment 9

[Mark Capella [:capella]]

Well, then the bigger issue I was considering, was that after the editor.selection object changes out from under me, I seem to stop receiving selectionChange notifications, even though as I mentioned, my reference to the original element remains unchanged. 

I can hack the SelectionHandler's entry points to notice the loss of the original editor.selection, and simply close the mobile selection UI, or perhaps better yet, remove and re-establish the listener and continue.

It's not a terrible hack, (just guarding four entry-point methods), but I wonder if there's an easier way to observe / be informed of the change?

Comment 10

[(no longer active)]

(In reply to Mark Capella [:capella] from comment #9)
> Well, then the bigger issue I was considering, was that after the
> editor.selection object changes out from under me, I seem to stop receiving
> selectionChange notifications, even though as I mentioned, my reference to
> the original element remains unchanged. 

Yes, that is because the new selection object doesn't have a listener associated with it, and the old object doesn't see any selection changes.

> I can hack the SelectionHandler's entry points to notice the loss of the
> original editor.selection, and simply close the mobile selection UI, or
> perhaps better yet, remove and re-establish the listener and continue.
> 
> It's not a terrible hack, (just guarding four entry-point methods), but I
> wonder if there's an easier way to observe / be informed of the change?

There is currently no way of knowing when a selection object changes like this.  We should really stop doing all of these hacks, and use the touch caret and selection handles that we have developed for b2g for Android.  We need to do some theming work to make sure they are painted with the correct native themes, but other than that they should mostly work fine now, and they'll be robust to these types of issues because they are managed by Gecko directly.

Comment 11

[Mark Capella [:capella]]

Awesome, thanks.

I've got an enhanced patch I'll post to this for margaret later. I've found we can target our detection of loss of editor.selection on reflow events, and this reduces the hackiness.

But, yah, I was looking at designMode and contentEditable and agreeing with you.

I think I'll dig into Bug 1065138 - Use Platform selection carets / Bug 988143 - Enable text selection and touch caret in Gecko on Fennec.

Comment 12

[Mark Capella [:capella]]

Attachment: bug1133155.diff

This catches any change in editor at reflow, and re-attaches our listener. I've been testing and this works with a minimum of fuss.

Comment 13

[:Margaret Leibovic]

Comment on attachment 8572412 [details] [diff] [review]
bug1133155.diff

Review of attachment 8572412 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Thanks for digging into this problem to find out what's really going on.

Comment 14

[Mark Capella [:capella]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16a62e05c347

Comment 15

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/9f81eb113f46

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f81eb113f46

Comment 17

[Teodora Vermesan (:TeoVermesan)]

The selection handles close after tapping  the "check" icon from actionbar, so:
Verified as fixed using:
Device: Nexus 4 (Android 4.4)
Build: Firefox for Android 39.0a1 (2015-03-11)