826657 - [TSF] Implement ITfMouseTrackerACP

Status: RESOLVED FIXED

(Core :: Widget: Win32, enhancement)

Description

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

We should implement ITfMouseTrackerACP interface. It's QIed by ATOK 2013. Probably, it's necessary for composing clause selection with mouse.

Comment 1

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

According to http://msdn.microsoft.com/en-us/library/windows/desktop/ms628802%28v=vs.85%29.aspx , TIP needs to decide the range of text which they want to handle mouse event.

When it's called, I think that nsTextStore should cache character rects of the range and refresh them at OnLayoutChange() called. This prevents to use CPU excessively and helps e10s mode.

I think that this should be implemented as that bug 1050644 can use same mechanism.

I'll work on this next week.

Comment 2

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

Attachment: part.1 nsTextStore should support ITfMouseTrackerACP

Comment 3

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

Attachment: part.1 nsTextStore should support ITfMouseTrackerACP

Comment 4

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

Attachment: part.2 Implement NOTIFY_IME_OF_MOUSE_BUTTON_EVENT in XP part

This patch still has a bug on XUL panel.

Comment 5

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

Attachment: part.3 nsTextStore should handle NOTIFY_IME_OF_MOUSE_BUTTON_EVENT

Comment 6

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

Attachment: part.2 Implement NOTIFY_IME_OF_MOUSE_BUTTON_EVENT in XP part

First, Smaug, could you review this patch?

This patch is the main part of this bug. For making IME mouse event handling e10s aware, we cannot query if mousedown or mouseup is fired on a character and its offset and rect from widget synchronously. Therefore, this patch adds a new notification to IME, that is NOTIFY_IME_OF_MOUSE_BUTTON_EVENT. This is notified only when widget returns NOTIFY_MOUSE_BUTTON_EVENT_ON_CHAR when nsIWidget::GetIMEUpdatePreference() is called.

When mousedown or mouseup event is fired on the observing editor (it must be the focused editor), nsEditorEventListener calls IMEStateManager::OnMouseButtonEventInEditor(). If there is active IMEContentObserver and the editor which received the mouse button event is managed by it, IMEContentObserver::OnMouseButtonEvent() is called. Then, IMEContentObserver::OnMouseButtonEvent() checks if the mouse button event is fired on a character of the editor. If it's not fired on any character of the editor, it does nothing. Otherwise, it notifies its widget of the event message, button, buttons, modifiers, refPoint in the widget and character rect in the widget.

If the mouse button event is consumed by IME, editor ignores the event. Otherwise, handles it normally.

For returning the result whether IME consumed the mouse button event, this patch adds new success code for nsIWidget::NotifyIME(). That is NS_SUCCESS_EVENT_CONSUMED.

If you agree with this approach, I'll request review the editor part to Ehsan.

Comment 7

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

Attachment: part.3 nsTextStore should handle NOTIFY_IME_OF_MOUSE_BUTTON_EVENT

Comment 8

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

NS_SUCCESS_EVENT_CONSUMED is a bit ugly, but I don't have better suggesting right now.

Comment 9

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

Based in IRC people don't like to add more success codes.
So use perhaps an out param to indicated whether the event was consumed.

Comment 10

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

Comment on attachment 8478217 [details] [diff] [review]
part.2 Implement NOTIFY_IME_OF_MOUSE_BUTTON_EVENT in XP part


