1257446 - [TSF] Candidate window is overlapped on composition string when using Google Japanese IME on gmail compose window

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Makoto Kato [:m_kato]]

Attachment: screenshot

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

Comment 1

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

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.)

Comment 2

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

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

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cc611640ee7

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e82dfb0cd996

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc504a0e1693

Comment 6

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

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.

Comment 7

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

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.

Comment 8

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

Attachment: Bug 1257446 part.0 ContentCacheInParent::HandleQueryContentEvent() should log the cause of failure when it makes the event's input offset absolute

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

Comment 9

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

Attachment: Bug 1257446 part.1 ContentCache should store previous character of selection

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/

Comment 10

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

Attachment: Bug 1257446 part.2 ContentEventHandler::OnQueryTextRectArray() shouldn't set empty rect to the result

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/

Comment 11

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

Attachment: 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

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 12

[Makoto Kato [:m_kato]]

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 13

[Makoto Kato [:m_kato]]

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 14

[Makoto Kato [:m_kato]]

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

Comment 15

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

(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.

Comment 16

[Jonathan Kew [:jfkthame]]

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?

Comment 17

[Makoto Kato [:m_kato]]

(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.

Comment 18

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

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 19

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56df64e983af

Comment 20

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01e4fa1d5cd8

Comment 21

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40d20bc209e8

Comment 22

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9aa27ac54db

Comment 23

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

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 24

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

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 25

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

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 26

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

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/

Comment 27

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

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 28

[Jonathan Kew [:jfkthame]]

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.)

Comment 29

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

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 30

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

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/

Comment 31

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

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/2-3/

Comment 32

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

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/2-3/

Comment 33

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

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/2-3/

Comment 34

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

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/3-4/

Comment 35

[Pulsebot]

Pushed by [e-mail]:
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

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 37

[Sylvestre Ledru [:Sylvestre]]

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

Comment 38

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

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.