Closed Bug 907271 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: Text Selection, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: capella, Assigned: capella)

Details

Attachments

(1 file, 4 obsolete files)

Attached patch ts (obsolete) — Splinter Review
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
Attached patch bug907271 (obsolete) — Splinter Review
Attachment #792914 - Attachment is obsolete: true
Attachment #793003 - Flags: review?(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?
Attachment #793003 - Flags: review?(margaret.leibovic) → review-
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}]
Or to be more precise, I forgot that _closeSelection() in the new case wouldn't do that.
Attached patch bug907271 (obsolete) — Splinter Review
How about dividing the _CloseSelection() function and making more targeted calls?
Attachment #793003 - Attachment is obsolete: true
Attachment #793301 - Flags: feedback?(margaret.leibovic)
Attached patch bug907271 (obsolete) — Splinter Review
Rebased after fig -> m-c ... cleaner _closeSelection() ...
Attachment #793301 - Attachment is obsolete: true
Attachment #793301 - Flags: feedback?(margaret.leibovic)
Attachment #793984 - Flags: review?(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()?
Attachment #793984 - Flags: review?(margaret.leibovic) → feedback+
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?)
Attached patch bug907271Splinter Review
So that's this version ...

I went with your _deactivate() ... curious, did you mean _deactivateSelection() ? I can always do another quick fixup  :-)
Attachment #793984 - Attachment is obsolete: true
Attachment #796410 - Flags: review?(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 :)
Attachment #796410 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/1536b018539c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.