336637 - Caret turds when pressing repeatedly backspace in this testcase

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect, P2)

Description

[Martijn Wargers (dead)]

See upcoming testcase.
I get caret **** when pressing backspace repeatetly in the textarea of this testcase, starting from the end of the text.

This regressed between 2006-04-27 and 2006-04-28, so I think a regression from bug 334649.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Uri Bernstein]

I'm seeing (on Mac) turds of the bidi marker (that is, small dots). Is that what you're seeing, or do you also see turds of the actual caret?

Comment 3

[Martijn Wargers (dead)]

Yes, I see mostly **** of the bidi marker. But I also see one time (in the middle of the textarea, somewhere) a **** of the actual caret.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Hack

This patch fixes two cases of BiDi turds that I saw:
  --  The turds when hitting backspace. I didn't dig too far down into why we were getting them, but one cause was when we were backspacing between BiDi levels, we were changing which frame the caret was recovering, causing full caret turds (see also the second thing). I'm not entirely sure of the second cause, but it seems like either a frame is moving or otherwise changing, confusing the caret.

  -- I also noticed a turd at the end of the line, between the English and Hebrew text. This was because when we were trying to erase the caret, the caret's BiDi level was changing before it could erase itself, causing it to invalidate the wrong area.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 220864 [details] [diff] [review]
Hack

Uri, could you also take a look at this? Thanks!

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

   mBidiLevel = aLevel;
 
+  // The caret depends on the current bidi level to find the frame it's in, so
+  // make sure we don't change the bidi level out from under it.
+  if (mCaret)
+    mCaret->InvalidateOutsideCaret();

Don't you want to do this before changing the bidi level? or both?

+
+  // Only invalidate if SetCaretBidiLevel didn't do it for us.
+  if (invalidate)
+  {
+    nsCOMPtr<nsICaret> caret;
+    shell->GetCaret(getter_AddRefs(caret));
+    if (caret)
+      caret->InvalidateOutsideCaret();
+  }

I think this is in the wrong place. We're not changing anything here that should require the caret to be invalidated.

Comment 7

[juan becerra [:juanb]]

*** Bug 337742 has been marked as a duplicate of this bug. ***

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better patch

It turns out that we do need to InvalidateOutsideCaret in PresShell::CharacterDataChanged. The problem is that when we send the CharacterDataChanged notification to the frame constructor (and thus to the frame), we're clearing all of the continuation frames' information about their character data, meaning that when the caret tries to compute the frame that it's in, we end up finding the wrong one and invalidating the wrong area, leaving turds.

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

Sorry for the spam, but does this fix bug 345640 and bug 313940 too?

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #9)
> Sorry for the spam, but does this fix bug 345640 and bug 313940 too?

No, those both show the same different problem, though.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 234128 [details] [diff] [review]
Better patch

Could you please put your explanation in a comment in there? thanks!

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: What I'm about to check in

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Using a SeaMonkey trunk build (2006-08-23-08) I no longer see caret turds when deleting with Backspace in the testcase at https://bugzilla.mozilla.org/attachment.cgi?id=220805.

Verified FIXED.