960836 - (e10s-ime-tsf) [meta] [TSF] TSF support on e10s

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Makoto Kato [:m_kato]]

Even if landed bug 960493, each character seems to be always committed, not keeping composition state.  This is TSF+e10s only issue.

Comment 1

[Makoto Kato [:m_kato]]

If OnTextChangeMsg is called, text is committed, not keep composition state.

Non-e10s, OnTextChangeInternal is called from composition/text event. So when OnTextChangeInternal is called, document is always locked since FlushPendingActions is called when locked.

OnTextChangeInternal is useless now on non-e10s...

Comment 2

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

(In reply to Makoto Kato (:m_kato) from comment #1)
> If OnTextChangeMsg is called, text is committed, not keep composition state.

If it's caused by our editor, it might be fixed in the future. Otherwise, it may depend on TIP.

> Non-e10s, OnTextChangeInternal is called from composition/text event. So
> when OnTextChangeInternal is called, document is always locked since
> FlushPendingActions is called when locked.

Text change notifications have to be ignored if the change is caused by TIP's composition. Therefore, while document is locked, OnTextChangeInternal() does nothing. However, if it's called by a change of editor content caused by JS or without composition (e.g., direct input from keyboard), we need to notify TSF of text change.

Comment 3

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #2)
> (In reply to Makoto Kato (:m_kato) from comment #1)
> > If OnTextChangeMsg is called, text is committed, not keep composition state.
> 
> If it's caused by our editor, it might be fixed in the future. Otherwise, it
> may depend on TIP.

When having composition, if we calls mSink->OnTextChange, it seems to be composition information into TIP will be broken.  Then, TIP doesn't work after that.  

- Microsoft IME commits composition string by abandon state.
- Google IME and ATOK becomes TIP doesn't work well.

> > Non-e10s, OnTextChangeInternal is called from composition/text event. So
> > when OnTextChangeInternal is called, document is always locked since
> > FlushPendingActions is called when locked.
> 
> Text change notifications have to be ignored if the change is caused by
> TIP's composition. Therefore, while document is locked,
> OnTextChangeInternal() does nothing. However, if it's called by a change of
> editor content caused by JS or without composition (e.g., direct input from
> keyboard), we need to notify TSF of text change.

On e10s, DispatchEvent is asynchronized, so OnTextChangedInternal is called by unlocked document and having composition.  Then, it always hits debug assertion.

Maybe, we should check composition state on WinIMEHandler.cpp to call OnTextChangedInternal.

This situation is same as OnSelectionChange.

Comment 4

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

(In reply to Makoto Kato (:m_kato) from comment #3)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #2)
> > (In reply to Makoto Kato (:m_kato) from comment #1)
> > > If OnTextChangeMsg is called, text is committed, not keep composition state.
> > 
> > If it's caused by our editor, it might be fixed in the future. Otherwise, it
> > may depend on TIP.
> 
> When having composition, if we calls mSink->OnTextChange, it seems to be
> composition information into TIP will be broken.  Then, TIP doesn't work
> after that.  
> 
> - Microsoft IME commits composition string by abandon state.
> - Google IME and ATOK becomes TIP doesn't work well.

> This situation is same as OnSelectionChange.

I'm thinking that, anyway, we need to create a wall for preventing the critical state.

> > > Non-e10s, OnTextChangeInternal is called from composition/text event. So
> > > when OnTextChangeInternal is called, document is always locked since
> > > FlushPendingActions is called when locked.
> > 
> > Text change notifications have to be ignored if the change is caused by
> > TIP's composition. Therefore, while document is locked,
> > OnTextChangeInternal() does nothing. However, if it's called by a change of
> > editor content caused by JS or without composition (e.g., direct input from
> > keyboard), we need to notify TSF of text change.
> 
> On e10s, DispatchEvent is asynchronized, so OnTextChangedInternal is called
> by unlocked document and having composition.  Then, it always hits debug
> assertion.

I've concerned about it and hoped I'm wrong, though...

If we add a new flag to nsIMEUpdatePreference.mWantUpdates, e.g., IGNORE_CHANGES_BY_COMPOSITION and nsTextStateManager can ignore any changes during dispatching composition/text events. I think that this improves running performance during composition.

I think that we should do this, first. Should I do it in another bug?

However, even while there is composition, text change and selection change may occur by JS. We need to care this case.

> Maybe, we should check composition state on WinIMEHandler.cpp to call
> OnTextChangedInternal.

No. It should be done in nsTextStore because MetroWidget directly communicates with nsTextStore.

Comment 5

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

FYI: I'd like to enable TSF mode in default settings only on Nightly in next cycle (34). Check bug 1037328 for that.

Comment 6

[Jim Mathies [:jimm]]

removing this from m2, we are blocking on individual bugs.

Comment 7

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

Moving all open keyboard/IME handling bugs to DOM: UI Events & Focus Handling component.

Comment 8

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

The remaining open bugs are trivial issues and we don't have any pending big task for supporting TSF anymore. So I think that we can close this meta bug.