478146 - [TSF] When dragging application window, candidate window is not moved with application window.

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Masahiro YAMADA]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090211 Minefield/3.2a1pre

If TSF is enabled, when dragging application window, candidate window is not moved with application window.

Step to reproduce:
1. Enable TSF
2. enable IME
3. show candidate window of IME
4. Drag application's window
5. Candidate window's position is not moved with application window.
----------
When TSF is not enabled, this bug is not reproduced.

Comment 1

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

This is minor issue, and this is not a critical issue for usability/accessibility. So, this should not block bug 478029.

Comment 2

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

this is not important, but it's easy to fix, maybe. taking.

Comment 3

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

Attachment: Patch v1.0

We need to call ITfContextOwnerServices::OnLayoutChange after that window is moved. But we cannot hook all ancestor WM_MOVE messages, unfortunately. E.g., we are embedded, so, the top level window is not ours and the window's thread and our thread may be different. For this issue, I add a WM_USER_TSF_WINDOWMOVE message. Top level windows should post the message at WM_MOVE if the window is active.

Note that I found a bug. During page loading and composing, the window moving is caused of a crash bug. However, I can reproduce it without this patch too. You should test this patch after all page loading finished.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Do we want to also check WM_SIZE, or maybe just check WM_WINDOWPOSCHANGED?

>+  // Remove all WM_USER_TSF_WINDOWMOVE messages from queue
>+  for (;;) {
>+    MSG msg;
>+    if (!::PeekMessageW(&msg, NULL, WM_USER_TSF_WINDOWMOVE,
>+                        WM_USER_TSF_WINDOWMOVE, PM_REMOVE))
>+      break;
>+  }

Can't you use while(::PeekMessageW(...?  And is it a problem if we don't remove all the messages from the queue?

Comment 5

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

Attachment: Patch v2.0

thank you for your review.

* removed PeekMessage.
* OnLayoutChange is called by WM_MOVE, WM_SIZE and WM_WINDOWPOSCHANGED.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Thank you. It looks good so far.

Do you know if we should use ITfContextOwnerServices::OnLayoutChange or ITextStoreACPSink::OnLayoutChange?

It might be better to use ITextStoreACPSink::OnLayoutChange because MSDN says ITfContextOwnerServices is for "context owners that do not implement the ITextStoreACP or ITextStoreAnchor interfaces."

Comment 7

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

Attachment: Patch v2.1

Thank you, Jim. You're right.

If you can agree to this patch, please change the flag of 'review' to '+'.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 363599 [details] [diff] [review]
Patch v2.1

Thank you!

Comment 9

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

Comment on attachment 363599 [details] [diff] [review]
Patch v2.1

roc:

This patch uses an ad hoc way. This patch defines an user message which is used for notification of the window position changing.

So, embedded gecko can have this bug if the implementers don't send this message to us.

This is not smart way, however, I don't have better idea.

Comment 10

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

I don't have any better ideas about that either.

Should we also fire this notification whenever we reflow?

Comment 11

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

(In reply to comment #10)
> Should we also fire this notification whenever we reflow?

Ugh, right. And also we need to handle scrolling.

Comment 12

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

(In reply to comment #11)
> (In reply to comment #10)
> > Should we also fire this notification whenever we reflow?
> 
> Ugh, right. And also we need to handle scrolling.

Ah, but such cases are not regression bugs of the TSF implementation, please ignore them here. I'll file a new bug for them, we also need to change IMM32 implementation too.

Comment 13

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

I don't think adding this message for embedders is very useful. They're not likely to know about it. They may not even be able to use it.

Could we just have a repeating timer running while the composition window is open, which constantly re-checks the position of the candidate window and moves it if necessary?

Comment 14

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

ok, I'll try to it.

Comment 15

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

I'm assuming that the candidate window is usually only open while the user is typing something.

Using the timer approach is a bit inefficient but it also solves the reflow and scrolling problem.

Comment 16

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

Attachment: Patch v3.0

This patch always calls OnLayoutChange by timer. But that might cause of flickering IME related windows of some third party's IMEs. If so, we need to change this to ideal implementation. But this is ok for now...

Comment 17

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

+static PRUint32
+GetLayoutChangeIntervalTime()
+{

+  sTime = PR_MIN(PR_INT32_MAX, sTime);

Ah, this line doesn't have any meanings, please ignore this.

Comment 18

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

checked-in.
http://hg.mozilla.org/mozilla-central/rev/ccf89b42c2e2