308471 - Implement NSTextInput -characterIndexForPoint

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Katsuhiro MIHARA]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050913 Camino/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050913 Camino/1.0+

In ChildView, implementation of protocol NSTextInput
-firstRectForCharacterRange(), -characterIndexForPoint() return fixed value.

So, IME(TSM) displays candidate window at upperleft fixed location.

In source tree, seamonkey/widget/src/cocoa/nsChildView.mm line 3212 - 3252.

At line 3221,
> #if BRADE_GETS_THIS_WORKING
http://lxr.mozilla.org/seamonkey/source/widget/src/cocoa/nsChildView.mm#3221

If this option is effective, -firstRectForCharacterRange() returns variable
value. But this value is correct?


Reproducible: Always

Steps to Reproduce:
1.turn on IME(TSM)
2.input characters
3.convert characters

Actual Results:  
IME(TSM) displays candidate window at upperleft fixed location.

Expected Results:  
IME(TSM) should display candidate window under caret.

Comment 1

[Simon Fraser [no longer active]]

Can you tell me how to get the input window for testing?

Comment 2

[Katsuhiro MIHARA]

I'm a Japanese and use IME(TSM) to input japanese characters. But I don't know
how a Western handle IME or another method to get input window. Sorry.

Comment 3

[Katsuhiro MIHARA]

What thing does occurs when you press Option-E in textarea? If the accent mark
appers, where does it appers?

Apple's document says that when a user press Option-E NSTextView displays the
accent mark in a highlighted box.
http://developer.apple.com/documentation/Cocoa/Conceptual/InputManager/Tasks/TextViewTask.html#//apple_ref/doc/uid/20001040-97257

Comment 4

[Wevah (Nate Weaver)]

I think he's talking about the window that appears when spacebar is pressed to
change hiragana into kanji when there is more than one possible choice.

Comment 5

[Katsuhiro MIHARA]

Yes, I'm talking about candidate window. But almost all of readers of this bug
can't see that window. So, I must think about another way to realize this bug.
Writer of #3 is # "He" (me, reporter of this bug).

Sorry. I enter the accentt mark and Camino displays the mark at correct
position. So there is no relation to this bug.

Comment 6

[Wevah (Nate Weaver)]

I can see what the reporter is describing. The window that shows the kanji
choices is appearing in the upper-left instead of under the hiragana to be
changed, just as comment 0 states.

Comment 7

[Simon Fraser [no longer active]]

Attachment: Patch for -firstRectForCharacterRange

This patch implements -firstRectForCharacterRange, and fixes a string fumble
elsewhere.

Comment 8

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 198854 [details] [diff] [review]
Patch for -firstRectForCharacterRange

r=pink with tweak from irc.

Comment 9

[Simon Fraser [no longer active]]

Patch checked into trunk and branch. Keeping bug around for -characterIndexForPoint

Comment 10

[Simon Fraser [no longer active]]

Comment on attachment 198854 [details] [diff] [review]
Patch for -firstRectForCharacterRange

Patched checked into trunk and branch.

Comment 11

[Katsuhiro MIHARA]

