Closed Bug 943466 Opened 8 years ago Closed 8 years ago

Text selection Actionbar generates JS error in browser.js


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

Not set



Firefox 28
Tracking Status
fennec 28+ ---


(Reporter: capella, Assigned: Margaret)



(Keywords: regression)


(1 file)

If I use the actionbar to 'select All' text in an <input> field, then use it to 'Cut', I get a javascript message generated into logcat:

Error: "TypeError: aElement is null" {file: "chrome://browser/content/browser.js" line: 6598}]

This is currently pointing here:
Blocks: 768667
tracking-fennec: --- → ?
Keywords: regression
tracking-fennec: ? → 28+
I can look into this.
Assignee: nobody → margaret.leibovic
This error is caused by the fact that aElement is null in the SEARCH_ADD selector:

This happens because this._targetElement is null here in SelectionHandler._sendMessage:

This happens because the cut actions calls copySelection, which calls _closeSelection, which clears the state of the SelectionHandler, but then _updateMenu expects there to be an active selection.

However, even if we get rid of the _closeSelection call in copySelection (which I think makes sense), notifySelectionChanged will close the selection for us.

This feels like a mess :/ I think we need to audit the places we close the selection vs. keeping it active. For example, I see we're calling _closeSelection in the share action (although we're also calling _closeSelection inside shareSelection itself).
Attached patch patchSplinter Review
This fixes some issues with copy and cut.

As I mentioned in my last comment, copySelection closes the selection. Since after performing a cut, we want a caret to appear when the text got removed, we should actually just be calling attachCaret, to set up SelectionHandler with TYPE_CURSOR.

And since copySelection closes the selection, we shouldn't call _updateMenu afterwards in the COPY action. It just throws an error.

We also don't need to call _closeSelection in the SHARE action, since shareSelection also takes care of that for us.
Attachment #8341991 - Flags: review?(wjohnston)
Comment on attachment 8341991 [details] [diff] [review]

Review of attachment 8341991 [details] [diff] [review]:

I'm not sure who should own these closeSelection calls, but I obviously went overboard with them :) Thanks.
Attachment #8341991 - Flags: review?(wjohnston) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.