181105 - Selection behaves badly when SpellCheck Selection is on

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[rods (gone)]

If I have a couple of words on a line highlighted with the SpellCheck selection
the "regualr" selection will only select the entire line.

If I try to double click on a word, or do a drag select, or even position the
caret and then hold the shift key down and use the arrow keys.

Comment 1

[mjudge]

gotta fix this one.

Comment 2

[chris hofmann]

unsetting target milestone

Comment 3

[Peter Lubczynski]

I think I have a fix for this particular situation too but I see a few other
bugs afterwords:

In nsTextFrame.cpp, |DrawSelectionIterator::CurrentBackGroundColor| and
|DrawSelectionIterator::CurrentForeGroundColor| there are a few 'if' statements
that should do an 'and' rather than an 'or':

mTypes[mCurrentIdx] | nsISelectionController::SELECTION_NORMAL

...should be ...

mTypes[mCurrentIdx] & nsISelectionController::SELECTION_NORMAL

Comment 4

[Kathleen :Brade]

-->peterlubczynski

Comment 5

[Neil Deakin]

Attachment: Make alternate selection types render properly

This patch also fixes bug 220157.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 141155 [details] [diff] [review]
Make alternate selection types render properly

>Index: content/base/src/nsSelection.cpp
>@@ -2792,17 +2792,37 @@ nsSelection::LookUpSelection(nsIContent 
>+      mDomSelections[j]->GetIsCollapsed(&iscollapsed);
>+      if (iscollapsed){

!iscollapsed, no?

r=bzbarsky with that.  Gotta love that code in nsFrame.

sfraser should take a look at this, probably....

Comment 7

[Neil Deakin]

Attachment: Better patch

Fixes the iscollapsed as mentioned above. I had to have the 'slow check' done
whenever one of the extra selections is set. Changing it to be better would
require more digging.

Comment 8

[Boris Zbarsky [:bzbarsky]]

It'll take me a few days to get to this (maybe till this coming weekend,
worst-case).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 141176 [details] [diff] [review]
Better patch

>Index: content/base/src/nsSelection.cpp
>   PRInt8 j;
>+  for (j = (PRInt8) 1; j < (PRInt8)nsISelectionController::NUM_SELECTIONTYPES; j++){

Isn't the selection at "1" the normal selection?  So this patch always follows
the slow path when a selection exists?

>+      if (!iscollapsed){
>+        aSlowCheck = PR_TRUE;

and break here?

>Index: layout/html/base/src/nsFrame.cpp
>+          if (curDetail->mType != nsISelectionController::SELECTION_SPELLCHECK &&

This bizarreness should be documented in the selection controller IDL,
probably.

Comment 10

[Neil Deakin]

> Isn't the selection at "1" the normal selection?  So this patch always follows
> the slow path when a selection exists?

Yes, but that's misleading since the normal path is the slow method and the slow
 path is the even slower method. Both methods will iterate over all ranges in
the selection every time a text frame is drawn which is incredibly slow with
lots of ranges. I haven't been able to come up with a good way of improving it.

Comment 11

[Martijn Wargers (dead)]

Bug 220157 works for me now. That bug was probably fixed by the fix for bug 278312.
Maybe this bug is now also worksforme?

Comment 12

[Neil Deakin]

Yes, the inline spell code should have fixed this.