947284 - End text-selection handle is not visible when opting to select all text

Status: RESOLVED FIXED

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

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (12/06) - Screenshot

See test-case, long-tap on a portion of the selected text like 'test-case' (see both selection handles appear). Now tap the 'select all' button in the action-bar. See the beginning handle appear in-front of the first character, see no end handle appear at all. We do get an entire selection of the text, but there is no end handle.

See screenshot.

--
Nightly (12/06) | LG Nexus 4 (Android 4.4)

Comment 1

[Mark Capella [:capella]]

quickglance ...

I can't see it in your screenshot, but when I test, the right handle is actually displayed, but all the way to the rightmost side of the screen ... on my GS3 I can just barely see it as-is, on my N7 I have to slide the screen over ...

This repros in release channel

Comment 2

[Aaron Train [:aaronmt]]

Oh yeah it's way way out there to the far right, I see it.

Comment 3

[Mark Capella [:capella]]

Attachment: bug947284 (v0)

This tightens up SelectionHandler in a couple ways, as well as fixs the basic annoying thing-that-was-wrong.

There's one annoying thing I'm still tracking down, but this is the basic direction, so I thought I'd give you time to look it over.

Comment 4

[Mark Capella [:capella]]

And while we're on the subject, can you elaborate on this:

  _moveSelection: function sh_moveSelection(aIsStartHandle, aX, aY) {
    // XXX We should be smarter about the coordinates we pass to caretPositionFromPoint, especially
    // in editable targets. We should factor out the logic that's currently in _sendMouseEvents.

It helps me focus here when I remember I want to rename that private method _sendMouseEvents() to instead be _clickToPoint();

Comment 5

[Mark Capella [:capella]]

Attachment: bug947284p1 (v0)

I've been using these objects a lot lately and have seen other situations where "debug" objects are checked in for handy use later ... optional tools, bonus points (?)

Comment 6

[:Margaret Leibovic]

Comment on attachment 8345732 [details] [diff] [review]
bug947284p1 (v0)

Review of attachment 8345732 [details] [diff] [review]:
-----------------------------------------------------------------

You can get all of this information using the remote debugger, so I think it's better to just use that, rather than inserting logging to inspect the state of different variables. Let me know if you need help getting the remote debugger set up, but it's really straightforward, and it's a really powerful tool! Just choose "Main Process", then you can open up SelectionHandler.js to add breakpoints. For more info, see: https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging/Firefox_for_Android

(Sadly bug 946813 broke this on Nightly, but hopefully that will be resolved soon, soon enough that I still don't think we should land this patch.)

Comment 7

[Mark Capella [:capella]]

Attachment: bug947284 (v1)

I'm reposting this patch with clarification in the code where this bug 947284 is corrected. I'm also pointing out in the code where bug 945976 was corrected as a result ...

They are basically part of the same problem, so I'd like to keep them here in patch. It's fairly small and I believe defendable / testable.

This bug:
    testcase for bug 947284, selectAll method for #text in a <p> (HTMLPreElement)
    data:text/plain,this%20is%20a%20testcase

That other bug:
    testcase for bug 945976, "User Selected All" while tapped into <textarea> in IFRAME
    https://www.dropbox.com/s/w33y57n5cqjuou7/bug945976TextCase.html

Comment 8

[:Margaret Leibovic]

Comment on attachment 8347448 [details] [diff] [review]
bug947284 (v1)

Review of attachment 8347448 [details] [diff] [review]:
-----------------------------------------------------------------

The logic in startSelection is continuing to grow more gnarly with this patch, I think we need to take a step back and make this logic easier to follow. In particular, I want us to be explicit about whether we're starting a selection to select a word vs. select all. I think it's fine to get rid of the selectAll method, but maybe we can break out some of startSelection into helper methods here.

Also, as I reported on IRC, I'm still seeing this disappearing handle issue at some zoom levels, so there's still some more figuring out to do here.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +249,5 @@
> +      if (aElement instanceof HTMLPreElement) {
> +        Cu.reportError("SelectionHandler: fixs this bug 947284, selectAll method for #text in a <p> (HTMLPreElement)");
> +        if (!this._domWinUtils.selectAtPoint(aX, aY, Ci.nsIDOMWindowUtils.SELECT_PARAGRAPH)) {
> +          Cu.reportError("SelectionHandler: Failed to selectAll in the requested HTMLPreElement");
> +          return false;

We should make sure to still call this._deactivate() in here before bailing, since this is after we called this._initTargetInfo.

@@ +264,1 @@
>        return false;

Same thing here.

@@ -284,5 @@
> -    // Do not select text far away from where the user clicked
> -    if (distance > maxSelectionDistance) {
> -      this._closeSelection();
> -      return false;
> -    }

Why are you removing all of this logic here?

@@ +506,5 @@
>      // in editable targets. We should factor out the logic that's currently in _sendMouseEvents.
>      let viewOffset = this._getViewOffset();
>      let caretPos = this._contentWindow.document.caretPositionFromPoint(aX - viewOffset.x, aY - viewOffset.y);
> +    if (!caretPos) {
> +      return;

When we run into this case? Should we be logging an error here?

::: mobile/android/chrome/content/browser.js
@@ +6233,5 @@
>      SelectionHandler.startSelection(aElement, aX, aY);
>    },
>  
> +  selectAll: function(aElement) {
> +    SelectionHandler.startSelection(aElement);

I don't like this overloading of startSelection to sometimes select all and sometimes not. If we're going to do this, I think we should change the method signature to make it explicit that we're selecting all (I generally don't like boolean flags, but I would forgive one here).

Comment 9

[Mark Capella [:capella]]

startSelection() and attachCaret() are basically the common entry points into the SelectionHandler ... with _closeSelection() being the common exit point (unless we determine internally that _deactivate() is appropriate.

The addition of selectAll() as en entry point seems one-too-many so I was looking for a way to simplify ...

startSelection() already does double-duty ... defaulting to "selectAllText()" in aElement, unless a point coordinate is provided, then we try to selectAllWord() in the element.

I'd changed the routine a little to use the midoint of the selected text element to avoid doing the proximity check, as I don't understand the requirement in the first place for such a check, and it seems to interfere with selecting all text in a large container.

Rethinking there, using the upper (left/top)+1 might be the better initial position, especially if we can still lose the check.

If it's easier for this patch, I don't think changing either of those now is technically required.

|if (!caretPos)| isn't an error, just normal while moving handles around into invalid areas (offscreen etc), I can add a comment there.

You're right re: needing a couple extra this._deactivate() during startSelection() "failures" ...

I'd like to entertain the thought of encapsulating this._cache into something like
let Handle = {
   start : { foo stuff }
}

Comment 10

[Mark Capella [:capella]]

Also, yah to "startSelection into helper methods here" for a trend ...

re; |as I reported on IRC, I'm still seeing this disappearing handle issue at some zoom levels|

I'm confused, as this is what this patch with the url provided above, demonstrably fixs for me on both a N7 and GS3 ... Not sure why an N4 would be any different ?

Comment 11

[Mark Capella [:capella]]

Attachment: bug947284

Ok, here's the next set of revisions. I've having a hard time separating priorities, so I've pointed out ones for you to see also.

I'm also planning to look further at our fine-point tuning, noticing some of our handle positions are not accurately placed during text zoom.

Killing little bugs is fun, but it might be easier if I could offer you a longer plan of attack  :-)

Comment 12

[Mark Capella [:capella]]

Attachment: bugProvideGetSelectionStatus

If you had time to read the first draft, bonus points! If not, this might be some easier reading. This is the first patch I plan to use in patches 2, 3, and possibly the 4th one I don't have working yet.

These three combine to kill a lot of little nastys along the way, some demonstrable to the user, others more a source of logcat noise. I've added comments as much as possible to clarify error conditions avoided, and general comments to provoke feedback / questions.

Comment 13

[Mark Capella [:capella]]

Attachment: bugSelectAllInTextarea

Uses the method established in the first patch ...

Fixes a bug with selecting all via the new action bar ... not sure there's a bug reported yet.

Involves code in browser.js was making a determination of whether of not an element had "select all" ability, and we were not in agreement, so it failed.

I'm using this testcase, the iframe on the left
https://www.dropbox.com/s/n39cs5mxx91upwj/OLD%20bug943944.html
If you tape into the field and get a selection caret, then you press the select all button, the actionbar just closes.

Apply patch 1 and 2, and this works.

Comment 14

[Mark Capella [:capella]]

Attachment: bugCleanupHandleMovePosition

I ran into so many leaks and excpetions using your test page
http://people.mozilla.org/~mleibovic/test/text_select.html
and just monkey testing while watching the log led me to this patch.

Advantages include moving handles off-screen more cleanly, and more accurrately reflecting the displayed state of the selection, using that first routine to help with state switching.

Comment 15

[:Margaret Leibovic]

Comment on attachment 8347961 [details] [diff] [review]
bugProvideGetSelectionStatus

Review of attachment 8347961 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good (nice job with comments explaining things), but I'd like to see another version that simplifies some of the if/else logic (minimizing the number of nested if/else statements we need as much as possible).

::: mobile/android/chrome/content/SelectionHandler.js
@@ +491,5 @@
> +   */
> +  _getSelectionStatus: function sh_getSelectionStatus() {
> +    // Do we still have a selection of some sort?
> +    let selection = this._getSelection();
> +    if (selection != null) {

To avoid some extra indentation, we could just return this.TYPE_NONE if the selection is null.

@@ +492,5 @@
> +  _getSelectionStatus: function sh_getSelectionStatus() {
> +    // Do we still have a selection of some sort?
> +    let selection = this._getSelection();
> +    if (selection != null) {
> +      if (selection.rangeCount > 0) {

It also looks like we always return this.TYPE_NONE if this condition isn't met. So you could just start out with

if (selection == null || selection.rangeCount == 0) {
  return this.TYPE_NONE;
}

@@ +498,5 @@
> +        let rect = range.getBoundingClientRect();
> +
> +        // If we have a range that's not collapsed to a caret
> +        if (!range.collapsed) {
> +            if (rect.width != 0 || rect.height != 0) {

Oops, looks like a 4-space indentation got in here accidentally.

@@ +503,5 @@
> +              // There is still an active selection, something is "orange"
> +              return this.TYPE_SELECTION;
> +            } else {
> +              // Completely closed, not even a blinking caret, no "orange stuff" visible to user
> +              return this.TYPE_NONE;

General comment: If you're returning in an if statement, you don't need an else statement. So the code would look like this:

if (...) {
  return ...;
}
return ...;

Comment 16

[:Margaret Leibovic]

Comment on attachment 8347969 [details] [diff] [review]
bugSelectAllInTextarea

Review of attachment 8347969 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for breaking these changes into smaller patches. This also looks like it's going in the right direction, but I don't like the duplication of code, and it looks like you may have accidentally left out some logic to select all in the case where the element isn't a textarea.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +252,5 @@
> +        // selectAll() for all other elements 
> +        let rect = this._targetElement.getBoundingClientRect();
> +        aX = rect.left + 1;
> +        aY = rect.top  + 1;
> +      }

This is a repeat of the same logic up above. I think we should only have one copy of this logic, so let's move it out above the |if ... instanceof Ci.nsIDOMHTMLTextAreaElement| check. We can set a boolean above that to let us know whether or not we want to select all (default to false, but set it to true if we see that aX/aY are undefined).

However, it doesn't look like you're actually doing anything to select all for elements that aren't textareas. Is this a mistake?

I'm worried that using an undefined aX/aY to signal that we want to do a "select all" isn't being very explicit, but I also don't love the idea of needing to pass in some "selectAll" boolean flag as a parameter, so I think it's fine to stick with this for now.

@@ +257,5 @@
> +
> +      // Use selectWORD for all other elements, from element and supplied or determined x/y point
> +      if (!this._domWinUtils.selectAtPoint(aX, aY, Ci.nsIDOMWindowUtils.SELECT_WORDNOSPACE)) {
> +        this._deactivate();
> +        return false;

This also duplicates logic up above. Can we reorganize this code so that we don't need to duplicate this?

@@ +281,5 @@
>      let positions = this._getHandlePositions(scroll);
>      let clickX = scroll.X + aX;
>      let clickY = scroll.Y + aY;
> +
> +    // I still don't think we need this check ...

So file a bug about it, and put the bug number in here!

Comment 17

[:Margaret Leibovic]

Comment on attachment 8347969 [details] [diff] [review]
bugSelectAllInTextarea

Oh, and you should also update this patch (or write another patch) to remove SelectionHandler.selectAll, and just use startSelection instead.

Comment 18

[:Margaret Leibovic]

Comment on attachment 8347980 [details] [diff] [review]
bugCleanupHandleMovePosition

Review of attachment 8347980 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review, since I want some answers to questions before continuing to review this.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +125,5 @@
>        case "TextSelection:Position": {
>          if (this._activeType == this.TYPE_SELECTION) {
> +          // Review current selection status after on-the-fly handle positioning
> +          // If overall status needs to change, do so and exit, else finish processing below
> +          let newSelectionStatus = this._getSelectionStatus();

I'm nervous about doing this... this._getSelectionStatus calls getBoundingClientRect on the range, which triggers reflows. Since the "TextSelection:Position" message get sent a lot as the user drags the handles, we need to be sensitive to performance here.

@@ +157,5 @@
>            this._ignoreCompositionChanges = false;
> +          this._positionHandles();
> +
> +        } else {
> +          // Do nothing

You don't need to have an else statement if there's nothing that goes in it :)

Also, why did you move the this._positionHandles() calls? That doesn't change any behavior here.

@@ +248,5 @@
>  
>      // Clear any existing selection from the document
>      this._contentWindow.getSelection().removeAllRanges();
>  
> +    // <textarea's want to be special

You should put these comments in the patch that actually changes this part of the code, not in this patch.

@@ +497,5 @@
>        return this._targetElement.QueryInterface(Ci.nsIDOMNSEditableElement).editor.selection;
> +    } else {
> +      // this._contentWindow might be null when user drops selection handle offscreen
> +      // (when observing "TextSelection:Position" request)
> +      return (this._contentWindow) ? this._contentWindow.getSelection() : null;

Can you elaborate on how this._contentWindow ends up being null here? I believe this._contentWindow should always be defined if we have an active selection, and I don't think we should be calling this._getSelection when selection isn't active. If we are, I'm curious what codepath is getting us in this state.

@@ +615,5 @@
> +    // Constrain text selection within editable elements.
> +    let targetIsEditable = this._targetElement instanceof Ci.nsIDOMNSEditableElement;
> +    if (targetIsEditable && (caretPos.offsetNode != this._targetElement)) {
> +      return;
> +    }

Why did you move this down below the logic to update the cache?

@@ +631,5 @@
>          selection.collapse(caretPos.offsetNode, caretPos.offset);
>          selection.extend(focusNode, focusOffset);
> +      } catch (e) {
> +        // Collapse/Extend towards the desired range, take partial results if we get them
> +        // Cu.reportError("XYZZY non-text selection move/drag partially successful");

When do we hit this case? I think we should report exceptions if they're not expected.

Comment 19

[Mark Capella [:capella]]

Thanks so much for all that!

fyi, I created the functional patches working backwards from a final solution, so I may have qref'd illogically in between bugSelectAllInTextarea and bugCleanupHandleMovePosition. I'll straighten that out and repost all patches in the next go-around.

Also, for all the three patches (so far) I've been posting the un-optimized versions on purpose. That's how I had them laid out, so I had quick access to all logical test points for debugging and (yes even...) logging...

I'll certainly incorporate all your suggestions to flatten things further having spent some time getting more comfortable with the leak-profiness of the code as I've layered things up.

re: |this._getSelectionStatus calls getBoundingClientRect on the range, which triggers reflows| is the terribly interesting TIL moment for me ... I didn't know all of the DOM beahoviour in detail, but I swore I was seeing something like that happening ... unexpected DOM changes in respose to things I thought were of the the read-only nature.

The fourth patch (not yet posted - titled bugSelectAllInPRE as a working name) that I'm working towards, is where I actually fix this title bug.

This is where, selectallis being clicked / requested where the document contains |date:text/plain, this is a test|, and the end part of the handle jumps to end-of-viewport instead of wrapping the "orangy" text.

I was trying to range.setEndBefore(node) during each selectionChange notification as a way of adjusting selection on the fly for that condition (where the range endContainer node was HTML or BODY).

I was expecting and seeing selectionChangeNotifications generated back to me as a result, and was ignoring them properly during the phase, but then I was seeming to lose reference to |this._contentWindow| !!!

This is where I can't imagine changing a selection object range might affect the base DOM it's tracking?

So if you get a chance to clarify / "say more things" there, it's welcome  :-)

I've gotten so far as to pull out https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver

(I want to observe this a little deeper. as I continue)

Comment 20

[Mark Capella [:capella]]

Attachment: bProviderNew

This and the next patch refine my train of thought on future SelectionHandler fixs, as well as fixing the bug mentioned in comment 13, which I now believe to be the same bug wesj is fixing in bug 945976. Doh!

Comment 21

[Mark Capella [:capella]]

Attachment: bRefactorStartSelection

First was the provider, here's the consumer.

Comment 22

[Mark Capella [:capella]]

Sorry, wes's bug is 950464. bug 945976 I referred to is still fixed here, but one assigned to me when I wasn't looking

o_O

Comment 23

[Mark Capella [:capella]]

Re: |this._getSelectionStatus calls getBoundingClientRect on the range, which triggers reflows. Since the
"TextSelection:Position" message get sent a lot as the user drags the handles.|

>>> Cool, I'll try to note instances of it's use. Doesn't a DOM reflow also invalidate the selection / range / etc object tied to the targetElement ?



Re:|
@@ +157,5 @@
>            this._ignoreCompositionChanges = false;
> +          this._positionHandles();
> +
> +        } else {
> +          // Do nothing
You don't need to have an else statement if there's nothing that goes in it . Also, why did you move the this._positionHandles() calls? That doesn't change any behavior here.|

>>> Changed to protect the null case ... if we observe a TextSelection:Position message while we are in an invalid state, we do nothing by design.



re:|
@@ +497,5 @@
>        return this._targetElement.QueryInterface(Ci.nsIDOMNSEditableElement).editor.selection;
> +    } else {
> +      // this._contentWindow might be null when user drops selection handle offscreen
> +      // (when observing "TextSelection:Position" request)
> +      return (this._contentWindow) ? this._contentWindow.getSelection() : null;
Can you elaborate on how this._contentWindow ends up being null here? I believe
this._contentWindow should always be defined if we have an active selection,
and I don't think we should be calling this._getSelection when selection isn't
active. If we are, I'm curious what codepath is getting us in this state.

>>> Can't explain, not entirely not yet, /me thinks I have a clue.



re:|
@@ +615,5 @@
> +    // Constrain text selection within editable elements.
> +    let targetIsEditable = this._targetElement instanceof Ci.nsIDOMNSEditableElement;
> +    if (targetIsEditable && (caretPos.offsetNode != this._targetElement)) {
> +      return;
> +    }
Why did you move this down below the logic to update the cache?

>>> has to do with a possible invalid caretPos().


@@ +631,5 @@
>          selection.collapse(caretPos.offsetNode, caretPos.offset);
>          selection.extend(focusNode, focusOffset);
> +      } catch (e) {
> +        // Collapse/Extend towards the desired range, take partial results if we get them
> +        // Cu.reportError("XYZZY non-text selection move/drag partially successful");
When do we hit this case? I think we should report exceptions if they're not
expected.

>>> This was bizarre behavior if I saw the whole thing right, and since I commented it I must have. But the STR (iirc) ar as follow ...
I think you can observe logcat spew without the patch

0) Use the textSelection testcase link you provided me with all the boxs, textareas, etc.
1) Type in a string of chars in an editable and select all in the field
2) Drag the right handle back so you've got partial text selected up to that point ...
around  has a selection range, then moves the
3) Drag the caret up out of the editable

4a) At that point you should see some text still highlighted in orange from before where the handle left the field.
4b) You should also see a logcat failure as the entend() did it's job then crashed (?) in the middle.


It seems we want to let it fail ... in order to maintain the "orange" portion it accomplishes before that.
Then we still have a valid nsISelection object, and it's associated nsIRange, nsINode, and etc. in as complete s state as possible.

That's my theory and I'm sticking to it.

Comment 24

[Mark Capella [:capella]]

Comment on attachment 8348525 [details] [diff] [review]
bProviderNew

Invalidated by recent code changes.

Comment 25

[Mark Capella [:capella]]

Comment on attachment 8348526 [details] [diff] [review]
bRefactorStartSelection

Also invalidated :)

Comment 26

[:Margaret Leibovic]

I think we should take a step back here and try to figure out why this handle is being positioned so far to the right, since that's really the problem here.

Are we getting incorrect coordinates in _updateCacheForSelection? If so, why? I'm guessing this is some edge case with the fact that this testcase is just a text document. If so, perhaps we should loop in some platform folks to see if this is actually a platform bug.

Has anyone ever seen this happen in the wild on a normal webpage? If not, I don't think we should spend too much energy focusing on it if there are other more pressing bugs that need fixing.

Comment 27

[Mark Capella [:capella]]

Attachment: bugEOL

It's because we're defaulting to a select all method when handed an HTMLPreElement, which winds up selecting all the way from the start of the HTMLHtmlElement, to the end of the document HTMLBodyElement.

If we simply SELECT_PARAGRAPH for this type of node instead of the default "entire document" |_getSelectionController().selectAll();| kinda-thing, then this works.

Comment 28

[:Margaret Leibovic]

Comment on attachment 8350917 [details] [diff] [review]
bugEOL

Review of attachment 8350917 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I also like the refactoring to break up startSelection a bit.

Comment 29

[Mark Capella [:capella]]

push to try https://tbpl.mozilla.org/?tree=Try&rev=6e67e4a48c1d

Comment 30

[Mark Capella [:capella]]

an on to fx-team: https://hg.mozilla.org/integration/fx-team/rev/9f2b115ff15a

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9f2b115ff15a

Comment 32

[Aaron Train [:aaronmt]]

This is still reproducible on 01/08 using the exact steps I outlined in comment #0. I see one handle at the beginning the test-case and the other is far far right after scrolling.

Comment 33

[Mark Capella [:capella]]

Ah! For my testing I've been consistently zooming into the page to make selecting the text easier.

If I leave the screen zoomed out / tiny, and attempt the initial long-tap WORD to start the selection at the first word in the test text string, the bug re-appears as minor variation on the original.

Where the original patch corrects for selecting ALL text in an element presented to SelectionHandler.js as an HTMLPreElement, the zoomed out code is failing due to being presented an HTMLHtmlElement for the same (user visible) text string.

If I tweak the patch that landed back a day to include checking for an instanceof HTMLPreElement || HTMLHtmlElement, then the new incarnation of this bug is also corrected.

Which leads me to wonder (before yet doing any actual digging) why this routine ever needs to default to selection method |this._getSelectionController().selectAll();|

(Can we avoid adding one more DOM element to the logic, then maybe another, etc)

Comment 34

[Mark Capella [:capella]]

Attachment: bug947284 Comment 32 Fix

I'll follow with a TRY build

Comment 35

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Try&rev=0008450b6556

Comment 36

[Mark Capella [:capella]]

Comment on attachment 8358435 [details] [diff] [review]
bug947284 Comment 32 Fix

aaronmt: Can I ask you to try out this follow on patch and see if it completely fixes the original issue for you?

I was easily able to repro in local testing and positively verify the fix to myself.

Comment 37

[Aaron Train [:aaronmt]]

Works for me with the Try build.

Comment 38

[:Margaret Leibovic]

Comment on attachment 8358435 [details] [diff] [review]
bug947284 Comment 32 Fix

Review of attachment 8358435 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the slow review! This looks good, thanks for investigating.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +282,5 @@
> +      Cu.reportError("SelectionHandler.js: _performSelection() Invalid selection mode " + aOptions.mode);
> +      return false;
> +    }
> +
> +    if (this._targetElement instanceof HTMLPreElement)  {

Add a comment here explaining why HTMLPreElements are special.

@@ +294,5 @@
> +    let selection = this._getSelection();
> +    let lastNode = selection.focusNode;
> +    while (lastNode && lastNode.lastChild) {
> +      lastNode = lastNode.lastChild;
> +    }

Doing this DOM manipulation does seem like it could be a performance issue, but luckily _performSelection is only called once in startSelection, so I don't think we need to worry. Also, we only go through this path for "select all" actions, which is hit even less frequently.

@@ +302,5 @@
> +        selection.extend(lastNode, lastNode.length);
> +      } catch (e) {
> +        Cu.reportError("SelectionHandler.js: _performSelection() whitespace trim fails: lastNode[" + lastNode +
> +          "] lastNode.length[" + lastNode.length + "]");
> +      }        

Nit: whitespace.

Comment 39

[Mark Capella [:capella]]

"Yeah, baby!" :-p

https://hg.mozilla.org/integration/fx-team/rev/72c4c3be4360

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/72c4c3be4360