553975 - Caret is painted under textframe in input/textarea element

Status: RESOLVED FIXED

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

Description

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

I'm working on bug 299603 which will support IME selection style customization by IME. However, I'm troubled by this problem.

When IME sets background color, the caret isn't visible on the IME selection except the caret position is the end of the composition. I guess the causes:
* nsTextFrame paints the selection background color at painting its foreground.
* The caret paints on the background layer.
So, I guess the caret is overwritten by the selection background color.

And also nsCaret uses CSS text color of the frame. However, if the IME selection background color is same as the text color, the caret is never visible. Even if we fix the first problem.

I'm not sure what approach is best for this bug.

Comment 1

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

The caret's not in the background layer. It's in the content layer and should be displayed on top of the text frame it belongs to.

Comment 2

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

Oh, really? I'll check it tomorrow.

Comment 3

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

nsCaret::PaintCaret() is called *before* the nsTextFrame::PaintTextWithSelection(). So, is the z-order broken accidentally?

If I changed the following aLists.Content() to aLists.Outlines(), the caret becomes visible.

nsresult
nsIFrame::DisplayCaret(nsDisplayListBuilder* aBuilder,
                       const nsRect& aDirtyRect, const nsDisplayListSet& aLists)
{
  if (!IsVisibleForPainting(aBuilder))
    return NS_OK;

  return aLists.Content()->AppendNewToTop(
      new (aBuilder) nsDisplayCaret(this, aBuilder->GetCaret()));
}

Comment 4

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

looks like the nsCaret references correct textframe. So, the caret is drawn under the textframe.

Comment 5

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

nsIFrame::BuildDisplayListForChild does

      rv = aChild->BuildDisplayList(aBuilder, dirty, aLists);
      if (NS_SUCCEEDED(rv)) {
        rv = aBuilder->DisplayCaret(aChild, dirty, aLists);
      }

so the caret should be on top of anything added to the Content() list by aChild. I don't know how the caret is ending up under the text.

Comment 6

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

roc, nsTextControlFrame inherits nsStackFrame which sends DISPLAY_CHILD_FORCE_STACKING_CONTEXT to the aFlag. So, pseudoStackingContext is true.

The caret is inserted here:

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1595

but the contents are appended after that.
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1623

Isn't this the cause? I don't understand this code well. So, I'm not sure how to fix this.

Comment 7

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

I'm confused. Shouldn't we end up in the "true stacking context" path,
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1574 ?

Comment 8

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

(In reply to comment #7)
> I'm confused. Shouldn't we end up in the "true stacking context" path,
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1574 ?

Ah, right... I'm confused, sorry.

Comment 9

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

Attachment: Patch v1.0

I see the problem. In stacking context, we should append the caret to PositionedDescendants() because the contents are appended to there.

Comment 10

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

I don't think this is the right fix. The right fix would be to pass pseudoStack as the nsDisplayListSet instead of aLists in that one place, that should be all that's needed.

Comment 11

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

Attachment: Patch v1.1

fix a nit.

Can I use IsAtRootOfPseudoStackingContext() instead of the new param?

Comment 12

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

Comment on attachment 434446 [details] [diff] [review]
Patch v1.1

see comment #10.

Comment 13

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

Attachment: Patch v2.0

thank you for your advie! and I found a way for the testing.

Comment 14

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

Comment on attachment 434473 [details] [diff] [review]
Patch v2.0

The code looks good, but can't you just write a reftest? In reftests, the caret doesn't blink and the window has focus. You can call element.focus() to ensure that some particular element has focus.

Comment 15

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

(In reply to comment #14)
> (From update of attachment 434473 [details] [diff] [review])
> The code looks good, but can't you just write a reftest? In reftests, the caret
> doesn't blink and the window has focus. You can call element.focus() to ensure
> that some particular element has focus.

Because we need to synthesize the composition. Does reftest have the permission now?

Comment 16

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

OK, but you can test just the z-ordering of the caret in other ways. For example you should be able to demonstrate this bug using a positioned contenteditable block containing the caret.

Comment 17

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

(In reply to comment #16)
> OK, but you can test just the z-ordering of the caret in other ways. For
> example you should be able to demonstrate this bug using a positioned
> contenteditable block containing the caret.

No, I cannot reproduce it by such way. Isn't the important point of this bug that the selection background is painted by nsTextFrame as content?

Comment 18

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

Attachment: Testcase

Doesn't this testcase show the bug? After loading the page, the caret should be visible, but isn't. If you remove 'position:absolute' and reload, the caret is visible. So you could use this as the test and the version without 'position:absolute' as the reference.

Comment 19

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

Oh, OK. If the block has a textnode, this bug cannot reproduce. I was trapped by this.

Comment 20

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

Yes, because textnodes can't be positioned, only elements can.

Comment 21

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

Attachment: Patch v3.0

let's take this.

Comment 22

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

+      nsDisplayListCollection pseudoStack;
+      rv = aBuilder->DisplayCaret(aChild, dirty, pseudoStack);
+      list.AppendToTop(pseudoStack.Content());

Instead of doing this, it would be cleaner if DisplayCaret just appended to a list instead of an nsDisplayListCollection. Can you change that? You'll need to change nsFrame::DisplayCaret too.

Comment 23

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

Attachment: Patch v4.0

I posted this to tryserver.

Comment 24

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

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

Thank you, roc.