1241408 - Google docs commits composition text per input with TSF and non-e10s mode

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Makoto Kato [:m_kato]]

- Env
Windows 10
Nightly 2016-01-21 (not-e10s mode)
Google Japanese IME

- Step
1. Run Firefox with non-e10s mode
2. Open docs.google.com and new document
3. Focus document body
4. Input あああ
5. Hit [Enter] key to commit string
6. Input 'い' multiple times

- Result
When inputting 'い', this is committed.  But composing state is kept, so character is duplicated.

- Expected Result
Don't commit character until user commits it

- Note
* This issue doesn't reproduce on e10s mode
* This issue is only on Google Japanese IME with TSF mode


0[1dffb31d800]: TSF: 0x1df8ef89790 TSFTextStore::InsertTextAtSelection(dwFlags=TF_IAS_QUERYONLY, pchText=0x0 "", cch=0, pacpStart=0xa4499fde64, pacpEnd=0xa4499fde60, pChange=0xa4499fde68), IsComposing()=false
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::CurrentSelection(): acpStart=3, acpEnd=3 (length=0), reverted=false


Although inputting first composition character, acpStart of selection doesn't become 0.  If using e10s, it is always 0.


0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::InsertTextAtSelection() succeeded: *pacpStart=3, *pacpEnd=3, *pChange={ acpStart=3, acpOldEnd=3, acpNewEnd=3 })
0[1dffb31d800]: TSF: 0x1df8ef89790 TSFTextStore::InsertTextAtSelection(dwFlags=TF_IAS_QUERYONLY, pchText=0x0 "", cch=0, pacpStart=0xa4499fddc4, pacpEnd=0xa4499fddc0, pChange=0xa4499fddc8), IsComposing()=false
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::CurrentSelection(): acpStart=3, acpEnd=3 (length=0), reverted=false
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::InsertTextAtSelection() succeeded: *pacpStart=3, *pacpEnd=3, *pChange={ acpStart=3, acpOldEnd=3, acpNewEnd=3 })
0[1dffb31d800]: TSF: 0x1df8ef89790 TSFTextStore::OnStartComposition(pComposition=0x1df8c4c50a0, pfOk=0xa4499fde60), mComposition.mView=0x0
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::RecordCompositionStartAction(aComposition=0x1df8c4c50a0, aRange=0x1df8c4c3f68, aPreserveSelection=false), mComposition.mView=0x0
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::RecordCompositionStartAction(aComposition=0x1df8c4c50a0, aStart=3, aLength=0, aPreserveSelection=false), mComposition.mView=0x0
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::CurrentSelection(): acpStart=3, acpEnd=3 (length=0), reverted=false
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::GetCurrentText(): retrieving text from the content...

...

0[1dffb31d800]: TSF: 0x1df8ef89790 TSFTextStore::SetSelection(ulCount=1, pSelection=a4499fde78 { acpStart=5, acpEnd=5, style={ ase=TS_AE_END, fInterimChar=false } }), mComposition.IsComposing()=true
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::SetSelectionInternal(pSelection={ acpStart=5, acpEnd=5, style={ ase=TS_AE_END, fInterimChar=false} }, aDispatchCompositionChangeEvent=true), mComposition.IsComposing()=true
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::CurrentSelection(): acpStart=6, acpEnd=6 (length=0), reverted=false
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::RestartCompositionIfNecessary(aRangeNew=0x0), mComposition.mView=0x1df8c4c50a0
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::RestartCompositionIfNecessary(), range=3-6, mComposition={ mStart=3, mString.Length()=5 }
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::CurrentSelection(): acpStart=6, acpEnd=6 (length=0), reverted=false
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::RestartComposition(aCompositionView=0x1df8c4c50a0, aNewRange=0x1df8c4c4a48 { newStart=3, newLength=3 }), mComposition={ mStart=3, mCompositionString.Length()=5 }, currentSelection={ IsDirty()=false, StartOffset()=6, Length()=0 }
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::CurrentSelection(): acpStart=6, acpEnd=6 (length=0), reverted=false
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::LockedContent(): mLockedContent={ mText="いいいいい" (Length()=5), mLastCompositionString="いい" (Length()=2), mMinTextModifiedOffset=5 }
0[1dffb31d800]: TSF: 0x1df8ef89790   TSFTextStore::RecordCompositionEndAction(), mComposition={ mView=0x1df8c4c50a0, mString="いいいいい" }

Record compositionend unfortunately, so compositionend is dispatched

Comment 1

[Makoto Kato [:m_kato]]

selection is changed, but we don't mark dirty.  So we should mark as dirty even if TSF has read lock.

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Mark dirty for selection

Comment 3

[Makoto Kato [:m_kato]]

Comment on attachment 8711529 [details] [diff] [review]
Mark dirty for selection

When using Google Docs with TSF/Google IME, we cannot input composing text well.

When starting composition, we use current selection to recognize start position of composition, but it isn't valid on Google docs.  After committed string, although selection will changed, selection cache into TSFTextStore isn't updated.

Since it is during read lock, OnSelectionChangeInternal will return without setting selection.  So we should mark dirty even if locked.

This issue won't occur on e10s.  Because OnSelection is from child process, so we don't have read lock on this situation.

Comment 4

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

Comment on attachment 8711529 [details] [diff] [review]
Mark dirty for selection

I'm not sure if this is right approach since mSelection is assumed that it isn't dirty during mLockedContent is valid.

If you just remove this early return, how does it work? Currently we support putting off to notify TSF of selection change until unlocking the document. This feature was implemented after this block had been written. I *guess* it works well for this case.

Comment 5

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #4)
> Comment on attachment 8711529 [details] [diff] [review]
> Mark dirty for selection
> 
> I'm not sure if this is right approach since mSelection is assumed that it
> isn't dirty during mLockedContent is valid.
> 
> If you just remove this early return, how does it work? Currently we support
> putting off to notify TSF of selection change until unlocking the document.
> This feature was implemented after this block had been written. I *guess* it
> works well for this case.

It works fine even if removing return by read lock.

Comment 6

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

Then, please do that. I think that that is better approach.

Comment 7

[Makoto Kato [:m_kato]]

Attachment: Update selection cache even if having read lock

Remove read lock check on OnSelectionChange

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1280633aeae

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e1280633aeae

Comment 10

[Makoto Kato [:m_kato]]

Comment on attachment 8711599 [details] [diff] [review]
Update selection cache even if having read lock

Approval Request Comment
[Feature/regressing bug #]:
Bug 478029

[User impact if declined]:
All Google docs users that use Google Japanese IME.  Users cannot input character via Japanese IME well.  Firefox 45 is also ESR, so we should fix this for corporate users.

[Describe test coverage new/current, TreeHerder]:
landed in m-c

[Risks and why]: 
Low.  This removes unnecessary readonly check for TSF.

[String/UUID change made/needed]:
N/A

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8711599 [details] [diff] [review]
Update selection cache even if having read lock

Improve the situation for JP users. Taking it

Should be in 45 beta 2.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/564af02081f0

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/12dbb5c532d6