not sure if this is a dup, couldn't quite find an existing bug. pls dup/reassign as needed. found using 2003.05.13.05 comm bits on win2k; strangely, it appears limited to win2k, as i cannot repro this on linux rh8.0 or mac 10.2.6. 1. open a new editor window (composer or html mail compose). 2. enter a line of text, eg, "blah blah" 3. hit Enter key. 4. click either the bullet or number list buttons in the formatting toolbar. 5. hit the Backspace key to remove the [empty] list item and return to the empty line above (created in step 3). 6. repeat step 4 (click list button). results: an unblinking caret remains in the empty line, even though the active, blinking caret is in the first list item. you might need to repeat steps 4-6 to see this, but it'll show up.
resizing or minimizing/restoring the window clears away the "extra" caret.
this was prolly not caused by the fixed for bug 201242, since a build from 2003.04.28 still exhibits this. however, i think bug 35296 (which affected bug 195798) is involved here: a build from 2003.04.01.05 works fine, but one from 2003.04.02.09 shows this bug.
Assignee: jfrancis → leon.zhang
This bug is related to timer-controlled blink of caret: 1) when reproduce this bug in step 4 of bug's discription, if click bullet hust after caret have been painted, the bug can be reproduced, else, failed. 2) After debug, I found that: when the timer fire(nsTimerImpl::Fire()), nsCaret::CaretBlinkCallback will be called and it paint the caret once(so that we can see the caret), then when user click bullet, the caret be painted in new position, and the old caret(turd) has not been erased. Normally, the caret can blink, this controlled by timer and paint cate and erase it, but in this turd case, the caret just has be painted and has not be erased.
Created attachment 123579 [details] [diff] [review] proposed fix turn off caret before bullet process, and turn on caret after it
I'm not that familiar with the caret code, but don't we have code somewhere that should handle erasing the old caret position when the caret moves, like when you're typing text? Or does SetCaretEnabled get called in that case too?
Comment on attachment 123579 [details] [diff] [review] proposed fix There is another similar incorrectness when delelte chars: 1) input a char 2) type <CR> 3) type del key 4) move caret upward you will find a turd in original pos. So current patch should be upgraded.
Created attachment 123677 [details] [diff] [review] new version can reslove caret turd discribed in preious comments
Comment on attachment 123677 [details] [diff] [review] new version what does "return 0" mean for an nsresult? Please use an appropriate NS_ERROR value.
Attachment #123677 - Flags: review? → review-
Created attachment 123849 [details] [diff] [review] return NS_ERROR_FAILURE
Attachment #123677 - Attachment is obsolete: true
The patch doesn't have enough context to let me easily review it. Is this more special-casing, or do we understand why DeleteSelection() needs this, and other editor methods don't? When did this regress, and why?
I think this bug is related to bug 35296. There exist similar codes in nsHTMLEditRules::Before(/After)Edit, which turn off/on the caret before other actions. The second patch for bug 35296 have removed them, becuase it will produce an invalid cache value of caret position. (http://bugzilla.mozilla.org/attachment.cgi?id=117987&action=view). Maybe we just erase caret in function nsHTMLEditRules::BeforeEdit, so that we need not modiy current codes. I will try soon.
Created attachment 123850 [details] [diff] [review] turn off caret in function nsHTMLEditRules::BeforeEdit nsAutoRules() -> nsHTMLEditor::StartOperation -> nsHTMLEditRules::BeforeEdit I think current modifying point is appropriate.
Attachment #123849 - Attachment is obsolete: true
Comment on attachment 123850 [details] [diff] [review] turn off caret in function nsHTMLEditRules::BeforeEdit This change is ok with me. Some folks would probably prefer you return the real res value on this line: + if (NS_FAILED(res) || !caret) return NS_ERROR_FAILURE; but I don't particularly care. Simon or Kin should examine this change since they know caret functionality & performance better than I.
Attachment #123850 - Flags: review?(jfrancis) → review+
Comment on attachment 123850 [details] [diff] [review] turn off caret in function nsHTMLEditRules::BeforeEdit >+ // turn off caret You're not really turning off the caret, you're just erasing it from the screen. >+ nsCOMPtr<nsIPresShell> shell; >+ mEditor->GetPresShell(getter_AddRefs(shell)); >+ if (!shell) return NS_ERROR_NOT_INITIALIZED; >+ nsCOMPtr<nsICaret> caret; >+ res = shell->GetCaret(getter_AddRefs(caret)); >+ if (NS_FAILED(res) || !caret) return NS_ERROR_FAILURE; >+ caret->EraseCaret(); Is there a reason you're just erasing the caret, rather than having a SetCaretVisible(PR_FALSE) call here, and some later SetCaretVisible(PR_TRUE) call?
Comment on attachment 123850 [details] [diff] [review] turn off caret in function nsHTMLEditRules::BeforeEdit I really think that the "Patch to get rid of stCaretHider in nsEditor.cpp" (attachment 122300 [details] [diff] [review]) I posted in bug 194343 will prevent the caret cruft that occurs in the 2 cases in this bug. That patch prevents the situation where ClearFrameRefs() toggles the visible state of the caret without erasing it ... which throws the caret drawing code out of sync ... by turning the caret off prior to any edits happening, instead of just turning it on and off in EndUpdateViewBatch().
In old codes, nsHTMLEitRules::BeforeEdit and nsHTMLEitRules::AfterEdit will turn off /on caret. I removed them in bug 35296 because of better performance and nsHTMLEitRules::AfterEdit will compute an incorrect caret pos which will be cached. timer always fire events and draw caret, if it can not erase the caret itself and we do not erase it too, the caret turd will appear. I think old codes in nsHTMLEitRules::AfterEdit can do the same thing :) In the last patch named by "turn off caret in function nsHTMLEditRules::BeforeEdit", (http://bugzilla.mozilla.org/attachment.cgi?id=123850&action=view), I use function "caret->EraseCaret();" instead of "SetCaretVisible", because the latter function will related to timer process which is lead to performance regression in sol8 and it will not work for current test case. If Kin's patch works, it should be (r/sr)ed and checked in ASAP. There exist a similar bug 204434 which we should pay attention to. thx jfrancis, simon, kin :)
Comment on attachment 123850 [details] [diff] [review] turn off caret in function nsHTMLEditRules::BeforeEdit I just put attachment 122300 [details] [diff] [review] from bug 194343 into my tree, and I can no longer reproduce either problems mentioned in this bug.
kin: you are patch for bug 194343 is ok and can resolve this bug. bug 204434 is not related to this one, it reslults from reflow happen but our cache is not refreshed. I pasted a patch there.
*** This bug has been marked as a duplicate of 194343 ***
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → DUPLICATE
mozilla1.4 shipped. unsetting blocking1.4 request.
You need to log in before you can comment on or make changes to this bug.