Closed
Bug 205544
Opened 22 years ago
Closed 21 years ago
caret turd persists after deleting and recreating list
Categories
(Core :: DOM: Editor, defect)
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)
(deleted),
patch
|
mozeditor
:
review+
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
resizing or minimizing/restoring the window clears away the "extra" caret.
Assignee | ||
Comment 2•22 years ago
|
||
I feel bug 201242 and bug 195798 is related to this one.
Reporter | ||
Comment 3•21 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 5•21 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•21 years ago
|
||
turn off caret before bullet process, and turn on caret after it
Assignee | ||
Updated•21 years ago
|
Attachment #123579 -
Flags: superreview?(kin)
Attachment #123579 -
Flags: review?(kin)
Assignee | ||
Updated•21 years ago
|
Flags: blocking1.4?
Comment 7•21 years ago
|
||
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•21 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•21 years ago
|
||
can reslove caret turd discribed in preious comments
Assignee | ||
Updated•21 years ago
|
Attachment #123677 -
Flags: superreview?
Attachment #123677 -
Flags: review?
Comment 10•21 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•21 years ago
|
||
Attachment #123677 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #123677 -
Flags: superreview?
Comment 12•21 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•21 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•21 years ago
|
||
nsAutoRules() -> nsHTMLEditor::StartOperation -> nsHTMLEditRules::BeforeEdit
I think current modifying point is appropriate.
Attachment #123849 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #123850 -
Flags: superreview?(sfraser)
Attachment #123850 -
Flags: review?(jfrancis)
Comment 15•21 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•21 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•21 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•21 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•21 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•21 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•21 years ago
|
||
*** This bug has been marked as a duplicate of 194343 ***
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → DUPLICATE
Updated•19 years ago
|
Attachment #123850 -
Flags: superreview?(sfraser_bugs)
You need to log in
before you can comment on or make changes to this bug.
Description
•