840837 - test_imestate testing for MozIMEFocusOut is broken

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The test calls is_not() inside the "function onFocus" in the MozIMEFocusOut case of the switch, but there is no such function.  So that just throws an exception, which is never reported due to bug 503244 and hence the test ends up passing.

If I fix the test to call isnot(), then I get test failures.  If I comment out the entire call, the test is green but of course not testing what it presumably means to be testing...

Comment 1

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

Masayuki, could you look at this?

Comment 2

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

Yep, probably, I will have some time for this next week.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Masayuki-san, have you had a chance to look into this?

Comment 4

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

Unfortunately, no for now. This is still in my queue, but I have a lot of work now. So, I need more days for starting this.

Looks like this has to be fixed by me. How much important this for the blocking bug?

Comment 5

[Boris Zbarsky [:bzbarsky]]

That bug is blocked on other things too, for the moment, so this is not the only thing blocking it.  If we get to the point where this is the only problem left before I can land the other bug, I'll comment here.

Comment 6

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

If you don't touch nsIMEStateManager nor nsTextStateManager, then, actually it must not cause any regression about the stuff tested by the test. So, even if I failed to fix it before you need, I think that you don't mind this.

But I try to find time to work on this next week or next of it.

Comment 7

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

I got a change to fix this today. Now testing on tryserver:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=420bcd14cda9

Comment 8

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

Attachment: Patch

The causes of the failures are:

1. If the focus is moved to a design mode editor, the IME status is changed before the previous focused content loses focus because any element doesn't take focus in design mode document, which happens before the previous focused element loses focus. Therefore, when MozIMEFocusOut event is fired at blurring the previous focused element, the IME status has already been updated.

2. If the container of the elements has contenteditable="true", the test is testing the focus move from the container to one of its children. Therefore, if the new focused element's IME enabled state isn't changed from the container's state, it's doing wrong check.

So, at #1, the IME state should be enabled since nsIMEStateManager has already been changed the IME state before dispatching the MozIMEFocusOut event.

And at #2, the not changing IME state should be checked.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 728208 [details] [diff] [review]
Patch

>+            if (aFocus.toDesignModeEditor) {

aTest.toDesignModeEditor, right?

r=me with that.  Thank you!

Comment 10

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

Oh yes...

Comment 11

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

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

Comment 12

[Joe Drew (not getting mail)]

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