Summary: | WebHTMLView (WebNSTextInputSupport) - "characterIndexForPoint: not yet implemented" | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Evan Gross <evan> | ||||||||||||||
Component: | HTML Editing | Assignee: | Dave Hyatt <hyatt> | ||||||||||||||
Status: | VERIFIED FIXED | ||||||||||||||||
Severity: | Normal | CC: | ap | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 420+ | ||||||||||||||||
Hardware: | Mac | ||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||
Bug Depends on: | 5717 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Evan Gross
2005-10-02 00:22:52 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)?
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... 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. 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). (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"? (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 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! 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 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). 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.
Created attachment 4295 [details]
test results
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.
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 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.
(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 :) Filed the remaining [WebCoreBridge firstRectForDOMRange:] issue as bug 5401. Comment on attachment 4371 [details]
proposed implementation
Looks like a good fix. r=me
An apparently unrelated problem makes this patch cause a crash when running layout tests. Filed as bug 5717. |