Closed Bug 943466 Opened 6 years ago Closed 6 years ago
Text selection Actionbar generates JS error in browser
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: 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).
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+
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.