372139 - the composition string of IME is not rendered as selected text

Status: VERIFIED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

The composition string of IME is not rendered as selected text. Therefore, it is very hard for using IME. This is regressed at 27th build. I think that the cause is bug 371839.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

I'll attach a patch.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm sorry for the trouble; I've never really figured out how to test IME, unfortunately.  :(

Out of curiousity, does IME trigger the "internal offsets may be out-of-sync" warning?  Or is this something else?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Patch rv1.0

fix :-)

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Um.. I'm confusing.

The patch fixes this bug, but is it correct??

    if (cur->mContentOffset + cur->mContentLength >= startOffset) {
    if (cur->mContentOffset < endOffset) {

should be:

    if (cur->mContentOffset >= startOffset) {
    if (cur->mContentOffset + cur->mContentLength < endOffset) {

??

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm...  That's odd...  If cur->mContentOffset + cur->mContentLength == startOffset, then that means that the text this frame maps ends right before aRange starts, doesn't it?  So why would the selection affect it?  What are the offsets and length in this case?

One thing I wonder.  If you change this code:

+    if (GetContent() == range->GetStartParent()) {
+      startOffset = range->StartOffset();
+    } else {
+      startOffset = 0;
+    }

to set startOffset to -1, does that help?

Could it be that we're basically changing the text in the textframe without ending up inside SetSelected after it's changed or something?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Oops, sorry, please ignore comment 4... it's wrong.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I mean set startOffset to -1 when we currently set it to 0.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Patch rv2.0

You're right. This patch works fine.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Uh... no.  That wasn't the patch I suggested.  And if _that_ patch works fine I think we have a problem.  Do you happen to know the answers to the questions in comment 5?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Give me a bit; I'll try to write some tests to determine what's going on here...

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Patch rv3.0

This patch doesn't work fine.

And we find strange things.
1. Type some keys -> the composition string is rendered as normal text.
2. Commit the string.
3. Move caret to left by arrow key or mouse.
4. Type some keys -> the composition string is rendered as selected text. (fine)

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

5. Move caret to right most.
6. Type some keys -> (fine)
7. Delete a character by Backspace.
8. Type some keys -> (fine)
9. Delete all characters by Backspace.
10. Type some keys -> (buggy)

Comment 13

[Boris Zbarsky [:bzbarsky]]

So how does IME actually work, visually?  Is there basically a selection in the original textfield that you're trying to put the text in that shows what the text will be if you commit?

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

(In reply to comment #13)
> So how does IME actually work, visually?  Is there basically a selection in the
> original textfield that you're trying to put the text in that shows what the
> text will be if you commit?

Sorry, I cannot understand well.

When I typed some characters, they are a composition string that is a selection text in Gecko. When I commit the composition string, the composition string becomes normal text.

The IME behavior is not broken on current build. The composition string rendering is only broken. But the characters of the composition string are correct, there are not dropped characters.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Right.  I understand that.  OK.  Let me see what I can figure out here.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: screenshot of composition string

The left is current on current trunk build(28th).
The right is 26th trunk build.

The composition string should be underlined.

Note that at the converting process, the composition string will be separated to multi parts. And the focused part will be rendering as normal selection color if this bug is fixed.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

It's strange... While IME composing, the cur->mContentLength is always zero.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

And when I typed the ASCII text, the cur->mContentLength is actual length - 1. So, the current inputting character is not included there.

Comment 19

[Boris Zbarsky [:bzbarsky]]

"current inputting character"?  What's the callstack to that SetSelected call?

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Unfortunately, I don't create the debug build. I don't know the caller.

Comment 21

[Boris Zbarsky [:bzbarsky]]

You can get that in an opt build too, as long as you didn't strip it...

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing what I suspect is the same bug

As I was afraid of, as stuff moves around we need to move the NS_FRAME_SELECTED_CONTENT bits to new frames as needed...

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

I built the debug build and got the callstack.

at typed with IME:
>>	gklayout.dll!nsTextFrame::SetSelected(nsPresContext * aPresContext=0x00f60da0, nsIDOMRange * aRange=0x00000000, int aSelected=1, nsSpread aSpread=eSpreadDown)  行 4286	C++
>  	gklayout.dll!nsTypedSelection::selectFrames(nsPresContext * aPresContext=0x00f60da0, nsIDOMRange * aRange=0x04a30cd4, int aFlags=1)  行 5060	C++
>  	gklayout.dll!nsTypedSelection::AddRange(nsIDOMRange * aRange=0x00f60da0)  行 5723	C++
>  	gklayout.dll!IMETextTxn::CollapseTextSelection()  行 353	C++
>  	gklayout.dll!IMETextTxn::DoTransaction()  行 94 + 0x6 バイト	C++
>  	txmgr.dll!nsTransactionItem::DoTransaction()  行 182	C++
>  	txmgr.dll!nsTransactionManager::BeginTransaction(nsITransaction * aTransaction=0x04a30a60)  行 1071 + 0x5 バイト	C++
>  	txmgr.dll!nsTransactionManager::DoTransaction(nsITransaction * aTransaction=0x036b43e0)  行 132 + 0xd バイト	C++
>  	gklayout.dll!nsEditor::DoTransaction(nsITransaction * aTxn=0x036b43e0)  行 723	C++
>  	gklayout.dll!nsEditor::InsertTextIntoTextNodeImpl(const nsAString_internal & aStringToInsert={...}, nsIDOMCharacterData * aTextNode=0x04a30b00, int aOffset=0, int suppressIME=0)  行 2638 + 0x9 バイト	C++
>  	gklayout.dll!nsEditor::InsertTextImpl(const nsAString_internal & aStringToInsert={...}, nsCOMPtr<nsIDOMNode> * aInOutNode=0x04a30b00, int * aInOutOffset=0x04a30b00, nsIDOMDocument * aDoc=0x00000000)  行 2539 + 0x14 バイト	C++
>  	gklayout.dll!nsTextEditRules::WillInsertText(int aAction=2001, nsISelection * aSelection=0x0416ad10, int * aCancel=0x0013efcc, int * aHandled=0x0013efc0, const nsAString_internal * inString=0x0013f0f8, nsAString_internal * outString=0x0013efe4, int aMaxLength=-1)  行 667 + 0x1d バイト	C++
>  	gklayout.dll!nsTextEditRules::WillDoAction(nsISelection * aSelection=0x0416ad10, nsRulesInfo * aInfo=0x0013ef84, int * aCancel=0x0013efcc, int * aHandled=0x0013efc0)  行 314 + 0x17 バイト	C++
>  	gklayout.dll!nsPlaintextEditor::InsertText(const nsAString_internal & aStringToInsert={...})  行 759 + 0x32 バイト	C++
>  	gklayout.dll!nsPlaintextEditor::SetCompositionString(const nsAString_internal & aCompositionString={...}, nsIPrivateTextRangeList * aTextRangeList=0x00000203, nsTextEventReply * aReply=0x0013f4f0)  行 1637	C++

at after reflow:
>>	gklayout.dll!nsTextFrame::MeasureText(nsPresContext * aPresContext=0x00f60da0, const nsHTMLReflowState & aReflowState={...}, nsTextTransformer & aTx={...}, nsTextStyle & aTs={...}, nsTextFrame::TextReflowData & aTextData={...})  行 5045	C++
>  	gklayout.dll!nsTextFrame::Reflow(nsPresContext * aPresContext=0x00f60da0, nsHTMLReflowMetrics & aMetrics={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0)  行 6099 + 0x1a バイト	C++
>  	gklayout.dll!nsLineLayout::ReflowFrame(nsIFrame * aFrame=0x03577c14, unsigned int & aReflowStatus=0, nsHTMLReflowMetrics * aMetrics=0x00000000, int & aPushedFrame=0)  行 889	C++
>  	gklayout.dll!nsBlockFrame::ReflowInlineFrame(nsBlockReflowState & aState={...}, nsLineLayout & aLineLayout={...}, nsLineList_iterator aLine={...}, nsIFrame * aFrame=0x03577c14, LineReflowStatus * aLineReflowStatus=0x0013e4b0)  行 3414	C++
>  	gklayout.dll!nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState & aState={...}, nsLineLayout & aLineLayout={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0013e724, LineReflowStatus * aLineReflowStatus=0x0013e5b4, int aAllowPullUp=1)  行 3236	C++
>  	gklayout.dll!nsBlockFrame::ReflowInlineFrames(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0013e724)  行 3084	C++
>  	gklayout.dll!nsBlockFrame::ReflowLine(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0013e724)  行 2180	C++
>  	gklayout.dll!nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & aState={...})  行 1788	C++
>  	gklayout.dll!nsBlockFrame::Reflow(nsPresContext * aPresContext=0x00f60da0, nsHTMLReflowMetrics & aMetrics={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=1)  行 912	C++
>  	gklayout.dll!nsContainerFrame::ReflowChild(nsIFrame * aKidFrame=0x04168da8, nsPresContext * aPresContext=0x00f60da0, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, int aX=0, int aY=0, unsigned int aFlags=3, unsigned int & aStatus=1)  行 760 + 0x14 バイト	C++
>  	gklayout.dll!nsHTMLScrollFrame::ReflowScrolledFrame(const ScrollReflowState & aState={...}, int aAssumeHScroll=0, int aAssumeVScroll=0, nsHTMLReflowMetrics * aMetrics=0x0013ecbc, int aFirstPass=1)  行 471	C++
>  	gklayout.dll!nsHTMLScrollFrame::ReflowContents(ScrollReflowState * aState=0x0013ee08, const nsHTMLReflowMetrics & aDesiredSize={...})  行 533 + 0x15 バイト	C++
>  	gklayout.dll!nsHTMLScrollFrame::Reflow(nsPresContext * aPresContext=0x00000000, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0)  行 749	C++
>  	gklayout.dll!PresShell::ProcessReflowCommands(int aInterruptible=1)  行 5955	C++
>  	gklayout.dll!PresShell::WillPaint()  行 5640	C++
>  	gklayout.dll!nsViewManager::FlushPendingInvalidates()  行 2594	C++
>  	gklayout.dll!nsViewManager::EnableRefresh(unsigned int aUpdateFlags=2)  行 2230	C++
>  	gklayout.dll!nsViewManager::EndUpdateViewBatch(unsigned int aUpdateFlags=2)  行 2279 + 0x7 バイト	C++
>  	gklayout.dll!nsEditor::EndUpdateViewBatch()  行 4363	C++
>  	gklayout.dll!nsEditor::EndPlaceHolderTransaction()  行 984	C++
>  	gklayout.dll!nsAutoPlaceHolderBatch::~nsAutoPlaceHolderBatch()  行 66 + 0x16 バイト	C++
>  	gklayout.dll!nsPlaintextEditor::SetCompositionString(const nsAString_internal & aCompositionString={...}, nsIPrivateTextRangeList * aTextRangeList=0x00000203, nsTextEventReply * aReply=0x0013f4f0)  行 1651	C++

So, the SetSelected is called *before* reflow. Therefore, the mContentOffset and mContentLength are not set the valid values. In nsEditor, for optimizing and suppressing the flicker, a transaction (i.e., inserting the text) edits the DOM tree and changes the selection while suppressing the painting and reflow.

I think that we should back out your patch. Because this regression is very unusable for us.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> I built the debug build and got the callstack.

Do you get the warning I asked about in comment 2 as well?

> I think that we should back out your patch. 

How about we fix this bug instead, which is what I've spent the last few hours on?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: More extensive testcase, by a bit

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

(In reply to comment #24)
> > I built the debug build and got the callstack.
> 
> Do you get the warning I asked about in comment 2 as well?

I cannot found the warning, sorry.

> > I think that we should back out your patch. 
> 
> How about we fix this bug instead, which is what I've spent the last few hours
> on?

If we can few days, it's better. But I cannot accept that this bug lives in long time.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

I think this should fix things.  If it does, we should land it, then file a followup on a saner way of doing this.

Comment 28

[Boris Zbarsky [:bzbarsky]]

I have no intention of letting this live a long time.  ;)

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

O.K. thank you. I'm building with the patch, please wait.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Great! The patch works fine. Let's land it.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Actually, I thought about it some more and I think that patch would cause an unacceptable pageload regression.

Thanks to the testing you did in this bug, Masayuki-san, I do have some ideas for making it better, but I'm going to wait on roc's new textframe to land first.  So I backed out bug 371839 for now.

I'm not quite sure how to go about testing this.  My testcases _sorta_ work, except they start to paint correctly if I invalidate the whole window.  :(  So reftest won't catch that issue, and mochitest has no way do it either....

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

->v. thank you.