Closed Bug 205544 Opened 22 years ago Closed 21 years ago

caret turd persists after deleting and recreating list

Categories

(Core :: DOM: Editor, defect)

x86
Windows 2000
defect
Not set
minor

Tracking

()

RESOLVED DUPLICATE of bug 194343

People

(Reporter: bugzilla, Assigned: leon.zhang)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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.
I feel bug 201242 and bug 195798 is related to this one.
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.
Keywords: regression
take it
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.
Attached patch proposed fix (obsolete) (deleted) — Splinter Review
turn off caret before bullet process, and turn on caret after it
Blocks: 188318
Attachment #123579 - Flags: superreview?(kin)
Attachment #123579 - Flags: review?(kin)
Flags: blocking1.4?
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.
Attachment #123579 - Attachment is obsolete: true
Attachment #123579 - Flags: superreview?(kin)
Attachment #123579 - Flags: review?(kin)
Attached patch new version (obsolete) (deleted) — Splinter Review
can reslove caret turd discribed in preious comments
Attachment #123677 - Flags: superreview?
Attachment #123677 - Flags: review?
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-
Attached patch return NS_ERROR_FAILURE (obsolete) (deleted) — Splinter Review
Attachment #123677 - Attachment is obsolete: true
Attachment #123677 - Flags: superreview?
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.
nsAutoRules() -> nsHTMLEditor::StartOperation -> nsHTMLEditRules::BeforeEdit I think current modifying point is appropriate.
Attachment #123849 - Attachment is obsolete: true
Attachment #123850 - Flags: superreview?(sfraser)
Attachment #123850 - Flags: review?(jfrancis)
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
Closed: 21 years ago
Resolution: --- → DUPLICATE
mozilla1.4 shipped. unsetting blocking1.4 request.
Flags: blocking1.4?
Attachment #123850 - Flags: superreview?(sfraser_bugs)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: