907271 - Fix startSelection() processing re: new text SelectionListener()

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

Attachment: ts

From mihaip, initially thought related to bug 903871, but actually regressed by bug 854589

The text selection state is changed too early in the process, and early exit in the routine causes us to try to remove a listener that hasn't been added yet.

Adding STR to another scenario:
1. Open http://www.businessinsider.com/4-billion-year-old-fossil-proteins-resurrected-2013-8
2. Long tap on the article text
3. Open another site in a different tab without closing the current tab
4. Long tap on text or a link

Expected result:
The selection handler or the context menu are displayed

Actual result:
At step 2 you can see in logcat a JS error, and the selection handler is not displayed
At step 4 The selection handler or the context menu are not displayed when long taping

Comment 1

[Mark Capella [:capella]]

Test build
https://tbpl.mozilla.org/?tree=Try&rev=56e435ed6c76

Comment 2

[Mark Capella [:capella]]

Attachment: bug907271

Comment 3

[:Margaret Leibovic]

Comment on attachment 793003 [details] [diff] [review]
bug907271

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ +204,5 @@
>      // Clear any existing selection from the document
>      this._contentWindow.getSelection().removeAllRanges();
>  
>      if (!this._domWinUtils.selectAtPoint(aX, aY, Ci.nsIDOMWindowUtils.SELECT_WORDNOSPACE)) {
>        this._closeSelection();

The problem with doing this is that the _closeSelection() calls in here won't work anymore, because _closeSelection() bails early if _activeType is TYPE_NONE.

I'm actually a bit confused about this patch... the _initTargetInfo() call up above adds the observers, so there shouldn't be an error if _closeSelection() is called. What is the JS error you're seeing? Could it be caused by something else?

Comment 4

[Mark Capella [:capella]]

When you go to the www.businessinsider.com page that has the user-select none, and long tap, we try to start a selection, complete the _initTargetInfo(), set the _activeType to TYPE_SELECTION then exit early due to no selectable text and _closeSelection() tries to remove the new selectionListener we haven't added yet in startSelection() ...

I forgot we'd still need to remove the observers and eventlisteners that _initTargetInfo() actually did add ...


Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISelectionPrivate.removeSelectionListener]" {file: "chrome://browser/content/SelectionHandler.js" line: 488}]

Error: "[Exception... "Index or size is negative or greater than the allowed amount"  code: "1" nsresult: "0x80530001 (IndexSizeError)"  location: "chrome://browser/content/SelectionHandler.js Line: 538"]" {file: "chrome://browser/content/SelectionHandler.js" line: 538}]

Comment 5

[Mark Capella [:capella]]

Or to be more precise, I forgot that _closeSelection() in the new case wouldn't do that.

Comment 6

[Mark Capella [:capella]]

Attachment: bug907271

How about dividing the _CloseSelection() function and making more targeted calls?

Comment 7

[Mark Capella [:capella]]

Attachment: bug907271

Rebased after fig -> m-c ... cleaner _closeSelection() ...

Comment 8

[:Margaret Leibovic]

Comment on attachment 793984 [details] [diff] [review]
bug907271

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

Apologies for the delay. I like where this is going, but I'd like for us to rename these new functions to make it clearer what they're doing. Let's also take this opportunity to add more documentation about what they do as well.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +484,3 @@
>  
> +      case this.TYPE_CURSOR:
> +        this._closeToNone();

I'd prefer if statements to this switch statement because I find that more readable. What do you think?

if (this._activeType == this.TYPE_NONE)
  return;

if (this._activeType == this.TYPE_SELECTION)
  this._clearSelection(); // see comment below

this._deactivate(); // see comment below

@@ +489,3 @@
>  
> +  _closeToCursor: function sh_closeToCursor() {
> +    this._activeType = this.TYPE_CURSOR;

This method doesn't quite do what it implies it does, since it doesn't send a message to update the text selection handles. But since you're only ever using it in _closeSelection right now, I don't think we should worry about that, and we can just focus on the case where it's used.

Given that, I think we should get rid of this line to set the _activeType and rename this function something like _clearSelection(), since that's what it's really doing.

@@ +498,5 @@
> +      selection.collapseToStart();
> +    }
> +  },
> +
> +  _closeToNone: function sh_closeToNone() {

I'd also like to rename this function to be more descriptive of what it's actually doing. How about _deactivate()?

Comment 9

[Mark Capella [:capella]]

I agree to everything :)

fyi, I was shying away from the early exit in _closeSelection() thinking that expected low call rates / potential speed savings, and a desire to target calls to the routine as required ... as-in (if this._activeType == this.TYPE_NONE), logically I'd avoid call _closeSelection() in the first place

( but I kvetch, this wfm  :-D )

Also, I originally had the if / else but then changed to case stmt thinking it was more javascripty  (-ish?)

Comment 10

[Mark Capella [:capella]]

Attachment: bug907271

So that's this version ...

I went with your _deactivate() ... curious, did you mean _deactivateSelection() ? I can always do another quick fixup  :-)

Comment 11

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #9)

> fyi, I was shying away from the early exit in _closeSelection() thinking
> that expected low call rates / potential speed savings, and a desire to
> target calls to the routine as required ... as-in (if this._activeType ==
> this.TYPE_NONE), logically I'd avoid call _closeSelection() in the first
> place

Yeah, I agree we shouldn't be calling _closeSelection() where there isn't already an active selection, so maybe we could get rid of this check, but I'd prefer to avoid any edge-casey regressions :)

(In reply to Mark Capella [:capella] from comment #10)

> I went with your _deactivate() ... curious, did you mean
> _deactivateSelection() ? I can always do another quick fixup  :-)

I like _deactivate more than _deactivateSelection, since it's more generic (this method is called for both selection and caret active types). The naming of things in this file is already a bit of a mess because the caret bit was added after the fact, but at least this can be a minor improvement :)

Comment 12

[Mark Capella [:capella]]

Let's give it a TRY:
https://tbpl.mozilla.org/?tree=Try&rev=7be63301291e

Comment 13

[Mark Capella [:capella]]

Onward and upward ;-)
https://hg.mozilla.org/integration/fx-team/rev/1536b018539c

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1536b018539c