554822 - Caret should refer the actual text color instead of the value of CSS color property

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

spinning out from bug 553975.

nsCaret uses CSS text color of the reference frame. However, if the IME
selection background color is same as the text color, the caret is never
visible. Even if we fix bug 553975.

I think that nsTextFrame should set the color to nsCaret at painting the selection.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

The caret is always painted by the actual foreground color of the character which is the caret position. This patch uses (n+1)th character's color when aOffset is (n) because caret is painted over the (n+1)th character.

I have another idea for this patch. It might be better that I create nsIFrame::PaintCaretAt(nsRect &aRect). Then, nsTextFrame can paint caret with clipped character glyph whose color is background color. If so, we can support wide caret (needed for Korean IME) and we can fix the CJK caret issue (bug 335359) completely. However, seems I need more complex code for that, so, this patch is better for now.

Comment 2

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

+    foregroundColor = aForFrame->GetCaretColorAt(mLastContentOffset - start);

GetCaretColorAt should just take the real content offset, instead of an offset "relative to the frame's start offset".

+    NS_WARNING("nsIFrame::GetOffsets() failed");
+    foregroundColor = aForFrame->GetStyleColor()->mColor;

This shouldn't happen, so just return instead of handling it.

+  if (!IsSelected()) {
+    return nsFrame::GetCaretColorAt(aOffset);
+  }

I don't think we need this code.

+    if (start < end && start <= aOffset && aOffset < end &&

You don't need to check start < end here.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

Thank you for your review.

And I found a bug. mLastContentOffset sometimes isn't in the text content. E.g., when I clicked right space of a line which is broken by <br>, mLastContentOffset value is the <br>'s offset in its parent node. I added mLastFrameOffset for the bug.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0.1

Sorry, the previous patch has a debug printf().

Comment 5

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

Instead of storing mLastFrameOffset I think we can call GetCaretFrameForNodeOffset to get it. Or maybe GetCaretFrame should pass the frame offset as an out parameter, and then we can ensure the caller of GetCaretFrame eventually passes the offset to nsCaret::PaintCaret?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.0

Comment 7

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

Comment on attachment 436150 [details] [diff] [review]
Patch v3.0

+  if (frame != aForFrame) {
+    return;
+  }

This should be an assertion that frame == aForFrame.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.1

thank you, roc.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/50781197a2b3

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

backed-out the patch due to the patch cannot build with latest trunk.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.2

I implemented the nsIFrame::GetCaretColorAt() in #ifdef DEBUG. So, this patch doesn't change any meaning.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/c746dd4dde4c