>+nsEditorEventListener::GetFocusedRootContent()
>+{
>+  if (NS_WARN_IF(!mEditor)) {
>+    return nullptr;
>+  }
>+  nsCOMPtr<nsIContent> focusedContent = mEditor->GetFocusedContent();
>+  if (!focusedContent) {
>+    return nullptr;
>+  }
>+  nsIDocument* currentDoc = focusedContent->GetCurrentDoc();
This should use newer GetComposedDoc() which deals with shadow dom too.


>+nsEditorEventListener::EditorHasFocus()
>+{
>+  NS_PRECONDITION(mEditor,
>+    "The caller must check whether this is connected to an editor");
>+  nsCOMPtr<nsIContent> focusedContent = mEditor->GetFocusedContent();
>+  if (!focusedContent) {
>+    return false;
>+  }
>+  nsIDocument* currentDoc = focusedContent->GetCurrentDoc();
This should use newer GetComposedDoc() which deals with shadow dom too.


        The notification is
>+    // notified to IME as a mouse event.
I don't understand this sentence.



>+    // NOTIFY_IME_OF_MOUSE_BUTTON_EVENT specific data
>+    struct
>+    {
>+      // The value of WidgetEvent::message
>+      uint32_t mEventMessage;
>+      // Character offset under the cursor
>+      uint32_t mOffset;
So is this offset from the beginning of the text node or what?

>+      // Cursor position relative to the widget
>+      struct
>+      {
>+        int32_t mX;
>+        int32_t mY;
>+
>+        void Set(const nsIntPoint& aPoint)
>+        {
>+          mX = aPoint.x;
>+          mY = aPoint.y;
>+        }
>+        nsIntPoint AsIntPoint() const
>+        {
>+          return nsIntPoint(mX, mY);
>+        }
>+      } mCursorPos;
>+      // Character rect under the cursor relative to the widget
>+      struct
>+      {
>+        int32_t mX;
>+        int32_t mY;
>+        int32_t mWidth;
>+        int32_t mHeight;
>+
>+        void Set(const nsIntRect& aRect)
>+        {
>+          mX = aRect.x;
>+          mY = aRect.y;
>+          mWidth = aRect.width;
>+          mHeight = aRect.height;
>+        }
>+        nsIntRect AsIntRect() const
>+        {
>+          return nsIntRect(mX, mY, mWidth, mHeight);
>+        }
>+      } mCharRect;

Please document in which coordinate space the values are.

Comment 11

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

Thank you for the review.

(In reply to Olli Pettay [:smaug] from comment #9)
> Based in IRC people don't like to add more success codes.
> So use perhaps an out param to indicated whether the event was consumed.

Do you think that the argument of nsIWidget::NotifyIME() should be non-const? Or add a new out argument for it?

Comment 12

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

FYI: current definition:

http://mxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h#1818
> /**
>  * Notify IME of the specified notification.
>  */
> NS_IMETHOD NotifyIME(const IMENotification& aIMENotification) = 0;

Changing the method needs a lot of minor changes in all widgets.

Comment 13

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

I was thinking a new bool& aConsumed param.

Yeah, it is a bit annoying to change it everywhere, but non-NS_OK success values are so odd.
We don't have many such.
(I shouldn't have added http://mxr.mozilla.org/mozilla-central/source/xpcom/base/ErrorList.h#65)

Comment 14

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

Though, looks like we have quite a few success values...
So fine, let's use success value here.

Comment 15

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

(In reply to Olli Pettay [:smaug] from comment #13)
> I was thinking a new bool& aConsumed param.

If we would add a new out argument, it might be better to add a struct which has only a bool value, such as |struct ResultOfIME { bool aEventConsumed; }|. Then, we can modify it easier in the future.

(In reply to Olli Pettay [:smaug] from comment #14)
> Though, looks like we have quite a few success values...
> So fine, let's use success value here.

Okay, I'll keep current approach (won't add a new out argument).

Comment 16

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

Comment on attachment 8477323 [details] [diff] [review]
part.1 nsTextStore should support ITfMouseTrackerACP

First, we need to implement ITfMouseTrackerACP interface and make nsTextStore accept ITfMouseSink instances.

In MSDN, it doesn't say explicitly if ITextStore implementation should be able to accept multiple MouseSink. However, other implementations, e.g., https://github.com/Ouroboros/Arianrhod/blob/f0a1ced11540be9cc6f30f74db34c04fef0e5906/Source/sources/DotNetReferenceSource/wpf/src/Framework/System/Windows/Documents/TextStore.cs#L1397 accept multiple MouseSink.

Therefore, this patch makes nsTextStore support multiple MouseSink. Each installed MouseSink is wrapped by MouseTracker class. It stores cookie value and recyclable after uninstalled.

FYI:
ITfMouseTrackerACP interface
http://msdn.microsoft.com/en-us/library/windows/desktop/ms628799%28v=vs.85%29.aspx
ITfMouseTrackerACP::AdviseMouseSink method
http://msdn.microsoft.com/en-us/library/windows/desktop/ms628802%28v=vs.85%29.aspx
ITfMouseTrackerACP::UnadviseMouseSink method
http://msdn.microsoft.com/en-us/library/windows/desktop/ms628803%28v=vs.85%29.aspx


See part.3 for the code using MouseTracker class.

Comment 17

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

Attachment: part.2 Implement NOTIFY_IME_OF_MOUSE_BUTTON_EVENT in XP part

Ehsan, could you review the editor part?

When editor receives mousedown or mouseup event, it should notify IME of the event. If it's consumed by IME, editor should do nothing and also if mousedown or mouseup is consumed, following click event should be ignored. Note that click event must be fired after both mousedown and mouseup event on editor. So, resetting the flag at handling mosuedown event handler is enough.

And also, this patch makes some utility methods in nsEditorEventListener. Their cod is not almost changed except GetCurrentDoc() -> GetComposedDoc(). This is suggested by smaug in comment 10.

Comment 18

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

Attachment: part.3 nsTextStore should handle NOTIFY_IME_OF_MOUSE_BUTTON_EVENT

This patch makes nsTextStore handle new notification NOTIFY_IME_OF_MOUSE_BUTTON_EVENT. NotifyIME() is called with this when mouse button is pressed or released on a character in focused editor.

At that time, it should call ITfMouseSink::OnMouseEvent().
http://msdn.microsoft.com/en-us/library/windows/desktop/ms628795%28v=vs.85%29.aspx

The arguments are similar to IMM's mouse event handling.
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsIMM32Handler.cpp#2049

FYI: I'll rewrite IMM32's handler with this new notification for making it e10s aware in another bug. (Although, we need to make the notification cross process boundary.)

Comment 19

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

Comment on attachment 8478834 [details] [diff] [review]
part.3 nsTextStore should handle NOTIFY_IME_OF_MOUSE_BUTTON_EVENT

Oops, see the previous comment for the detail.

Comment 20

[(no longer active)]

Comment on attachment 8478826 [details] [diff] [review]
part.2 Implement NOTIFY_IME_OF_MOUSE_BUTTON_EVENT in XP part

Review of attachment 8478826 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the editor/ changes with the below addressed.  I did not look at the rest of the changes.

::: editor/libeditor/nsEditorEventListener.cpp
@@ +311,5 @@
> +nsEditorEventListener::GetFocusedRootContent()
> +{
> +  if (NS_WARN_IF(!mEditor)) {
> +    return nullptr;
> +  }

NS_ENSURE_TRUE(mEditor, nullptr) please.

@@ +315,5 @@
> +  }
> +  nsCOMPtr<nsIContent> focusedContent = mEditor->GetFocusedContent();
> +  if (!focusedContent) {
> +    return nullptr;
> +  }

NS_ENSURE_TRUE

@@ +319,5 @@
> +  }
> +  nsIDocument* composedDoc = focusedContent->GetComposedDoc();
> +  if (NS_WARN_IF(!composedDoc)) {
> +    return nullptr;
> +  }

NS_ENSURE_TRUE

@@ +331,5 @@
> +    "The caller must check whether this is connected to an editor");
> +  nsCOMPtr<nsIContent> focusedContent = mEditor->GetFocusedContent();
> +  if (!focusedContent) {
> +    return false;
> +  }

NS_ENSURE_TRUE

@@ +404,5 @@
>        nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aEvent);
>        return KeyPress(keyEvent);
>      }
>      // mousedown
>      case NS_MOUSE_BUTTON_DOWN: {

Please MOZ_ASSERT(!mMouseDownOrUpConsumedByIME) here and down in the NS_MOUSE_BUTTON_UP too.

@@ +408,5 @@
>      case NS_MOUSE_BUTTON_DOWN: {
>        nsCOMPtr<nsIDOMMouseEvent> mouseEvent = do_QueryInterface(aEvent);
> +      if (NS_WARN_IF(!mouseEvent)) {
> +        return NS_OK;
> +      }

NS_ENSURE_TRUE

@@ +417,5 @@
>      case NS_MOUSE_BUTTON_UP: {
>        nsCOMPtr<nsIDOMMouseEvent> mouseEvent = do_QueryInterface(aEvent);
> +      if (NS_WARN_IF(!mouseEvent)) {
> +        return NS_OK;
> +      }

NS_ENSURE_TRUE

@@ +428,5 @@
>      case NS_MOUSE_CLICK: {
>        nsCOMPtr<nsIDOMMouseEvent> mouseEvent = do_QueryInterface(aEvent);
> +      if (NS_WARN_IF(!mouseEvent)) {
> +        return NS_OK;
> +      }

NS_ENSURE_TRUE

@@ -591,5 @@
>  
>  nsresult
>  nsEditorEventListener::MouseClick(nsIDOMMouseEvent* aMouseEvent)
>  {
> -  NS_ENSURE_TRUE(aMouseEvent, NS_OK);

Hmm, why did you remove these?

@@ +660,5 @@
>    // consumed.
> +  if (EditorHasFocus()) {
> +    nsPresContext* presContext = GetPresContext();
> +    if (presContext) {
> +     IMEStateManager::OnClickInEditor(presContext, GetFocusedRootContent(),

Nit: indentation

@@ +756,5 @@
> +  bool defaultPrevented;
> +  nsresult rv = aMouseEvent->GetDefaultPrevented(&defaultPrevented);
> +  if (NS_WARN_IF(NS_FAILED(rv)) || defaultPrevented) {
> +    return false;
> +  }

NS_ENSURE_SUCCESS and NS_ENSURE_TRUE please.

@@ +760,5 @@
> +  }
> +  nsPresContext* presContext = GetPresContext();
> +  if (NS_WARN_IF(!presContext)) {
> +    return false;
> +  }

NS_ENSURE_TRUE

Comment 21

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #20)
> r=me on the editor/ changes with the below addressed.  I did not look at the
> rest of the changes.

Thank you, other part was already reviewed by smaug.

> ::: editor/libeditor/nsEditorEventListener.cpp
> @@ +311,5 @@
> > +nsEditorEventListener::GetFocusedRootContent()
> > +{
> > +  if (NS_WARN_IF(!mEditor)) {
> > +    return nullptr;
> > +  }
> 
> NS_ENSURE_TRUE(mEditor, nullptr) please.

Oh, sorry. I missed to change it after the fix of bug 1056545.

> @@ +315,5 @@
> > +  }
> > +  nsCOMPtr<nsIContent> focusedContent = mEditor->GetFocusedContent();
> > +  if (!focusedContent) {
> > +    return nullptr;
> > +  }
> 
> NS_ENSURE_TRUE

Hmm, no. This is possible case.

> @@ +331,5 @@
> > +    "The caller must check whether this is connected to an editor");
> > +  nsCOMPtr<nsIContent> focusedContent = mEditor->GetFocusedContent();
> > +  if (!focusedContent) {
> > +    return false;
> > +  }
> 
> NS_ENSURE_TRUE

ditto.

> @@ +404,5 @@
> >        nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aEvent);
> >        return KeyPress(keyEvent);
> >      }
> >      // mousedown
> >      case NS_MOUSE_BUTTON_DOWN: {
> 
> Please MOZ_ASSERT(!mMouseDownOrUpConsumedByIME) here and down in the
> NS_MOUSE_BUTTON_UP too.

mMouseDownOrUpConsumedByIME may be true. For example, if only mousedown is consumed by IME but mouseup is fired outside of the editor, click event is not fired on the editor. Therefore, nsEditorEventListener cannot clear it.

The possible cases are:

1. mousedown, mouseup and click are fired.
2. only mousedown is fired.
3. only mouseup is fired.

So, if click event is fired, there must be the preceding mousedown and mouseup event. This is the only case we can assume the flag state.

Therefore, at handling mousedown event, the patch overwrites the flag always.

Note that When #2 occurs and then, #3 occurs, the flag value is not correct. However, this is not a problem because the only user of the flag is click event handler but click event is never fired in case #3.

> @@ -591,5 @@
> >  
> >  nsresult
> >  nsEditorEventListener::MouseClick(nsIDOMMouseEvent* aMouseEvent)
> >  {
> > -  NS_ENSURE_TRUE(aMouseEvent, NS_OK);
> 
> Hmm, why did you remove these?

Because HandleEvent() checks if it's nullptr. Do you want they stay there?

Comment 22

[(no longer active)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> > @@ +315,5 @@
> > > +  }
> > > +  nsCOMPtr<nsIContent> focusedContent = mEditor->GetFocusedContent();
> > > +  if (!focusedContent) {
> > > +    return nullptr;
> > > +  }
> > 
> > NS_ENSURE_TRUE
> 
> Hmm, no. This is possible case.
> 
> > @@ +331,5 @@
> > > +    "The caller must check whether this is connected to an editor");
> > > +  nsCOMPtr<nsIContent> focusedContent = mEditor->GetFocusedContent();
> > > +  if (!focusedContent) {
> > > +    return false;
> > > +  }
> > 
> > NS_ENSURE_TRUE
> 
> ditto.

OK.

> > @@ +404,5 @@
> > >        nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aEvent);
> > >        return KeyPress(keyEvent);
> > >      }
> > >      // mousedown
> > >      case NS_MOUSE_BUTTON_DOWN: {
> > 
> > Please MOZ_ASSERT(!mMouseDownOrUpConsumedByIME) here and down in the
> > NS_MOUSE_BUTTON_UP too.
> 
> mMouseDownOrUpConsumedByIME may be true. For example, if only mousedown is
> consumed by IME but mouseup is fired outside of the editor, click event is
> not fired on the editor. Therefore, nsEditorEventListener cannot clear it.
> 
> The possible cases are:
> 
> 1. mousedown, mouseup and click are fired.
> 2. only mousedown is fired.
> 3. only mouseup is fired.
> 
> So, if click event is fired, there must be the preceding mousedown and
> mouseup event. This is the only case we can assume the flag state.
> 
> Therefore, at handling mousedown event, the patch overwrites the flag always.
> 
> Note that When #2 occurs and then, #3 occurs, the flag value is not correct.
> However, this is not a problem because the only user of the flag is click
> event handler but click event is never fired in case #3.

OK, but please document this in the patch.

> > @@ -591,5 @@
> > >  
> > >  nsresult
> > >  nsEditorEventListener::MouseClick(nsIDOMMouseEvent* aMouseEvent)
> > >  {
> > > -  NS_ENSURE_TRUE(aMouseEvent, NS_OK);
> > 
> > Hmm, why did you remove these?
> 
> Because HandleEvent() checks if it's nullptr. Do you want they stay there?

No, that's fine!  Thanks for explaining!

Comment 23

[Masatoshi Kimura [:emk]]

Comment on attachment 8477323 [details] [diff] [review]
part.1 nsTextStore should support ITfMouseTrackerACP

Review of attachment 8477323 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsTextStore.cpp
@@ +3413,5 @@
> +           ("TSF: 0x%p   nsTextStore::UnadviseMouseSink() FAILED due to "
> +            "the cookie is invalid value", this));
> +    return E_INVALIDARG;
> +  }
> +  // The cookie value must be stored index of mMouseTrackers.

"The cookie value must be an index of mMouseTrackers."

@@ +4451,5 @@
> +           ("TSF: 0x%p   nsTextStore::MouseTracker::Init() FAILED due to "
> +            "this is not the last element of mMouseTrackers", this));
> +    return E_FAIL;
> +  }
> +  if (aTextStore->mMouseTrackers.Length() >= kInvalidCookie) {

A cookie value should be still available if |aTextStore->mMouseTrackers.Length() == kInvalidCookie| because mCookie will be |aTextStore->mMouseTrackers.Length() - 1|.

@@ +4481,5 @@
> +            "due to already being used", this));
> +    return E_FAIL;
> +  }
> +
> +  HRESULT hr = aTextRange->GetExtent(&mStart, &mLength);

Who will use mStart and mLength after the initialization?

Comment 24

[Masatoshi Kimura [:emk]]

Comment on attachment 8477323 [details] [diff] [review]
part.1 nsTextStore should support ITfMouseTrackerACP

Ah, the part 3 patch added some accessors.

Comment 25

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/44808b493a7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/763a2f2ae231
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0461ce2e137

Comment 26

[(Out Sep 17-23) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/44808b493a7e
https://hg.mozilla.org/mozilla-central/rev/763a2f2ae231
https://hg.mozilla.org/mozilla-central/rev/e0461ce2e137