Text selection Actionbar generates JS error in browser.js

RESOLVED FIXED in Firefox 28

Status

()

Firefox for Android
Text Selection
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: capella, Assigned: Margaret)

Tracking

({regression})

unspecified
Firefox 28
ARM
Android
regression
Points:
---

Firefox Tracking Flags

(fennec28+)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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
(Assignee)

Updated

4 years ago
Blocks: 768667
tracking-fennec: --- → ?
Keywords: regression
tracking-fennec: ? → 28+
(Assignee)

Comment 1

4 years ago
I can look into this.
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 2

4 years ago
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).
(Assignee)

Comment 3

4 years ago
Created attachment 8341991 [details] [diff] [review]
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.
Attachment #8341991 - Flags: review?(wjohnston)
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.
Attachment #8341991 - Flags: review?(wjohnston) → review+
(Assignee)

Comment 5

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/14b3b255b2c8
https://hg.mozilla.org/mozilla-central/rev/14b3b255b2c8
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.