205544 - caret turd persists after deleting and recreating list

Status: RESOLVED DUPLICATE of bug 194343

(Core :: DOM: Editor, defect)

Description

[sairuh (rarely reading bugmail)]

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.

Comment 1

[sairuh (rarely reading bugmail)]

resizing or minimizing/restoring the window clears away the "extra" caret.

Comment 2

[leon.zhang]

I feel bug 201242 and bug 195798 is related to this one.
 

Comment 3

[sairuh (rarely reading bugmail)]

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.

Comment 4

[leon.zhang]

take it

Comment 5

[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.



  

Comment 6

[leon.zhang]

Attachment: proposed fix

turn off caret before bullet process, and turn on caret after it

Comment 7

[Brian Ryner (not reading)]

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 8

[leon.zhang]

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.

Comment 9

[leon.zhang]

Attachment: new version

can reslove caret turd discribed in preious comments

Comment 10

[Joe Francis]

Comment on attachment 123677 [details] [diff] [review]
new version

what does "return 0" mean for an nsresult?  Please use an appropriate NS_ERROR
value.

Comment 11

[leon.zhang]

Attachment: return NS_ERROR_FAILURE

Comment 12

[Simon Fraser [no longer active]]

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?

Comment 13

[leon.zhang]

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.

Comment 14

[leon.zhang]

Attachment: turn off caret in function nsHTMLEditRules::BeforeEdit

nsAutoRules() -> nsHTMLEditor::StartOperation -> nsHTMLEditRules::BeforeEdit
I think current modifying point is appropriate.

Comment 15

[Joe Francis]

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.

Comment 16

[Simon Fraser [no longer active]]

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 17

[kinmoz]

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().

Comment 18

[leon.zhang]

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 19

[kinmoz]

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.

Comment 20

[leon.zhang]

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.

Comment 21

[leon.zhang]

*** This bug has been marked as a duplicate of 194343 ***

Comment 22

[Asa Dotzler [:asa]]

mozilla1.4 shipped. unsetting blocking1.4 request.