460059 - Need IME state testing

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

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

Now, we manage the IME state when focused content is changed. For IME users, we should test the behavior in automated test.

For this test, we need to be able to get the IME state of the current focused content from Javascript.

I think that we should add new properties to nsIDOMWindowUtils. They are 'IMEOpenedState' and 'IMEEnabledState'. I was thinking these properties should be readonly. However, some Japanese add-on developers hope that they can control IME state without focus changing by the properties too. This makes happy the CJK users. I agree to the opinion of the developers. So, I try to implement it and create the tests.

Comment 1

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

Attachment: Patch v0.1 (work in progress)

The draft for this test.

Unfortunately, I found a bug during contenteditable testing :-(
It should be fixed after this bug.

Comment 2

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

Note for myself.

1. Add designMode test.
2. Add type attribute of input element changing by JS case.
3. Clean up the logging code.

Comment 3

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

Attachment: Patch v0.9 (work in progress)

Comment 4

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

Attachment: Patch v1.0

This patch should be final. Now, testing on tryserver...

Comment 5

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

Ugh, the latest patch fails creashtests. But looks like that the cause is not my patch :-(

Comment 6

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

stack trace of the crash on my environement:

>>	gklayout.dll!nsWeakView::Clear()  行 412 + 0x3 バイト	C++
>  	gklayout.dll!nsWeakView::Clear()  行 415	C++
>  	gklayout.dll!nsWeakView::Clear()  行 415	C++
>  	gklayout.dll!nsWeakView::Clear()  行 415	C++
>  	gklayout.dll!nsWeakView::Clear()  行 415	C++
>  	gklayout.dll!nsView::~nsView()  行 280	C++
>  	gklayout.dll!nsView::`scalar deleting destructor'()  + 0xf バイト	C++
>  	gklayout.dll!nsIView::Destroy()  行 313 + 0x21 バイト	C++
>  	gklayout.dll!nsFrame::Destroy()  行 503	C++
>  	gklayout.dll!nsSplittableFrame::Destroy()  行 74	C++
>  	gklayout.dll!nsContainerFrame::Destroy()  行 309	C++
>  	gklayout.dll!ViewportFrame::Destroy()  行 69	C++
>  	gklayout.dll!nsFrameManager::Destroy()  行 291	C++
>  	gklayout.dll!PresShell::Destroy()  行 1843	C++
>  	gklayout.dll!DocumentViewerImpl::DestroyPresShell()  行 4255	C++
>  	gklayout.dll!DocumentViewerImpl::Destroy()  行 1533	C++
>  	docshell.dll!nsSHistory::EvictContentViewersInRange(int aStart=0x0000002d, int aEnd=0x0000002e)  行 884	C++
>  	docshell.dll!nsSHistory::EvictWindowContentViewers(int aFromIndex=0x00000030, int aToIndex=0x00000031)  行 849	C++
>  	docshell.dll!nsSHistory::EvictContentViewers(int aPreviousIndex=0x00000030, int aIndex=0x00000031)  行 663	C++
>  	gklayout.dll!DocumentViewerImpl::Show()  行 1859	C++
>  	gklayout.dll!nsPresContext::EnsureVisible(int aUnsuppressFocus=0x00000000)  行 1575	C++
>  	gklayout.dll!PresShell::UnsuppressAndInvalidate()  行 4437 + 0xd バイト	C++
>  	gklayout.dll!PresShell::UnsuppressPainting()  行 4486	C++
>  	gklayout.dll!DocumentViewerImpl::LoadComplete(unsigned int aStatus=0x00000000)  行 1037	C++
>  	docshell.dll!nsDocShell::EndPageLoad(nsIWebProgress * aProgress=0x05f61194, nsIChannel * aChannel=0x06b19b38, unsigned int aStatus=0x00000000)  行 5264	C++
>  	docshell.dll!nsWebShell::EndPageLoad(nsIWebProgress * aProgress=0x05f61194, nsIChannel * channel=0x06b19b38, unsigned int aStatus=0x00000000)  行 985	C++
>  	docshell.dll!nsDocShell::OnStateChange(nsIWebProgress * aProgress=0x05f61194, nsIRequest * aRequest=0x06b19b38, unsigned int aStateFlags=0x00020010, unsigned int aStatus=0x00000000)  行 5160	C++
>  	docshell.dll!nsDocLoader::FireOnStateChange(nsIWebProgress * aProgress=0x05f61194, nsIRequest * aRequest=0x06b19b38, int aStateFlags=0x00020010, unsigned int aStatus=0x00000000)  行 1260	C++
>  	docshell.dll!nsDocLoader::doStopDocumentLoad(nsIRequest * request=0x06b19b38, unsigned int aStatus=0x00000000)  行 891	C++
>  	docshell.dll!nsDocLoader::DocLoaderIsEmpty(int aFlushLayout=0x00000001)  行 787	C++
>  	docshell.dll!nsDocLoader::OnStopRequest(nsIRequest * aRequest=0x06c98858, nsISupports * aCtxt=0x00000000, unsigned int aStatus=0x00000000)  行 683	C++
>  	necko.dll!nsLoadGroup::RemoveRequest(nsIRequest * request=0x06c98858, nsISupports * ctxt=0x00000000, unsigned int aStatus=0x00000000)  行 680 + 0x2e バイト	C++
>  	gklayout.dll!nsDocument::DoUnblockOnload()  行 7023	C++
>  	gklayout.dll!nsDocument::UnblockOnload(int aFireSync=0x00000001)  行 6969	C++
>  	gklayout.dll!nsDocument::DispatchContentLoadedEvents()  行 3913	C++
>  	gklayout.dll!nsRunnableMethod<nsDocument,void>::Run()  行 265	C++
>  	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=0x00000001, int * result=0x003bf4dc)  行 511	C++
>  	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x02675cf0, int mayWait=0x00000001)  行 230 + 0x16 バイト	C++
>  	gkwidget.dll!nsBaseAppShell::Run()  行 170 + 0xc バイト	C++
>  	tkitcmps.dll!nsAppStartup::Run()  行 192 + 0x1c バイト	C++
>  	xul.dll!XRE_main(int argc=0x00000006, char * * argv=0x026682b0, const nsXREAppData * aAppData=0x02672d08)  行 3340 + 0x25 バイト	C++
>  	firefox.exe!NS_internal_main(int argc=0x00000006, char * * argv=0x026682b0)  行 156 + 0x12 バイト	C++
>  	firefox.exe!wmain(int argc=0x00000006, wchar_t * * argv=0x0266b780)  行 110 + 0xd バイト	C++
>  	firefox.exe!__tmainCRTStartup()  行 583 + 0x19 バイト	C
>  	firefox.exe!wmainCRTStartup()  行 403	C
>  	kernel32.dll!762be4a5() 	
>  	[下のフレームは間違っているか、または見つかりません。kernel32.dll に対して読み込まれたシンボルはありません。]	
>  	ntdll.dll!76fecfed() 	
>  	ntdll.dll!76fed1ff() 	

This crash happened in:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/crashtests/407256-1.html?force=1

This is not same as the crash on tryserver...

Comment 7

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

roc:

See comment 4 - 6.
Do you have idea of the crash by my patch? The crash happened most times. However, sometimes the crash didn't happen on the same test, but the crash tests not finished correctly. Looks like my patch found random crash test failure.

I hope that this tests will be landed before focus refactoring...

Comment 8

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

The crash is not occurred on Linux and Mac of tryserver.

I found an assertion on my environment:
> ###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file t:\mozilla-c\firefox-debug-build\dist\include\xpcom\nsCOMPtr.h, line 796

This assertion is fired during |editor/libeditor/html/crashtests/456727-2.html|. But I cannot get the stack trace at this time. I also cannot crash at this time by following code:

>       operator->() const
>         {
>           NS_PRECONDITION(mRawPtr != 0, "You can't dereference a NULL nsCOMPtr with operator->().");
>           if (!mRawPtr) {
>             int y = 0;
>             int x = 300 / y;
>             printf("%d", x);
>           }
>           return get();
>         }

It's strange...

Comment 9

[timeless]

imagine you had this:

class A {
b() { };
};

int main () {
A *a  = 0;
a->b();
return 0;
}

obviously, you'll crash at a->b();

the thing is that what we do is:

class nsIA {
b() { };
};


int main () {
nsCOMPtr<nsIA> a = 0;
a->b();
return 0;
}

because a is a real object and not a pointer, we need to define operator -> so that we give out the underlying pointer (0).

But, when we do that, we know you're going to crash, just as you do in the original case. But we're nice and we give you an assertion so you can set a breakpoint on the assertion (set XPCOM_DEBUG_BREAK=break), otherwise you're likely to not be able to debug it....

http://www.mozilla.org/projects/xpcom/nsCOMPtr/
https://developer.mozilla.org/en/Using_nsCOMPtr
https://developer.mozilla.org/en/XPCOM_DEBUG_BREAK

Comment 10

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

Thank you timeless, I succeeded to break and getting stack trace at that time.

By the stack trace, looks like the cause of the crash is same as (or similar to) bug 487601.

Comment 11

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

Attachment: Patch v2.0

ok, this should work fine. I'll test this patch on tryserver.

Comment 12

[Martijn Wargers (dead)]

(In reply to comment #7)
> See comment 4 - 6.
> Do you have idea of the crash by my patch? The crash happened most times.
> However, sometimes the crash didn't happen on the same test, but the crash
> tests not finished correctly. Looks like my patch found random crash test
> failure.

I just filed bug 488779, maybe that bug is related to the crash you were seeing there?

Comment 13

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

Martijn, thank you for your information. But maybe they are not related. The crash happened in mDeletionObserver->Clear(); of #278. The memory of the nsWeakView was broken. Fortunately, the latest patch fixes completely the crash, probably.

Comment 14

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

Attachment: Patch v2.0 (excepting the part of bug 487601)

ok, we should land this tests after bug 487601's patch. This tests test IME enabled state of each input/textarea and other elements under normal/contentEditable/designMode.

Unfortunately, the IME open state tests don't work on tinderbox machines. Because they are not installed IMEs. I need to think better IME open state controlling on Win32 which is not using IMM32 APIs, then, we may be able to test that on tinderbox machines.

Comment 15

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

The changes in nsWindow is for TSF disabled case. When TSF is disabled, nsTextStore::OnFocusChange returns NS_ERROR_NOT_AVAILABLE. 
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsTextStore.cpp#1348
So, we should not return error code to the caller at that time. Because the caller outputs very many warnings by NS_ENSURE_SUCCESS.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  /**
+   * Get/Set IME enabled.
+   */
+  attribute unsigned long IMEEnabled;

Shouldn't this be IMEStatus?

+  var contanier = document.getElementById("display");

container

Do we really need to be able to set IMEEnabled? How about IMEOpenState? As long as nsIWidget::SetIMEEnabled and nsIWidget::SetOpenState are exercised by some test, I'd prefer not to add code to nsIDOMWindowUtils just so we can write more specific tests for them.

Comment 17

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

(In reply to comment #16)
> +  /**
> +   * Get/Set IME enabled.
> +   */
> +  attribute unsigned long IMEEnabled;
> 
> Shouldn't this be IMEStatus?

Why?

> Do we really need to be able to set IMEEnabled? How about IMEOpenState?

I thought that they are not needed, but some Japanese extension developers want the feature. Because ime-mode of CSS cannot control the IME state without focus changing.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #17)
> (In reply to comment #16)
> > +  /**
> > +   * Get/Set IME enabled.
> > +   */
> > +  attribute unsigned long IMEEnabled;
> > 
> > Shouldn't this be IMEStatus?
> 
> Why?

Because a property named "enabled" implies that it will be true or false. Here you have four possible values, so it's not just "enabled" or "not enabled", it's a status.

> > Do we really need to be able to set IMEEnabled? How about IMEOpenState?
> 
> I thought that they are not needed, but some Japanese extension developers want
> the feature. Because ime-mode of CSS cannot control the IME state without focus
> changing.

We shouldn't add new API for extension developers in a bug about testing. We should have a separate bug describing what they want and then we can talk about whether we should provide it and what the best API would be.

Comment 19

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

ok, I see. I'll post a new patch.

Comment 20

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

Attachment: Patch v3.0

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 375757 [details] [diff] [review]
Patch v3.0

+  readonly attribute boolean IMEOpenState;

Call this IMEIsOpen

Comment 22

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

Attachment: Patch v3.1

Comment 23

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

http://hg.mozilla.org/mozilla-central/rev/7ef08c530517

-> FIXED