Closed Bug 553975 Opened 10 years ago Closed 10 years ago

Caret is painted under textframe in input/textarea element

Categories

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

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9.3a4

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
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.
Oh, really? I'll check it tomorrow.
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()));
}
looks like the nsCaret references correct textframe. So, the caret is drawn under the textframe.
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.
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.
(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.
Attached patch Patch v1.0 (obsolete) — Splinter Review
I see the problem. In stacking context, we should append the caret to PositionedDescendants() because the contents are appended to there.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #434444 - Flags: review?(roc)
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.
Attached patch Patch v1.1 (obsolete) — Splinter Review
fix a nit.

Can I use IsAtRootOfPseudoStackingContext() instead of the new param?
Attachment #434444 - Attachment is obsolete: true
Attachment #434446 - Flags: review?(roc)
Attachment #434444 - Flags: review?(roc)
Attached patch Patch v2.0 (obsolete) — Splinter Review
thank you for your advie! and I found a way for the testing.
Attachment #434446 - Attachment is obsolete: true
Attachment #434473 - Flags: review?(roc)
Summary: Caret invisible problem on IME selection → Caret is painted under textframe in input/textarea element
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.
Attachment #434473 - Flags: review?(roc) → review+
(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?
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.
(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?
Attached file 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.
Oh, OK. If the block has a textnode, this bug cannot reproduce. I was trapped by this.
Yes, because textnodes can't be positioned, only elements can.
Attached patch Patch v3.0 (obsolete) — Splinter Review
let's take this.
Attachment #434473 - Attachment is obsolete: true
Attachment #434762 - Flags: review?(roc)
+      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.
Attached patch Patch v4.0Splinter Review
I posted this to tryserver.
Attachment #434762 - Attachment is obsolete: true
Attachment #434783 - Flags: review?(roc)
Attachment #434762 - Flags: review?(roc)
http://hg.mozilla.org/mozilla-central/rev/f9e4da420fbf

Thank you, roc.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
Duplicate of this bug: 256989
You need to log in before you can comment on or make changes to this bug.