492233 - [IMM32] Reimplement IME mouse handling

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

Attachment: Patch v1.0

The current IME mouse handling of IMM32 is very rough. nsIMM32Handler caches the character rects. And that is computed from caret position at inputting the characters. So, the cached rects are broken by scrolling, converting the composition string, etc...

So, we need to query the character at the mouse cursor directly.

Comment 1

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

Attachment: Patch v1.1

fix a bug
# this is not related to the native part.

Comment 2

[Masatoshi Kimura [:emk]]

Comment on attachment 376662 [details] [diff] [review]
Patch v1.1

>    nsIntRect cursorPosition;
>    ResolveIMECaretPos(event.theReply.mReferenceWidget,
>                       event.theReply.mCursorPosition, aWindow, cursorPosition);
> -
> -#ifdef ENABLE_IME_MOUSE_HANDLING
> -  memset(mCompCharPos, -1, sizeof(RECT) * IME_MAX_CHAR_POS);
> -  mCompCharPos[0].left = cursorPosition.x;
> -  mCompCharPos[0].top = cursorPosition.y;
> -  mCompCharPos[0].bottom = cursorPosition.YMost();
> -#endif // ENABLE_IME_MOUSE_HANDLING
>  }
Is ResolveIMECaretPos still required? cursorPosition is no longer referred at all.

>    nsIntRect cursorPosition;
>    ResolveIMECaretPos(event.theReply.mReferenceWidget,
>                       event.theReply.mCursorPosition, aWindow, cursorPosition);
> -
> -#ifdef ENABLE_IME_MOUSE_HANDLING
> -  if (mCursorPosition <= 0 || mCursorPosition >= IME_MAX_CHAR_POS) {
> -    return;
> -  }
(snip)
> -#endif // ENABLE_IME_MOUSE_HANDLING
>  }
Same as the above.

r=me with these nits fixed. Plaease ask someone else for additional review of layout/content part. I will not take responsibility for non-IMM32 part.

Comment 3

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

Attachment: Patch v1.2

Thank you, Kimura-san.

Roc, would you review the XP part and sr?

Comment 4

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

Comment on attachment 376743 [details] [diff] [review]
Patch v1.2

+  nsPoint ptInTarget =
+    nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, targetFrame);

Better to just use ptInRoot - targetFrame->GetOffsetTo(rootFrame)

Comment 5

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

Have we got any tests for this stuff?

Can we test the new event using the TSF test code?

Comment 6

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

(In reply to comment #5)
> Can we test the new event using the TSF test code?

Maybe, yes. We can get the target textrect by NS_QUERY_TEXT_RECT. But TSF implementation is bug 492394.

Comment 7

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

(In reply to comment #6)
> (In reply to comment #5)
> > Can we test the new event using the TSF test code?
> 
> Maybe, yes. We can get the target textrect by NS_QUERY_TEXT_RECT. But TSF
> implementation is bug 492394.

Ah, but just the event test can be included to TestWinTSF.cpp.

Comment 8

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

Attachment: Patch v1.2 + tests

adding tests.

Comment 9

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

+    // Note that charRect might be infrated at rounding to pixels!

You mean "inflated"

+  nsCOMPtr<nsIWidget> widget;
+  nsCOMPtr<nsIDocShell> docShell;
+  nsresult nsr = mWindow->GetDocShell(getter_AddRefs(docShell));
+  if (NS_SUCCEEDED(nsr) && docShell) {
+    nsCOMPtr<nsIPresShell> presShell;
+    nsr = docShell->GetPresShell(getter_AddRefs(presShell));
+    if (NS_SUCCEEDED(nsr) && presShell) {
+      nsCOMPtr<nsIViewManager> viewManager = presShell->GetViewManager();
+      if (viewManager) {
+        nsr = viewManager->GetWidget(getter_AddRefs(widget));
+      }
+    }
+  }

It would be better if you got the widget some other way because these APIs are going to change or go away. The widget created in Init() would be good if it works.

Comment 10

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

roc:

Unfortunately, the main widget cannot be used for the tests.

>   nsCOMPtr<nsIBaseWindow> baseWindow(do_QueryInterface(mWindow));
>   NS_ENSURE_TRUE(baseWindow, NS_ERROR_UNEXPECTED);
>   nsCOMPtr<nsIWidget> widget;
>   nsresult rv = baseWindow->GetMainWidget(getter_AddRefs(widget));
>   NS_ENSURE_TRUE(widget, NS_ERROR_UNEXPECTED);

Comment 11

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

Attachment: Patch v1.2 + tests

only updating the comment.

Comment 12

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

(In reply to comment #10)
> Unfortunately, the main widget cannot be used for the tests.

Why not?

Comment 13

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

(In reply to comment #12)
> (In reply to comment #10)
> > Unfortunately, the main widget cannot be used for the tests.
> 
> Why not?

The dispatched events are failed. I'm not sure the reason.

Comment 14

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

Comment on attachment 377075 [details] [diff] [review]
Patch v1.2 + tests

I guess I'll have to fix up the test later.

Comment 15

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

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