Closed Bug 478146 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Widget: Win32, defect)

x86
Windows XP
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: masa141421356, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(1 file, 3 obsolete files)

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.
This is minor issue, and this is not a critical issue for usability/accessibility. So, this should not block bug 478029.
Blocks: 88831
No longer blocks: 478029
this is not important, but it's easy to fix, maybe. taking.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attached patch Patch v1.0 (obsolete) — Splinter Review
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.
Attachment #363059 - Flags: review?(chenn)
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?
Attached patch Patch v2.0 (obsolete) — Splinter Review
thank you for your review.

* removed PeekMessage.
* OnLayoutChange is called by WM_MOVE, WM_SIZE and WM_WINDOWPOSCHANGED.
Attachment #363059 - Attachment is obsolete: true
Attachment #363276 - Flags: review?(chenn)
Attachment #363059 - Flags: review?(chenn)
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."
Attached patch Patch v2.1 (obsolete) — Splinter Review
Thank you, Jim. You're right.

If you can agree to this patch, please change the flag of 'review' to '+'.
Attachment #363276 - Attachment is obsolete: true
Attachment #363599 - Flags: review?(chenn)
Attachment #363276 - Flags: review?(chenn)
Comment on attachment 363599 [details] [diff] [review]
Patch v2.1

Thank you!
Attachment #363599 - Flags: review?(chenn) → review+
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.
Attachment #363599 - Flags: superreview?(roc)
I don't have any better ideas about that either.

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

Ugh, right. And also we need to handle scrolling.
(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.
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?
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.
Attached patch Patch v3.0Splinter Review
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...
Attachment #363599 - Attachment is obsolete: true
Attachment #363708 - Flags: superreview?(roc)
Attachment #363599 - Flags: superreview?(roc)
+static PRUint32
+GetLayoutChangeIntervalTime()
+{

+  sTime = PR_MIN(PR_INT32_MAX, sTime);

Ah, this line doesn't have any meanings, please ignore this.
Attachment #363708 - Flags: superreview?(roc) → superreview+
checked-in.
http://hg.mozilla.org/mozilla-central/rev/ccf89b42c2e2
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.