Closed Bug 1257446 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Platform:
Unspecified
Windows 10
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
platform-rel --- +
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- fix-optional
firefox51 --- fixed

People

(Reporter: m_kato, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: inputmethod, Whiteboard: [platform-rel-Google] [platform-rel-Gmail])

Attachments

(5 files)

screenshot
73.72 KB, image/png
Details
Bug 1257446 part.0 ContentCacheInParent::HandleQueryContentEvent() should log the cause of failure when it makes the event's input offset absolute
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1257446 part.1 ContentCache should store previous character of selection
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1257446 part.2 ContentEventHandler::OnQueryTextRectArray() shouldn't set empty rect to the result
58 bytes, text/x-review-board-request
m_kato
: review+
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
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
Attached image 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
Keywords: inputmethod
OS: Unspecified → Windows 10
Summary: Candidate window is overlapped on composition string when using Google Japanese IME on gmail composition window → Candidate window is overlapped on composition string when using Google Japanese IME on gmail compose window
Whiteboard: [platform-rel-Google] [platform-rel-Gmail]
platform-rel: --- → ?
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.
Summary: Candidate window is overlapped on composition string when using Google Japanese IME on gmail compose window → [TSF] Candidate window is overlapped on composition string when using Google Japanese IME on gmail compose window
platform-rel: ? → +
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
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.
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
Attachment #8770402 - Flags: review?(m_kato) → review+
Attachment #8770403 - Flags: review?(m_kato) → review+
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.
Attachment #8770404 - Flags: review?(m_kato) → review+
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
Attachment #8770405 - Flags: review?(m_kato) → review?(jfkthame)
(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.)
Attachment #8770405 - Flags: review?(jfkthame) → review+
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
Masayuki, is it something you are going to uplift to aurora (and beta)? Thanks
status-firefox49: --- → affected
status-firefox50: --- → affected
Flags: needinfo?(masayuki)
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.
Flags: needinfo?(masayuki)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: