Closed Bug 1133155 Opened 9 years ago Closed 9 years ago

Selection Handler closes leaving handles on screen

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(firefox36 unaffected, firefox37 affected, firefox38 affected, firefox39 verified)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox36 --- unaffected
firefox37 --- affected
firefox38 --- affected
firefox39 --- verified

People

(Reporter: capella, Assigned: capella)

Details

Attachments

(1 file, 1 obsolete file)

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  :-/
Attached patch bug1133155.diff (obsolete) — Splinter Review
This fixes it :-)
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #8564710 - Flags: review?(margaret.leibovic)
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 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).
Attachment #8564710 - Flags: review?(margaret.leibovic) → feedback+
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
Flags: needinfo?(ehsan)
I'm not really sure what information you're requesting from me.  Can you please clarify?
Flags: needinfo?(ehsan) → needinfo?(markcapella)
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 :-)
Flags: needinfo?(markcapella)
ni? for any final thoughts   -- mark
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
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?
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
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.
Attached patch bug1133155.diffSplinter Review
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.
Attachment #8564710 - Attachment is obsolete: true
Attachment #8572412 - Flags: review?(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.
Attachment #8572412 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/9f81eb113f46
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
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)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: