Closed Bug 346185 Opened 15 years ago Closed 15 years ago

fixing one of two misspelled words in a line makes spellcheck underline disappear on both

Categories

(Core :: Spelling checker, defect, P1)

1.8 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta2

People

(Reporter: dbaron, Assigned: brettw)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 1 obsolete file)

Fixing one of two misspelled words in a line makes the spellcheck underline disappear on both words:  but only until something else forces a repaint.

Steps to reproduce:
 1. load this bug report
 2. in the textarea, type "gren and bron "
 3. click on the "e" in "gren"
 4. type the additional "e"
 5. do something to trigger a window repaint (sometimes moving the mouse pointer out of and into the window does this, otherwise cover the window with another one)

Expected results:
 4. underline disappears on "green" but remains on "bron"

Actual results:
 4. underline disappears on both  [seems to disappear on any misspelled words in the same line]
 5. underline returns on "bron"

Tested in a Linux trunk build from a day or two ago.

Bug 345979 *could* be an instance of this problem, but it doesn't have clearly described "Actual Results".
I think I understand the problem.

There's a flag on all frames that have a selection on them. When this flag is set, the frame goes and asks the selection where the selection is on that frame. The selection is then painted. Bug 345099 changed a lot of how the selection does this lookup. It shouldn't have affected how this works, but it could have.

When we remove a range from the selection, the code uses a content iterator to remove the selected flag from the frames. The problem is if there is another range on the frame, the frame no longer knows to ask the selection controller to update itself.

If I remove the code that clears the bit, everything works properly, but then the selected bits never get cleared, slowing things down as you create and remove selections.

I'm going to try to find if there are other selections on the affected nodes first, and not clear the bit when there is.
Attached patch Patch (obsolete) — Splinter Review
Attachment #231036 - Flags: review?(uriber)
Assignee: mscott → brettw
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.8.1beta2
Version: Trunk → 1.8 Branch
What happens if the selection ends at a non-text leaf node (e.g. an <img> or a <br>)?
In this case, I think GetEndContainer will return the parent of that node, and you'll be processing all the children of the parent, which might be a lot more than what you actually need.

Other than that, just some trivial comments about comments:

>+//    We therefore find any ranges that intersect the same nodes as the node
>+//    being removed
I think you mean "the range being removed"

>+    // select the length of the text
...
>+    // select the number of children
"get" or "find out" would be clearer than "select" in this context, although I'm not sure whether these comments are necessary at all.

>+  // clear the selected bit from the removed node's frames
"removed range's frames"?
(In reply to comment #3)
> What happens if the selection ends at a non-text leaf node

I meant "the range", not "the selection".
*** Bug 345979 has been marked as a duplicate of this bug. ***
Attached patch New patchSplinter Review
I think you are right, I only want to select the entire node when it is a text node. For non-text nodes, using the given offsets and finding all ranges touching those nodes is sufficient.
Attachment #231036 - Attachment is obsolete: true
Attachment #231248 - Flags: review?(uriber)
Attachment #231036 - Flags: review?(uriber)
Whiteboard: [needs review]
Comment on attachment 231248 [details] [diff] [review]
New patch

r=me.
Attachment #231248 - Flags: review?(uriber) → review+
Attachment #231248 - Flags: superreview?(bryner)
Attachment #231248 - Flags: superreview?(bryner) → superreview+
Fixed on trunk, leaving open for branch checkin.
Whiteboard: [needs review]
Attachment #231248 - Flags: approval1.8.1?
Whiteboard: [needs approval]
Comment on attachment 231248 [details] [diff] [review]
New patch

a=drivers. Please land this on the MOZILLA_1_8_BRANCH
Attachment #231248 - Flags: approval1.8.1? → approval1.8.1+
Fixed on branch.
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs approval]
*** Bug 339077 has been marked as a duplicate of this bug. ***
This problem is back with a slight twist. It works fine if the two errors are on the 1st line of the email text area. But if the errors occur on a subsequent line the error reappears. Type the line

ths si a tst - corrected the 1st word other two underlines stayed, corrected 2nd word, underline on 3rd word disappeared

test ths si a tst  - corrected 'ths' underline on other two lines disappeared. 

The disappearance of the underline changes dependent on where the line is types. 

Ben Klein
This bug is already marked "Resolved".

The disappearing underlines are reported here in bug 1100966.
You need to log in before you can comment on or make changes to this bug.