Bug 5230 – WebHTMLView (WebNSTextInputSupport) - "characterIndexForPoint: not yet implemented"

• Status: VERIFIED
• Resolution: FIXED
• Component: HTML Editing

[Evan Gross]

This method needs to be implemented in order for many input methods to work properly, at least wrt 
handling mouse clicks in an inline input region.

[Evan Gross]

Created attachment 4140 [details]
proposed implementation of characterIndexForPoint

This implementation requires the convertDOMRangeToNSRange patch. I wasn't sure
whether it was possible to create a single file with both patches (probably is,
just don't know how).

Anyway, this an initial stab at getting this working. Seems to do the trick,
maybe needs some range/error checking, maybe there's a way to do it without
adding to WebCoreBridge (the convertDOMRangeToNSRange patch)?

[Evan Gross]

Created attachment 4141 [details]
Addition to WebCoreBridge, needed by attachment 4140 [details]

I couldn't find this functionality in an existing method - maybe I missed
something? Might be nice NOT to have to add to WebCoreBridge...

[Evan Gross]

Likely requires (proposed) fixes for bug 4394, bug 4680, and perhaps others we're working on, for input 
methods that use PosToOffset to truly be functional.

[Alexey Proskuryakov]

I agree that having this method implemented is very important.

There is a simple way to reproduce this problem: click inside Kotoeri's unconfirmed text - the caret moves 
to the beginning of the inline input area.

Several observations about the proposed patch:
1. It uses tabs instead of spaces in some places.
2. characterIndexForPoint is documented to return NSNotFound when a click happens outside of any 
character, and it does make a difference for input methods.
3. We should also get NSNotFound if the point is after the end of text (editableDOMRangeForPoint handles 
this in a different way).

[Evan Gross]

(In reply to comment #4)
> 1. It uses tabs instead of spaces in some places.
Uggh. My default Xcode settings. Easy enough to fix.

> 2. characterIndexForPoint is documented to return NSNotFound when a click happens outside of any 
> character, and it does make a difference for input methods.

Missed that one. Big question: how to detect when a click happens outside of any character? Have you 
looked into this? Remember: doesn't matter whether there's an inline session, postooffset returns 
document-relative offsets for no inline area.

> 3. We should also get NSNotFound if the point is after the end of text (editableDOMRangeForPoint 
handles this in a different way).
How does it handle it? This was my first time playine with that method. Can you detect "after the end of 
text"?

[Alexey Proskuryakov]

(In reply to comment #5)

editableDOMRangeForPoint just returns a (zero-length) range for an insertion point at the end of the text 
then (where the caret should move after such a click). When I first approached characterIndexForPoint, I 
have tried to extend the range returned by editableDOMRangeForPoint (to cover a character after the 
insertion point) and check if the point was in firstRectForDOMRange - but that didn't work reliably because 
of rounding errors. Perhaps, one should just grow the rect by one point to solve this?

Another approach is to drop down to hit-testing code (similar to what [WebCoreBridge elementAtPoint] 
does)... But if I knew for sure, I would have already implemented this myself :)

[Eric Seidel (no email)]

Comment on attachment 4140 [details]
proposed implementation of characterIndexForPoint

A couple comments:

1.  I assume you have some sort of test for this?  It's not immediately obvious
to me that [self convertPoint:[[self window] convertScreenToBase:thePoint]
fromView:nil]; is the right way to get the point in the proper coordinates.  (I
assume the point passed to you is in screen coords? not window or view coords?)

2.  You have several lines with tabs instead of spaces.  According to our
coding style guidelines we use tabs, and not spaces:
http://webkit.opendarwin.org/blog/?page_id=25

3.  The last else is not needed (and has been known to confuse	stupider
versions of gcc into thinking you can reach the end of the function w/o
returning.)

Other than that the patch looks fine.  Someone who knows more about the Obj-C
DOM should review this however.

Thanks for the patch!

[Evan Gross]

Created attachment 4163 [details]
Input Method for testing various issues

Here's a modified version of the BasicInputMethod sample, enhanced to do the
following:

1. The Send Event Palette can send OffsetToPos with positive/zero/negative
offsets. Good for this particular bug.
2. Various enhancements to the inline input:

- Can do click-positioning of the caret via PosToOffset
- Arrow keys work
- Drag-selecting within inline area works (well, not in NSTextView, radar filed

long, long ago)
- Shift-arrow extends selection
- You can type spaces and tabs into the inline text (good for showing all the
bugs with input methods sending whitespace vs. typing spaces and tabs - radars
filed long, long ago).

A bit buggy, you may need to open and close a window to get it to activate
properly at times.

To use it for testing with this bug, just activate the input method and click!
Click either inside the active inline area, or if there is none, just click
anywhere in the document. There is some useful info spewed to the Console by
the input method about the results from PosToOffset. Combine that with what's
happening in characterIndexForPoint to get a good idea of what's going on.

Good for testing with bug 4681 and bug 4682, perhaps others.

[Darin Adler]

Comment on attachment 4141 [details]
Addition to WebCoreBridge, needed by attachment 4140 [details]

Seems OK to add this, although I'm not super-happy with the name of it; it's a
little strange to have non-parallel names for two parallel conversion functions
-- we should rename convertToObjCDOMRange to convertNSRangeToDOMRange when we
add this one. Typically, though, we use a single patch with all the changes.
You can generate such a patch by executing "cvs-create-patch WebCore WebKit >
patch.txt" in the directory above.

I'm going to mark this review-, hoping to see such a combined patch and also
hoping to see my suggested name change included in the next round (although
strictly speaking, that part is optional and could be done later).

[Alexey Proskuryakov]

Created attachment 4294 [details]
automated test

(goes to editing/input)

This test needs a certain level of support from the implementation - when
running DumpRenderTree, the window is nil, so conversion from screen to base
coordinates shouldn't be attempted.

[Alexey Proskuryakov]

Created attachment 4295 [details]
test results

[Alexey Proskuryakov]

Comment on attachment 4295 [details]
test results

The results were not entirely correct... I'm working on a patch that will
include an improved test.

[Alexey Proskuryakov]

Created attachment 4371 [details]
proposed implementation

- Incorporated Darin's suggestions.
- Implementation passes the included expanded test (obviously).
- Fixed a bug in [WebCoreBridge firstRectForDOMRange] (no regression tests
broken; covered by the included test).
- Doesn't work correctly for the first character after a line wrap. Seems to be
another problem with firstRectForDOMRange, which I intend to deal with
separately.
- Tested to verify that the behavior matches NSTextView. The only difference I
found is that NSTextView's implementation doesn't return NSNotFound when a
click happens outside of any character (which is required by the spec). This
doesn't seem to cause problems.
- I don't understand why editableDOMRangeForPoint doesn't work like
characterRangeAtPoint, introduced in this patch.

[Darin Adler]

Comment on attachment 4371 [details]
proposed implementation

This looks pretty good. But is characterIndexForPoint: really supposed to tell
you what character you're over? Or is it instead supposed to tell you the
character index of the space between two characters that you happen to be at?

The characterRangeAtPoint: method you've written works hard to find the
character the point is over rather than the insertion point location, but I'm a
bit surprised if that's what characterIndexForPoint: is supposed to do.

[Alexey Proskuryakov]

(In reply to comment #14)
Yes, characterIndexForPoint: is supposed to return the character index rather than the insertion point 
index. Besides the name and the documentation <http://developer.apple.com/documentation/Cocoa/
Reference/ApplicationKit/ObjC_classic/Protocols/NSTextInput.html#//apple_ref/doc/uid/20000372-
BAJIIHDH>, which both say this, the implementations of NSTextView and Kotoeri work so.

NSTextView: type something with Kotoeri in TextEdit, and click on a character. The insertion point is 
then always moved to the left of the character, rather than to the nearest position, which suggests that 
this method indeed returns the character index.

Kotoeri: at first, I have tried to return the insertion point index rather than character index, and noticed 
that Kotoeri couldn't handle the position after the last character in an inline input area if it was returned 
from characterIndexForPoint:, which suggests that it treats the returned value as a character index.

Apparently, Kotoeri abuses characterIndexForPoint:, and thus achieves imperfect results (insertion point 
is positioned differently in inline input areas than in normal text), but it does that knowingly :)

[Alexey Proskuryakov]

Filed the remaining [WebCoreBridge firstRectForDOMRange:] issue as bug 5401.

[Maciej Stachowiak]

Comment on attachment 4371 [details]
proposed implementation

Looks like a good fix. r=me

[Alexey Proskuryakov]

An apparently unrelated problem makes this patch cause a crash when running layout tests. Filed as bug 
5717.