caret turd persists after deleting and recreating list

RESOLVED DUPLICATE of bug 194343

Status

()

--
minor
RESOLVED DUPLICATE of bug 194343
16 years ago
15 years ago

People

(Reporter: bugzilla, Assigned: leon.zhang)

Tracking

(Blocks: 1 bug, {regression})

Trunk
x86
Windows 2000
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

16 years ago
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.
(Reporter)

Comment 1

16 years ago
resizing or minimizing/restoring the window clears away the "extra" caret.
(Assignee)

Comment 2

16 years ago
I feel bug 201242 and bug 195798 is related to this one.
 
(Reporter)

Comment 3

16 years ago
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
(Assignee)

Comment 4

16 years ago
take it
Assignee: jfrancis → leon.zhang
(Assignee)

Comment 5

16 years ago
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.



  
(Assignee)

Comment 6

16 years ago
Created attachment 123579 [details] [diff] [review]
proposed fix

turn off caret before bullet process, and turn on caret after it
(Assignee)

Updated

16 years ago
Blocks: 188318
(Assignee)

Updated

16 years ago
Attachment #123579 - Flags: superreview?(kin)
Attachment #123579 - Flags: review?(kin)
(Assignee)

Updated

16 years ago
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?
(Assignee)

Comment 8

16 years ago
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)
(Assignee)

Comment 9

16 years ago
Created attachment 123677 [details] [diff] [review]
new version

can reslove caret turd discribed in preious comments
(Assignee)

Updated

16 years ago
Attachment #123677 - Flags: superreview?
Attachment #123677 - Flags: review?

Comment 10

16 years ago
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-
(Assignee)

Comment 11

16 years ago
Created attachment 123849 [details] [diff] [review]
return NS_ERROR_FAILURE
Attachment #123677 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #123677 - Flags: superreview?

Comment 12

16 years ago
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?
(Assignee)

Comment 13

16 years ago
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.
(Assignee)

Comment 14

16 years ago
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
(Assignee)

Updated

16 years ago
Attachment #123850 - Flags: superreview?(sfraser)
Attachment #123850 - Flags: review?(jfrancis)

Comment 15

16 years ago
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 16

16 years ago
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

15 years ago
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().
(Assignee)

Comment 18

15 years ago
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

15 years ago
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.
(Assignee)

Comment 20

15 years ago
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.
(Assignee)

Comment 21

15 years ago

*** This bug has been marked as a duplicate of 194343 ***
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → DUPLICATE

Comment 22

15 years ago
mozilla1.4 shipped. unsetting blocking1.4 request.
Flags: blocking1.4?

Updated

13 years ago
Attachment #123850 - Flags: superreview?(sfraser_bugs)
You need to log in before you can comment on or make changes to this bug.