Closed Bug 826657 Opened 7 years ago Closed 6 years ago

[TSF] Implement ITfMouseTrackerACP

Categories

(Core :: Widget: Win32, enhancement)

x86_64
Windows 8
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(3 files, 5 obsolete files)

13.50 KB, patch
emk
: review+
Details | Diff | Splinter Review
27.46 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
14.03 KB, patch
emk
: review+
Details | Diff | Splinter Review
We should implement ITfMouseTrackerACP interface. It's QIed by ATOK 2013. Probably, it's necessary for composing clause selection with mouse.
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.
Blocks: 1050644
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
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.
Attachment #8477330 - Attachment is obsolete: true
Attachment #8478217 - Flags: review?(bugs)
NS_SUCCESS_EVENT_CONSUMED is a bit ugly, but I don't have better suggesting right now.
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 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.
Attachment #8478217 - Flags: review?(bugs) → review+
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?
Flags: needinfo?(bugs)
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.
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)
Flags: needinfo?(bugs)
Though, looks like we have quite a few success values...
So fine, let's use success value here.
(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 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.
Attachment #8477323 - Flags: review?(VYV03354)
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.
Attachment #8478217 - Attachment is obsolete: true
Attachment #8478826 - Flags: review?(ehsan)
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.)
Attachment #8478218 - Attachment is obsolete: true
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.
Attachment #8478834 - Flags: review?(VYV03354)
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
Attachment #8478826 - Flags: review?(ehsan) → review+
(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?
Flags: needinfo?(ehsan)
(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!
Flags: needinfo?(ehsan)
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 on attachment 8477323 [details] [diff] [review]
part.1 nsTextStore should support ITfMouseTrackerACP

Ah, the part 3 patch added some accessors.
Attachment #8477323 - Flags: review?(VYV03354) → review+
Attachment #8478834 - Flags: review?(VYV03354) → review+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.