Although I don't know regression window, I found this issue today. Step 1. Open compose on gmail 2. input 'あああああ' 3. Enter * 2 (commit and insert CR) 4. input 'あ'. Until this step, it works fine 5. input 'あ' Result Candidate window is overlapped on composition string

Hmm, looks like that we notify TSF of temporary position if the layout information isn't available. Then, Google Japanese Input decides the position, and after that, even if we notify TSF of layout change, Google Japanese Input doesn't update the window position... But we still need more investigation before posting this issue to their community. (I investigated this just roughly.)

This must be TSF mode only issue, if I'm wrong, feel free to remove "[TSF]" from the description.

The cause of this bug is, Google Japanese Input tries to retrieve last character of the line when caret is at the end of line. So, ContentCache should store previous character's rects of mFocus and mAnchor. I'm now dogfooding the patches at least today.

FYI: Coming patches still have a bug when caret is at the end of contenteditable editor. In such case, ITextStoreACP::GetTextExt() still returns E_FAIL. But this is not so serious problem and we need another fix. So, let's fix it in a followup bug.

Review commit: https://reviewboard.mozilla.org/r/63878/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63878/

This patch makes ContentCache store previous character's rect of selection anchor and selection focus because if caret is at end of a line, IME may query the last character of the line. Review commit: https://reviewboard.mozilla.org/r/63880/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63880/

Returning empty rects for eQueryTextRectArray causes each dispatcher needing to check every rect. It doesn't make sense especially compared with eQueryTextRect. So, it should ensure that empty rect won't be returned to dispatchers. Review commit: https://reviewboard.mozilla.org/r/63882/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63882/

New eQueryTextRectArray causes a lot of assertions in various automated tests. The cause is that nsTextFrame::GetCharacterRectsInRange() calls gfxSkipCharsIterator::AdvanceOriginal(1) at the end of the |for| loop *without* checking if the iterator has already reached to the end. Therefore, this patch makes it call immediately after checking the range at top of the |for| loop except when |i| is 0. I have no idea of smarter fix. Review commit: https://reviewboard.mozilla.org/r/63884/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63884/

Comment on attachment 8770402 [details] Bug 1257446 part.0 ContentCacheInParent::HandleQueryContentEvent() should log the cause of failure when it makes the event's input offset absolute https://reviewboard.mozilla.org/r/63878/#review61148

Comment on attachment 8770403 [details] Bug 1257446 part.1 ContentCache should store previous character of selection https://reviewboard.mozilla.org/r/63880/#review61160 ::: widget/ContentCache.h:86 (Diff revision 1) > mWritingMode = WritingMode(); > - mAnchorCharRect.SetEmpty(); > - mFocusCharRect.SetEmpty(); > + mAnchorCharRects[0].SetEmpty(); > + mAnchorCharRects[1].SetEmpty(); > + mFocusCharRects[0].SetEmpty(); > + mFocusCharRects[1].SetEmpty(); > mRect.SetEmpty(); Other codes use eNextCharRect and ePrevCharRect as index. Why do you use both enum value? I think you create ClearAnchorCharRects and ClearFocusRects like the following. void ClearAnchorCharRects() \{ mAnchorCharRects\[0 or eNextCharRect\].SetEmpty(); mAnchorCharRects\[1 or ePrevCharRect\].SetEmpty(); \} and same of ClearFocusCharRects. Then, call ClearAnchorCharRects() and ClearFocusCharRects(). ::: widget/ContentCache.cpp:279 (Diff revision 1) > + uint32_t aOffset, > + uint32_t aLength, > + RectArray& aCharRectArray) const > +{ > + for (size_t i = 0; i < aCharRectArray.Length(); i++) { > + aCharRectArray[i].SetEmpty(); If eQueryTextRectArray is sucessful, it will use move semantics. So it isn't good for sucessful. ::: widget/ContentCache.cpp:286 (Diff revision 1) > + nsEventStatus status = nsEventStatus_eIgnore; > + WidgetQueryContentEvent textRects(true, eQueryTextRectArray, aWidget); > + textRects.InitForQueryTextRectArray(aOffset, aLength); > + aWidget->DispatchEvent(&textRects, status); > + if (NS_WARN_IF(!textRects.mSucceeded)) { > + return false; Instead of calling SetEmpty() on previous, you should call aCharRectArray.Clear() on this point. ::: widget/ContentCache.cpp:307 (Diff revision 1) > mCompositionStart = UINT32_MAX; > mTextRectArray.Clear(); > - mSelection.mAnchorCharRect.SetEmpty(); > - mSelection.mFocusCharRect.SetEmpty(); > + mSelection.mAnchorCharRects[0].SetEmpty(); > + mSelection.mAnchorCharRects[1].SetEmpty(); > + mSelection.mFocusCharRects[0].SetEmpty(); > + mSelection.mFocusCharRects[1].SetEmpty(); mSelection.ClearAnchorCharRect(); mSelection.ClearFocusCharRect(); ::: widget/ContentCache.cpp:344 (Diff revision 1) > + if (mSelection.mAnchor) { > + mSelection.mAnchorCharRects[ePrevCharRect] = > + mTextRectArray.GetRect(mSelection.mAnchor - 1); > + } > } else { > - LayoutDeviceIntRect charRect; > + AutoTArray<LayoutDeviceIntRect, 2> rects; QueryCharRectArray doesn't use copy for nsTArray, stack class such as AutoTArray is unnecessary. ::: widget/ContentCache.cpp:348 (Diff revision 1) > } else { > - LayoutDeviceIntRect charRect; > - if (NS_WARN_IF(!QueryCharRect(aWidget, mSelection.mAnchor, charRect))) { > + AutoTArray<LayoutDeviceIntRect, 2> rects; > + uint32_t startOffset = mSelection.mAnchor ? mSelection.mAnchor - 1 : 0; > + uint32_t length = mSelection.mAnchor ? 2 : 1; > + if (NS_WARN_IF(!QueryCharRectArray(aWidget, startOffset, length, rects))) { > MOZ_LOG(sContentCacheLog, LogLevel::Error, mSelection.mAnchorCharRects should be clear becasue it is old data. ::: widget/ContentCache.cpp:375 (Diff revision 1) > + if (mSelection.mFocus) { > + mSelection.mFocusCharRects[ePrevCharRect] = > + mTextRectArray.GetRect(mSelection.mFocus - 1); > + } > } else { > - LayoutDeviceIntRect charRect; > + AutoTArray<LayoutDeviceIntRect, 2> rects; QueryCharRectArray uses move semantics, so Auto\* is unnecessary. ::: widget/ContentCache.cpp:379 (Diff revision 1) > } else { > - LayoutDeviceIntRect charRect; > - if (NS_WARN_IF(!QueryCharRect(aWidget, mSelection.mFocus, charRect))) { > + AutoTArray<LayoutDeviceIntRect, 2> rects; > + uint32_t startOffset = mSelection.mFocus ? mSelection.mFocus - 1 : 0; > + uint32_t length = mSelection.mFocus ? 2 : 1; > + if (NS_WARN_IF(!QueryCharRectArray(aWidget, startOffset, length, rects))) { > MOZ_LOG(sContentCacheLog, LogLevel::Error, Call ClearFocusClearRects() because mFocusCharRects is old value.

