943466 - Text selection Actionbar generates JS error in browser.js

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

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:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?mark=6623-6623#6621

Comment 1

[:Margaret Leibovic]

I can look into this.

Comment 2

[:Margaret Leibovic]

This error is caused by the fact that aElement is null in the SEARCH_ADD selector:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6628

This happens because this._targetElement is null here in SelectionHandler._sendMessage:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#312

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).

Comment 3

[:Margaret Leibovic]

Attachment: patch

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.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 8341991 [details] [diff] [review]
patch

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.

Comment 5

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/14b3b255b2c8

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14b3b255b2c8