|Summary:||WebHTMLView (WebNSTextInputSupport) - "characterIndexForPoint: not yet implemented"|
|Product:||WebKit||Reporter:||Evan Gross <evan>|
|Component:||HTML Editing||Assignee:||Dave Hyatt <hyatt>|
|OS:||OS X 10.4|
|Bug Depends on:||5717|
Description Evan Gross 2005-10-02 00:22:52 PDT
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.
Comment 1 Evan Gross 2005-10-02 00:30:04 PDT
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)?
Comment 2 Evan Gross 2005-10-02 00:33:50 PDT
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...
Comment 3 Evan Gross 2005-10-02 00:46:10 PDT
Comment 4 Alexey Proskuryakov 2005-10-02 00:46:44 PDT
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).
Comment 5 Evan Gross 2005-10-02 02:29:37 PDT
(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"?
Comment 6 Alexey Proskuryakov 2005-10-02 08:16:43 PDT
(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 :)
Comment 7 Eric Seidel (no email) 2005-10-02 14:04:26 PDT
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!
Comment 8 Evan Gross 2005-10-02 20:27:05 PDT
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.
Comment 9 Darin Adler 2005-10-02 20:51:16 PDT
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).
Comment 10 Alexey Proskuryakov 2005-10-10 12:42:45 PDT
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.
Comment 11 Alexey Proskuryakov 2005-10-10 12:43:32 PDT
Created attachment 4295 [details] test results
Comment 12 Alexey Proskuryakov 2005-10-16 01:26:21 PDT
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.
Comment 13 Alexey Proskuryakov 2005-10-16 13:17:12 PDT
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.
Comment 14 Darin Adler 2005-10-16 17:22:14 PDT
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.
Comment 15 Alexey Proskuryakov 2005-10-16 21:38:12 PDT
(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 :)
Comment 16 Alexey Proskuryakov 2005-10-17 09:50:04 PDT
Filed the remaining [WebCoreBridge firstRectForDOMRange:] issue as bug 5401.
Comment 17 Maciej Stachowiak 2005-10-18 00:59:41 PDT
Comment on attachment 4371 [details] proposed implementation Looks like a good fix. r=me
Comment 18 Alexey Proskuryakov 2005-11-12 07:44:27 PST
An apparently unrelated problem makes this patch cause a crash when running layout tests. Filed as bug 5717.