801989 - TextComposition needs to get identifier of native IME context for managing composition per it

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

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

TextComposition should manage composition per IME native context instead of per widget.

Windows, Mac and GTK have native IME context we can use the pointer.

Qt and Android have only one IME context per our process.

I'm not sure about b2g, but it should be the latter case.

I'm planning to add a void* member to InputContext. GetInputContext() implementation should set it if the platform supports multiple IME context. Otherwise, set it nullptr.

Comment 1

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

Attachment: part.1 Add mNativeIMEContext to InputContext and TextComposition should use it instead of nsIWidget

One composition must be available per native IME context which can be shared by multiple native widgets. Currently, TextComposition and nsIMEStateManager manage it per widget. So, if we will allow to move composition across native widgets, it becomes problem.

We can use opaque pointer as identifier of native IME context. This patch adds mNativeIMEContext to the InputContext struct. It can be gotten with nsIWidget::GetInputContext(). See following patches for the widget side implementation.

Comment 2

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

Attachment: part.2 Set InputContext::mNativeIMEContext on Windows

Comment 3

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

Attachment: part.3 Set InputContext::mNativeIMEContext on Linux

Comment 4

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

Attachment: part.4 Set InputContext::mNativeIMEContext on Mac

Comment 5

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

Attachment: part.5 Set InputContext::mNativeIMEContext on Android

Comment 6

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

Attachment: part.6 Set InputContext::mNativeIMEContext on OS/2

Comment 7

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

Attachment: part.7 Set InputContext::mNativeIMEContext on B2G

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 672607 [details] [diff] [review]
part.1 Add mNativeIMEContext to InputContext and TextComposition should use it instead of nsIWidget


> TextComposition::TextComposition(nsPresContext* aPresContext,
>                                  nsINode* aNode,
>                                  nsGUIEvent* aEvent) :
>   mPresContext(aPresContext), mNode(aNode),
>-  // temporarily, we should assume that one native IME context is per native
>-  // widget.
>-  mNativeContext(aEvent->widget),
>+  mNativeContext(aEvent->widget->GetInputContext().mNativeIMEContext),
Hmm, if GetInputContext() never returns null, could we name it 
InputContext().
Could be done in a followup bug.

Comment 9

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

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 672607 [details] [diff] [review]
> part.1 Add mNativeIMEContext to InputContext and TextComposition should use
> it instead of nsIWidget
> 
> 
> > TextComposition::TextComposition(nsPresContext* aPresContext,
> >                                  nsINode* aNode,
> >                                  nsGUIEvent* aEvent) :
> >   mPresContext(aPresContext), mNode(aNode),
> >-  // temporarily, we should assume that one native IME context is per native
> >-  // widget.
> >-  mNativeContext(aEvent->widget),
> >+  mNativeContext(aEvent->widget->GetInputContext().mNativeIMEContext),
> Hmm, if GetInputContext() never returns null, could we name it 
> InputContext().
> Could be done in a followup bug.

Hmm, then, the pair should be SetInputContext() and InputContext()?

Comment 10

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

Comment on attachment 672608 [details] [diff] [review]
part.2 Set InputContext::mNativeIMEContext on Windows

When IME is disabled, we disassociate the IMC from window. When IME is enabled, we associate the default IMC from the window. So, we cannot get the context at GetInputContext() if IME is disabled.

This patch stores the default IMC at creating the window. And if SetInputContext() cannot get IMC for the window, it's not replacing the stored IMC. Otherwise, using the latest IMC. Then, GetInputContext() always returns the stored IMC.

Comment 11

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

Comment on attachment 672610 [details] [diff] [review]
part.4 Set InputContext::mNativeIMEContext on Mac

Fortunately, we can use NSView's inputContext for getting the input manager's pointer (available on 10.6 and later!). All of the values are same in each top level window.

Comment 12

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

Comment on attachment 672611 [details] [diff] [review]
part.5 Set InputContext::mNativeIMEContext on Android

I think that there is only one IME context in a process on Android. I.e., there are never two or more composition strings at a time. Then, we should set nullptr.

Otherwise, we need to set the pointer of IME context for the widget.

Comment 13

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

Comment on attachment 672613 [details] [diff] [review]
part.7 Set InputContext::mNativeIMEContext on B2G

I guess that b2g only supports only one composition in a process, isn't it? If so, we should set nullptr to the mNativeIMEContext. Otherwise, we need to set native IME context pointer (e.g., if one composition is available per widget, we should set the pointer of the widget).

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 672613 [details] [diff] [review]
part.7 Set InputContext::mNativeIMEContext on B2G

Nit: s/b2g/gonk/.  B2G is a product, like fennec.  Gonk is one of the operating systems that b2g runs on.

I'm not really sure what this patch is doing, but gonk doesn't have any native IMEs and gecko only creates one widget there, so this seems fine.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> Hmm, then, the pair should be SetInputContext() and InputContext()?
That would be ok. Especially in content/ and dom/ code
methods which start with Get* (and are not from .idl interfaces) can return null, but
without Get prefix it is guaranteed to be non-null.

Comment 16

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

(In reply to Olli Pettay [:smaug] from comment #15)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> > Hmm, then, the pair should be SetInputContext() and InputContext()?
> That would be ok. Especially in content/ and dom/ code
> methods which start with Get* (and are not from .idl interfaces) can return
> null, but
> without Get prefix it is guaranteed to be non-null.

Even if it returns reference? (not pointer)

Comment 17

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> (In reply to Olli Pettay [:smaug] from comment #15)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> > > Hmm, then, the pair should be SetInputContext() and InputContext()?
> > That would be ok. Especially in content/ and dom/ code
> > methods which start with Get* (and are not from .idl interfaces) can return
> > null, but
> > without Get prefix it is guaranteed to be non-null.
> 
> Even if it returns reference? (not pointer)

Er, it's not reference. It returns a copy of the struct.

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/19a8337465ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bb7b517fcb3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7da33982b8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/80bb0b88086f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e748a6c7216
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d2392ef0082
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0c4f4bd8a9d

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a0c4f4bd8a9d
https://hg.mozilla.org/mozilla-central/rev/8d2392ef0082
https://hg.mozilla.org/mozilla-central/rev/6e748a6c7216
https://hg.mozilla.org/mozilla-central/rev/80bb0b88086f
https://hg.mozilla.org/mozilla-central/rev/d7da33982b8a
https://hg.mozilla.org/mozilla-central/rev/6bb7b517fcb3
https://hg.mozilla.org/mozilla-central/rev/19a8337465ac

Comment 20

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

> Flags: in-testsuite?

It's impossible to test this now because our editor commits composition forcibly when it's losing focus. This fix will be necessary when we stop committing composition forcibly at that time.

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 672607 [details] [diff] [review]
part.1 Add mNativeIMEContext to InputContext and TextComposition should use it instead of nsIWidget

Review of attachment 672607 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/PBrowser.ipdl
@@ +156,5 @@
>       */
>      sync EndIMEComposition(bool cancel) returns (nsString composition);
>  
> +    sync GetInputContext() returns (int32_t IMEEnabled, int32_t IMEOpen,
> +                                    int64_t NativeIMEContext);

I guess we can't use intptr_t here?

Comment 22

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

Oh, right. I didn't know it. I'll file a followup bug with a patch.

Comment 23

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

filed bug 806300 for s/int64_t/intptr_t