Recent Nightly-trunk Camino displays candidate window at below of Hiragana.
Version 2005100808 (1.0+)

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Moving the IME bugs to 1.1 :-(

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Moving IME-related bugs to Camino 1.2 :(

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.

Comment 15

[Stuart Morgan]

Moving to Cocoa Widget, since that's where -characterIndexForPoint fixes would happen.

Comment 16

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

I'll post a patch.

Comment 17

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

Attachment: Patch v1.0

This implements characterIndexForPoint and sends the mouse events to current input manager, however, unfortunately, this patch cannot support IME mouse handling, because current nsEditor mouse handling during composition cannot support the behavior, so, we need more works in XP level code. It should be separated to another bug.

I debugged characterIndexForPoint by the debug messages. It works fine for me.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 378803 [details] [diff] [review]
Patch v1.0

(Sorry to take so long with this review.  I've been swamped.)

This looks fine to me ... to the extent that I understand it.

+  NSInputManager* im = [NSInputManager currentInputManager];
+  if ([im wantsToHandleMouseEvents] && [im handleMouseEvent: theEvent])
+    return;

I notice that WebKit does the same thing (in the mouseUp, mouseDown
and mouseDragged methods of its WebHTMLView class).  But I don't know
under what circumstances this code gets exercised (I don't know when
the current input manager "wants to handle mouse events", or what
happens when it does so).

Attempting to test your implementation of characterIndexForPoint:, I
found that it only gets called when the current input method uses IME
(I tested with Hiragana and Pinyin).  In that case it gets called
every time you click the mouse anywhere in a browser window's content
region (on a ChildView object) -- once on mouse-down, and again on
mouse-up.

I also found a small problem -- sometimes the charAt event returns an
incorrect result (or at least an unexpected one).  When you click on
character in a different nsChildView object (i.e. when the focus was
previously in a different nsChildView object), the "offset" on
mouse-down is incorrect (always '0'), or the charAt event fails
altogether.  This happens only on mouse-down (not on mouse-up).  This
is presumably a bug in the code that handles the charAt event.

> this patch cannot support IME mouse handling, because current
> nsEditor mouse handling during composition cannot support the
> behavior, so, we need more works in XP level code

I don't understand what you're saying here.

Comment 19

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

Thank you, Steven.

(In reply to comment #18)
> (From update of attachment 378803 [details] [diff] [review])
> +  NSInputManager* im = [NSInputManager currentInputManager];
> +  if ([im wantsToHandleMouseEvents] && [im handleMouseEvent: theEvent])
> +    return;
> 
> I notice that WebKit does the same thing (in the mouseUp, mouseDown
> and mouseDragged methods of its WebHTMLView class).  But I don't know
> under what circumstances this code gets exercised (I don't know when
> the current input manager "wants to handle mouse events", or what
> happens when it does so).

Yes. If the active IME handles the mouse events, this patch should work fine. Actually, IMEs do hittests by characterIndexForPoint. If IMEs need to handle the mouse events and they need to eat them, they can do by this patch.
# But Hiragana of Kotoeri doesn't eat the mouse events.

> I also found a small problem -- sometimes the charAt event returns an
> incorrect result (or at least an unexpected one).  When you click on
> character in a different nsChildView object (i.e. when the focus was
> previously in a different nsChildView object), the "offset" on
> mouse-down is incorrect (always '0'), or the charAt event fails
> altogether.  This happens only on mouse-down (not on mouse-up).  This
> is presumably a bug in the code that handles the charAt event.

Ah, good catch. Maybe, that is a bug of query content event handling code. But that should be separated to another bug.

> > this patch cannot support IME mouse handling, because current
> > nsEditor mouse handling during composition cannot support the
> > behavior, so, we need more works in XP level code
> 
> I don't understand what you're saying here.

Currently, nsEditor commits the composition string by mouse down event. And IMEs don't eat the mouse events by [im handleMouseEvent: theEvent]. Therefore, even if an user clicks in composition string rect, the composition string is committed by nsEditor.

Ideally, nsEditor should not commit the composition string when the mouse event is occurred in the composition string rect. (Maybe, the mouse events should be ignored.)

Comment 20

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

(In reply to comment #18)
> (From update of attachment 378803 [details] [diff] [review])
> (Sorry to take so long with this review.  I've been swamped.)

No problem, I'm working on Japanese new community creation/management now. So, I'm not working on development. Fortunately, trunk restricted helps me :-)

Comment 21

[Steven Michaud [:smichaud] (Retired)]

>> +  NSInputManager* im = [NSInputManager currentInputManager];
>> +  if ([im wantsToHandleMouseEvents] && [im handleMouseEvent: theEvent])
>> +    return;
>>
>> I notice that WebKit does the same thing (in the mouseUp, mouseDown
>> and mouseDragged methods of its WebHTMLView class).  But I don't
>> know under what circumstances this code gets exercised (I don't
>> know when the current input manager "wants to handle mouse
>> events", or what happens when it does so).
>
> Yes. If the active IME handles the mouse events, this patch should
> work fine.  Actually, IMEs do hittests by characterIndexForPoint. If
> IMEs need to handle the mouse events and they need to eat them, they
> can do by this patch.  # But Hiragana of Kotoeri doesn't eat the
> mouse events.

Thanks for the info.  I added more logging and now see that, when
you're using a keyboard layout that uses IME (like Kotoeri Hiragana or
Traditional Chinese Pinyin), the input manager always "wants to handle
mouse events", but never actually handles them.  Odd ... but I see the
code does get exercised.

>>> this patch cannot support IME mouse handling, because current
>>> nsEditor mouse handling during composition cannot support the
>>> behavior, so, we need more works in XP level code
>>
>> I don't understand what you're saying here.
>
> Currently, nsEditor commits the composition string by mouse down
> event. And IMEs don't eat the mouse events by [im handleMouseEvent:
> theEvent]. Therefore, even if an user clicks in composition string
> rect, the composition string is committed by nsEditor.

Thanks.  Now I understand.  I see the uncommitted text gets committed
whenever you click anywhere in the window's content region.  And when
using Kotoeri Hiragana the results are even weirder -- the next time
you start entering text, the previously uncommitted text (which was
committed when you clicked the mouse) reappears!

>> I also found a small problem -- sometimes the charAt event returns
>> an incorrect result (or at least an unexpected one).  When you
>> click on character in a different nsChildView object (i.e. when the
>> focus was previously in a different nsChildView object), the
>> "offset" on mouse-down is incorrect (always '0'), or the charAt
>> event fails altogether.  This happens only on mouse-down (not on
>> mouse-up).  This is presumably a bug in the code that handles the
>> charAt event.
>
> Ah, good catch. Maybe, that is a bug of query content event handling
> code. But that should be separated to another bug.

I'll open a bug on this after your patch lands ... unless someone else
beats me to it :-)

Comment 22

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #21)

> And when using Kotoeri Hiragana the results are even weirder -- the
> next time you start entering text, the previously uncommitted text
> (which was committed when you clicked the mouse) reappears!

Actually this behavior (the re-appearance of Hiragana text that was
committed by clicking somewhere with the mouse) is an artifact of your
patch.  It doesn't happen without the patch.  And it also doesn't
happen when you comment out your patch's additions to [ChildView
mouseDown:].

This must ultimately be an Apple bug.  But that part of your patch
probably can't land until you figure out some way around it.

Comment 23

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

(In reply to comment #22)
> (Following up comment #21)
> 
> > And when using Kotoeri Hiragana the results are even weirder -- the
> > next time you start entering text, the previously uncommitted text
> > (which was committed when you clicked the mouse) reappears!
> 
> Actually this behavior (the re-appearance of Hiragana text that was
> committed by clicking somewhere with the mouse) is an artifact of your
> patch.  It doesn't happen without the patch.  And it also doesn't
> happen when you comment out your patch's additions to [ChildView
> mouseDown:].
> 
> This must ultimately be an Apple bug.  But that part of your patch
> probably can't land until you figure out some way around it.

Thank you for your testing! I'll look this ASAP.

Comment 24

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

I'm rewriting some query content event related code on bug 528396 and bug 519913. And I fixed bug 513952. By them, many weird bugs of Mac IME will be gone, I hope the comment 21's problem too...

And note for myself: need to get focused widget, first. and should be fired the NS_QUERY_CHARACTER_AT_POINT event on the focused widgets with its related coordinates.

Comment 25

[Markus Stange [:mstange]]

Attachment: v2

This patch only implements characterAtPoint without touching any other methods. Does this alone break IME, too?

(In reply to comment #24)
> And note for myself: need to get focused widget, first. and should be fired the
> NS_QUERY_CHARACTER_AT_POINT event on the focused widgets with its related
> coordinates.

For the dictionary (bug 301451) it would be better to fire the event on the widget under the mouse. Also, the fact that content query events are only allowed in focused things breaks the dictionary everywhere else.

Comment 26

[Markus Stange [:mstange]]

Oh, I changed NSNotFound to 0 because I'm not sure NSNotFound is a valid return value here. If we return NSNotFound, the Dictionary will actually call firstRectForCharacterRange with ranges starting at NSNotFound - 1 and NSNotFound + 1, which are meaningless numbers.

Comment 27

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

(In reply to comment #25)
> This patch only implements characterAtPoint without touching any other methods.
> Does this alone break IME, too?

It should not break any IME implementation, so, you don't need to worry.

> (In reply to comment #24)
> > And note for myself: need to get focused widget, first. and should be fired the
> > NS_QUERY_CHARACTER_AT_POINT event on the focused widgets with its related
> > coordinates.
> 
> For the dictionary (bug 301451) it would be better to fire the event on the
> widget under the mouse. Also, the fact that content query events are only
> allowed in focused things breaks the dictionary everywhere else.

Ugh, it's difficult thing. Our ChildView has one or more contexts. One is all of the document. The others are each editors and sub documents. Focus is only the switch between the contexts. Does webkit create NSTextInput protocol to each editors and sub documents?

(In reply to comment #26)
> Oh, I changed NSNotFound to 0 because I'm not sure NSNotFound is a valid return
> value here. If we return NSNotFound, the Dictionary will actually call
> firstRectForCharacterRange with ranges starting at NSNotFound - 1 and
> NSNotFound + 1, which are meaningless numbers.

It should be a bug of the Dictionary. See
http://developer.apple.com/mac/library/documentation/Cocoa/Reference/ApplicationKit/Protocols/NSTextInput_Protocol/Reference/Reference.html#//apple_ref/occ/intfm/NSTextInput/characterIndexForPoint:
http://developer.apple.com/mac/library/documentation/Cocoa/Reference/NSTextInputClient_Protocol/Reference/Reference.html#//apple_ref/occ/intfm/NSTextInputClient/characterIndexForPoint:

Comment 28

[Markus Stange [:mstange]]

(In reply to comment #27)
> (In reply to comment #25)
> > This patch only implements characterAtPoint without touching any other methods.
> > Does this alone break IME, too?
> 
> It should not break any IME implementation, so, you don't need to worry.

Good.

> > (In reply to comment #24)
> > > And note for myself: need to get focused widget, first. and should be fired the
> > > NS_QUERY_CHARACTER_AT_POINT event on the focused widgets with its related
> > > coordinates.
> > 
> > For the dictionary (bug 301451) it would be better to fire the event on the
> > widget under the mouse. Also, the fact that content query events are only
> > allowed in focused things breaks the dictionary everywhere else.
> 
> Ugh, it's difficult thing. Our ChildView has one or more contexts. One is all
> of the document. The others are each editors and sub documents. Focus is only
> the switch between the contexts. Does webkit create NSTextInput protocol to
> each editors and sub documents?

I don't know. Let's have this discussion in bug 569331.

> (In reply to comment #26)
> > Oh, I changed NSNotFound to 0 because I'm not sure NSNotFound is a valid return
> > value here. If we return NSNotFound, the Dictionary will actually call
> > firstRectForCharacterRange with ranges starting at NSNotFound - 1 and
> > NSNotFound + 1, which are meaningless numbers.
> 
> It should be a bug of the Dictionary.

OK, I'll change it back.

Comment 29

[Markus Stange [:mstange]]

Attachment: v3

with NSNotFound instead of 0

Comment 30

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

Comment on attachment 456362 [details] [diff] [review]
v3

>+  if (!mGeckoChild || ![self window])
>+    return NSNotFound;

nit: use {}.

if (!mGeckoChild || ![self window]) {
  return NSNotFound;
}

>+  NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(0);

NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(NSNotFound);

With these changes, r=masayuki

Comment 31

[Nathan Yee [:nyee]]

This is a reminder email to give attention to this bug. This bug blocks [Bug 301451 - Gecko doesn't support Cmd-Ctrl-D lookup in Mac OS X Dictionary.app (needs to implement NSTextInput protocol)], and it appears that an approved patch for this bug was created in 2010. Masayuki, can you confirm that your patch is complete and/or you are still working on this bug?

Comment 32

[Nathan Yee [:nyee]]

It appears that this bug depends on Bug 569331. If this still is an issue, then this patch might be inadequate to fix this bug.

Comment 33

[Stephen A Pohl [:spohl]]

I've been getting up to speed on bug 301451 over the past week since I've been asked to mentor it. It appears that the necessary steps are unchanged. We need:
1. An update to the patch in this bug to make it apply to current trunk. I might be able to submit this early this week.
2. A patch for bug 569331.

Unless I missed something, this should be all that's required to get the OSX dictionary to work reliably in Firefox.

Clearing n-i for Masayuki, but feel free to chime in.

Comment 34

[Stephen A Pohl [:spohl]]

Attachment: Patch with logging enabled.

Updated patch to apply to trunk and added the use of backing scale factor for retina displays. Logging is currently enabled for convenience to figure out bug 569331.

Comment 35

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

If we need to implement this with whole content rather than only focused content, I guess that we need to change the behavior of some other methods of NSTextInput protocol. Of course, that may cause some regressions of handling IME and some performance regressions...

Comment 36

[Stephen A Pohl [:spohl]]

Attachment: Patch

After speaking with mstange on IRC, it looks like we might want to go ahead and land this patch while we keep working on bug 569331. Although this patch will only give us the ability to look up text in focused widgets, it is still an improvement over the current state with no obvious negatives.

I've updated the patch for current trunk, removed logging and made sure that we take the backing scale factor into account to make this work on retina displays. Masayuki, even though you've already reviewed and approved Markus' previous patch I'm setting this back to r? for you to have one more quick look at it.

Comment 37

[Nathan Yee [:nyee]]

I am testing attachment 8463189 [details] [diff] [review] from bug 308471. Strangely, cmd-ctrl-D/three-finger-tap-gesture works without needing to highlight text once text on a page is clicked on. This process has to be repeated again after changing pages though. As described in bug 569334, the Dictionary overlay text shows the wrong font superimposed on the desired text (possibly Apple’s default behavior when the font attributes of a block of text cannot be determined?). I will begin debugging soon. Stephen does https://developer.mozilla.org/en-US/docs/Debugging_on_Mac_OS_X accurately describe how to set up debugging in OS X?

Comment 38

[Nathan Yee [:nyee]]

My apologies, the previous comment was meant for bug 569331.

Comment 39

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

Comment on attachment 8463452 [details] [diff] [review]
Patch

>+  charAt.refPoint.x = (int32_t)ptInView.x * mWidget->BackingScaleFactor();
>+  charAt.refPoint.y = (int32_t)ptInView.y * mWidget->BackingScaleFactor();

Please use static_cast for them. Then, after |=| should be next line with one more indent like:

charAt.refPoint.x =
  static_cast<int32_t>(ptInView.x) * mWidget->BackingScaleFactor();

>+  mWidget->DispatchWindowEvent(charAt);
>+  if (!charAt.mSucceeded ||
>+      charAt.mReply.mOffset == WidgetQueryContentEvent::NOT_FOUND ||
>+      charAt.mReply.mOffset >= (uint32_t)NSNotFound) {

Please use static_cast here too.

Otherwise, looks okay to me.

Comment 40

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks, Masayuki! Addressed review feedback. Carrying over r+.

Comment 41

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e23aeaace71

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4e23aeaace71