Comment on attachment 8770404 [details] Bug 1257446 part.2 ContentEventHandler::OnQueryTextRectArray() shouldn't set empty rect to the result https://reviewboard.mozilla.org/r/63882/#review61172

(In reply to Makoto Kato [:m_kato] from comment #13) > Comment on attachment 8770403 [details] > Bug 1257446 part.1 ContentCache should store previous character of selection > ::: widget/ContentCache.cpp:348 > (Diff revision 1) > > } else { > > - LayoutDeviceIntRect charRect; > > - if (NS_WARN_IF(!QueryCharRect(aWidget, mSelection.mAnchor, charRect))) { > > + AutoTArray<LayoutDeviceIntRect, 2> rects; > > + uint32_t startOffset = mSelection.mAnchor ? mSelection.mAnchor - 1 : 0; > > + uint32_t length = mSelection.mAnchor ? 2 : 1; > > + if (NS_WARN_IF(!QueryCharRectArray(aWidget, startOffset, length, rects))) { > > MOZ_LOG(sContentCacheLog, LogLevel::Error, > > mSelection.mAnchorCharRects should be clear becasue it is old data. > ::: widget/ContentCache.cpp:379 > (Diff revision 1) > > } else { > > - LayoutDeviceIntRect charRect; > > - if (NS_WARN_IF(!QueryCharRect(aWidget, mSelection.mFocus, charRect))) { > > + AutoTArray<LayoutDeviceIntRect, 2> rects; > > + uint32_t startOffset = mSelection.mFocus ? mSelection.mFocus - 1 : 0; > > + uint32_t length = mSelection.mFocus ? 2 : 1; > > + if (NS_WARN_IF(!QueryCharRectArray(aWidget, startOffset, length, rects))) { > > MOZ_LOG(sContentCacheLog, LogLevel::Error, > > Call ClearFocusClearRects() because mFocusCharRects is old value. No, they're cleared at top of the method. I think that MOZ_ASSERT() is enough instead of clear them again.

https://reviewboard.mozilla.org/r/63884/#review61214 ::: layout/generic/nsTextFrame.cpp:7321 (Diff revision 1) > + // Don't call gfxSkipCharsIterator::AdvanceOriginal() before checking if > + // current offset has already reached to the end for avoiding assertion. > + // I.e., we shouldn't call this in the increment statement of |for| nor > + // the end of this loop. > + if (i > 0) { > + iter.AdvanceOriginal(1); > + } It seems a bit confusing to do the increment here; I see that because it's done for all iterations /except/ the first, it should work as intended, but I think an alternative approach might be clearer. Looking at the loop, we're effectively iterating from `aInOffset` to `min(aInOffset + aLength, GetContentEnd())`. So instead of having the loop condition itself and then a separate test-and-break for ContentEnd, how about precomputing the end: int32_t endOffset = std::min(aInOffset + aLength, GetContentEnd()); so that we can then just loop: while (aInOffset < endOffset) { ...the loop body goes here... aInOffset++; // don't advance iter if we've reached the end if (aInOffset < endOffset) { iter.AdvanceOriginal(1); } } ISTM it would be easier to understand using something like this code structure... does that make sense to you?

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #15) > No, they're cleared at top of the method. I think that MOZ_ASSERT() is > enough instead of clear them again. OK, ignore my comments.

https://reviewboard.mozilla.org/r/63884/#review61214 > It seems a bit confusing to do the increment here; I see that because it's done for all iterations /except/ the first, it should work as intended, but I think an alternative approach might be clearer. > > Looking at the loop, we're effectively iterating from `aInOffset` to `min(aInOffset + aLength, GetContentEnd())`. So instead of having the loop condition itself and then a separate test-and-break for ContentEnd, how about precomputing the end: > > int32_t endOffset = std::min(aInOffset + aLength, GetContentEnd()); > > so that we can then just loop: > > while (aInOffset < endOffset) { > > ...the loop body goes here... > > aInOffset++; > // don't advance iter if we've reached the end > if (aInOffset < endOffset) { > iter.AdvanceOriginal(1); > } > } > > ISTM it would be easier to understand using something like this code structure... does that make sense to you? Thank you, looks better!! I'll post new patch soon.

Comment on attachment 8770402 [details] Bug 1257446 part.0 ContentCacheInParent::HandleQueryContentEvent() should log the cause of failure when it makes the event's input offset absolute Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63878/diff/1-2/

Comment on attachment 8770403 [details] Bug 1257446 part.1 ContentCache should store previous character of selection Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63880/diff/1-2/

Comment on attachment 8770404 [details] Bug 1257446 part.2 ContentEventHandler::OnQueryTextRectArray() shouldn't set empty rect to the result Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63882/diff/1-2/

Comment on attachment 8770405 [details] Bug 1257446 part.3 nsTextFrame::GetCharacterRectsInRange() shouldn't call gfxSkipCharsIterator::AdvanceOriginal() before checking if the current offset has already been reached to the end for avoiding assertions Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63884/diff/1-2/

https://reviewboard.mozilla.org/r/63884/#review61214 > Thank you, looks better!! I'll post new patch soon. > const int32_t kEndOffset = std::min(aInOffset + aLength, kContentEnd + 1); Although, the latter condition is odd because the end offset means the first character of next nsTextFrame. However, I don't touch this point here because we need to change the caller's code too. A patch for bug 1286464 will fix this.

Comment on attachment 8770405 [details] Bug 1257446 part.3 nsTextFrame::GetCharacterRectsInRange() shouldn't call gfxSkipCharsIterator::AdvanceOriginal() before checking if the current offset has already been reached to the end for avoiding assertions https://reviewboard.mozilla.org/r/63884/#review65308 LGTM. (But the last bit of the patch description should be updated, or just removed.)

https://reviewboard.mozilla.org/r/63884/#review65308 Thank you for your quick review! and for checking the old commit message too. I'll land this with the patches bug 1286464 for not blocking Nightly testers' daily life.

Comment on attachment 8770405 [details] Bug 1257446 part.3 nsTextFrame::GetCharacterRectsInRange() shouldn't call gfxSkipCharsIterator::AdvanceOriginal() before checking if the current offset has already been reached to the end for avoiding assertions Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63884/diff/2-3/

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/b66e92eb8ed3 part.0 ContentCacheInParent::HandleQueryContentEvent() should log the cause of failure when it makes the event's input offset absolute r=m_kato https://hg.mozilla.org/integration/autoland/rev/eaad665922a5 part.1 ContentCache should store previous character of selection r=m_kato https://hg.mozilla.org/integration/autoland/rev/df84d061b101 part.2 ContentEventHandler::OnQueryTextRectArray() shouldn't set empty rect to the result r=m_kato https://hg.mozilla.org/integration/autoland/rev/a3c769e95c38 part.3 nsTextFrame::GetCharacterRectsInRange() shouldn't call gfxSkipCharsIterator::AdvanceOriginal() before checking if the current offset has already been reached to the end for avoiding assertions r=jfkthame

https://hg.mozilla.org/mozilla-central/rev/b66e92eb8ed3 https://hg.mozilla.org/mozilla-central/rev/eaad665922a5 https://hg.mozilla.org/mozilla-central/rev/df84d061b101 https://hg.mozilla.org/mozilla-central/rev/a3c769e95c38

Masayuki, is it something you are going to uplift to aurora (and beta)? Thanks

No, honestly, I'd like to uplift them for better UX, but it's too risky due to bug 1286464. So, we won't request it.