904288 - Incorrect display of text selection handles during Find In Page use

Status: RESOLVED DUPLICATE of bug 864589

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

Description

[Mark Capella [:capella]]

Attachment: bughandle

STR:

Open FF/Mobile
Select an article from arstechnica.com and open to view it
Longtap/select a common word such as |the|
Open the menu and select |Find in Page| to search for the target
Observe the target is found and highlit in green

Logtab/select a different target near the original one
Observe the new selection is hightlit in orange and has handles

Tap the down arrow in the (still open) FindBar
Observe the next occurence of the original target is found and highlit in green, while the handles from the second selection remain in place.
Subsequent taps of the up/down arrows don't affect the incorrect display of the handles


Easy fix, see attached

Comment 1

[:Margaret Leibovic]

Comment on attachment 789259 [details] [diff] [review]
bughandle

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

::: mobile/android/chrome/content/FindHelper.js
@@ +76,5 @@
>          this._targetTab.setViewport(JSON.parse(this._initialViewport));
>          this._targetTab.sendViewportUpdate();
>        }
>      } else {
> +      sendMessageToJava({ type: "TextSelection:HideHandles" });

I don't like this because it doesn't clean up the active selection. If we're going to go with this approach, this should be SelectionHandler._closeSelection(). However, I think this bug is actually just a symptom of bug 864589, so maybe we should investigate fixing that instead. I'm not immediately eager to jump to that conclusion though, since that bug is much harder to fix (but maybe you'd like a challenge!).

Comment 2

[Mark Capella [:capella]]

I actually went for a calling _closeSelection first but it wasn't working. It closes the handles but the |find| process was failing to locate and highlight in green.

I didn't see an obvious connection between the two functions to explain that, and decided on the simpler close handles solution, believing that the slection itself is already closed, and the handles are just displayed wrong.

I can look further here.

Comment 3

[Mark Capella [:capella]]

Attachment: bug904288

I'm not sure what bug 864589 is, so I'm shelving that thought for the moment. fyi I don't see this problem in v22, but I can duplicate it in v24.

The following patch fixes it closer to what we both wanted to do (gracefully finish the SelectionHandler._closeSelection() logic, that I had originally run into trouble with. (I found the conflict I mentioned in comment 2).

The tricky part is that the |Find| logic (where we locate / highlight in green the found text) depends on using the same nsISelection object on the page that we use for SelectionHandler (we talked about this in bug 903871 comment 9) ...

When we longTap / select a word, we run through the SelectionHandler.startSelection() stuff. If we then click on a find up/down button it does a find on the page for the text in the FindBar, and takes over the nsISelection object without affecting the selection handle display.

If I simply call into the top of the SelectionHandler._closeSelection() at that point, the logic that calls selection.removeAllRanges() actually removes the selection that the |Find| has just obtained and hightlit in green, effectively clearing it / removing it from the display (the bad part).

The rest of the _closeSelection() does all the good stuff for us including turning off the selection handles, which is what this bug is all about.

So my solution is two break _closeSelection() into two pieces as you'll see in the patch, and after the |Find| buttons are clicked up/down we call into SelectionHandler to execute all the good stuff except for the selection.removeAllRanges() block.

Comment 4

[Mark Capella [:capella]]

Comment on attachment 789699 [details] [diff] [review]
bug904288

Cancelling review for now ... I finally looked into bug 864589 and work there might tie into the solution here.

Comment 5

[Mark Capella [:capella]]

bug 864589 Solves this.