Bug 4394 – Mouse clicks ignored in inline input areas

• Component: HTML Editing
• Status: VERIFIED FIXED

[Alexey Proskuryakov]

Mouse clicks in inline input areas are not forwarded to input methods.

Steps to reproduce:
1. Open WebCore/layout-tests/editing/inserting/typing-001.html
2. With any input method that uses inline input (such as Kotoeri Hiragana), type something
3. Click in the middle of the inline input area
  The insertion point is correctly positioned where you have click; the input area doesn't close
4. Continue typing

Results: text is appended to the end of the inline input area

Discussion: I believe this happens because mouse events are not being correctly sent to TSM.

[Evan Gross]

(In reply to comment #0)
> Mouse clicks in inline input areas are not forwarded to input methods.
> 
... 
> Discussion: I believe this happens because mouse events are not being correctly sent to TSM.

Yep, this is definitely a problem. There is a comment in -mouseDown:

    // TEXTINPUT: if there is marked text and the current input
    // manager wants to handle mouse events, we need to make sure to
    // pass it to them. If not, then we need to notify the input
    // manager when the marked text is abandoned (user clicks outside
    // the marked area)

But I can't see where the above is actually being done. Some portion of the above functionality IS 
provided when _selectionChanged is called. However, what happens in _selectionChanged is not totally 
correct - the actual mouse down event does not actually get passed through to TSM (and therefore to 
the input method itself).

The impression is given that the line of code that follows the above comment does this:

    [_private->compController endRevertingChange:NO moveLeft:NO];

But it's solely for dealing with completion, and has nothing to do with the above comment.

So it looks like there is some not-yet-written code associated with that comment.

Any idea how this should be done? NSTextView does it properly...

[Alexey Proskuryakov]

Unfortunately, I am not familiar with using TSM from Cocoa... Yet, looks like there's enough evidence to at 
least mark this confirmed :)

[Alexey Proskuryakov]

  Looks like 
[[NSInputManager currentInputManager] handleMouseEvent:event];

  at this point almost does the job. That is, an event gets forwarded, but with a wrong character index 
(because -[WebHTMLView(WebNSTextInputSupport) characterIndexForPoint:] is simply not implemented 
yet).

[Alexey Proskuryakov]

Created attachment 3862 [details]
proposed patch

I think that the problem with characterIndexForPoint is a separate one -
Kotoeri itself asks for the index while processing a mouse event.

Because of that, and because this patch solves the problem for my own and,
presumably, Evan's input methods, I think characterIndexForPoint should be
handled separately.

[Darin Adler]

Comment on attachment 3862 [details]
proposed patch

Patch looks great.

I don't think that the check for clcking outside the marked text is good
enough. To communicate the rectangle to the input manager, it's true that we
have to use only "first rect for character range", but for us internally, we
should be able to accurately determine if a click is in the marked text or not.
So the check should not be to get a rect and check if the mouse is in the rect.
Instead we should be passing the point over to WebCore to let it determine if
the click is outside the marked text.

Formatting nitpick: This code uses formatting like "NSEvent *", but the new
code says "NSInputManager*" with no space. I'd love for this to be consistent
across the entire WebKit project, either way. Unfortunately we have a mix.

[Alexey Proskuryakov]

Created attachment 3879 [details]
updated patch

Re-implemented using editableDOMRangeForPoint, markedTextDOMRange and
compareBoundaryPoints.

There are at least two things I don't really understand about that code path:
- What are the input managers that don't want to handle mouse events and why we
should treat them as described in the comment (AFAIK, TSM input methods always
handle mouse events).
- What editableDOMRangeForPoint actually is - I am always getting a range with
start==end from this method. Anyhow, I'm handling it as a real range, just in
case.

[Darin Adler]

Comment on attachment 3879 [details]
updated patch

I'd like to see the "DOM ranges intersect" code factored into a separate
function.

editableDOMRangeForPoint does always return a range with start == end. It uses
DOMRange just because there's no "DOMPoint".

I think this is a good change, r=me.

I'm concerned, however, that we don't have anyone testing the Japanese and
Chinese input methods. It's was quite challenging getting them working with
WebKit and Mail for Tiger, and I worry that changes in this area will result in
regressions in their behavior.

[Alexey Proskuryakov]

(In reply to comment #7)
> I'd like to see the "DOM ranges intersect" code factored into a separate function.

Should it be something like -(bool)intersectsDOMRange:(DOMRange*)other in DOMRange itself? I'm 
wondering what would be the right way to treat end points... What seems natural to me is that the left 
end point is included in the range, and the right one isn't:
[0,0) intersects [0,1)
[0,1) doesn't intersect [1,1)
[0,1) doesn't intersect [1, 2)

Does it make sense for DOMRanges?

> I worry that changes in this area will result in regressions in their behavior.

I expect quite a number of changes after this one and those Evan is proposing are finished - we need 
them as a baseline for more involved ones...

[Darin Adler]

The idea of including the start but not the end doesn't really make sense for a general purpose 
DOMRange method. It's specific to a use of DOM ranges rather than the fundamental concept. So I 
could see that in a helper function but not in a DOMRange method.

I typically think of DOM ranges as describing sets of DOM content, so if I was writing an intersection 
test, then it would return false for ranges that touch only at end points and an empty range would not 
even intersect with itself. That's the kind of general purpose DOM range intersection function that I 
think should be around as a general purpose helper.

Another kind of helper would be a "Is this DOM point in this DOM range?" function. For that, we pass 
two ranges and use the start of one range as the point. That's the helper that would be directly 
applicable for this patch, and as you say we'd have to define whether "in this DOM range" includes the 
endpoints or not.

[Darin Adler]

I did some research, and the rule in AppKit is that selections at the end of the inline input area *do* count 
as "within the marked text", so I'm going to mimic that rule.

But we have no way to test this unless we come up with an input method that does inline input but does 
not handle mouse clicks. I don't know of one.

[Darin Adler]

I don't know why this didn't occur to me earlier, but it's incorrect to unmark the text because of the mouse 
down event. That should be done in response to a selection change, not at the time you click.

[Alexey Proskuryakov]

(In reply to comment #10)
> I did some research, and the rule in AppKit is that selections at the end of the inline input 
> area *do* count as "within the marked text", so I'm going to mimic that rule.

FWIW, NSTextView has an obvious bug in this situation:
1) Open a new document in TextEdit
2) Type something with Kotoeri, do not close the inline input session
3) Click somewhere to the right of the marked text

Results: the caret gets positioned _before_ the last character in the inline hole.

[Alexey Proskuryakov]

(In reply to comment #11)
Hmm... If the selection changes because an arrow key is pressed, the inline area shouldn't get 
automatically confirmed. How are mouse clicks different?

Besides, in the situation that I described in the previous comment, the caret position shouldn't change 
(and it's arguable whether the inline area should be confirmed).

[Darin Adler]

I completely omitted the code to unmark text when you do mouse down. The comment claimed it was a 
good idea to do that, but it wasn't.

I also fixed a bug where we'd retain and not release in one of the methods.

[Alexey Proskuryakov]

(In reply to comment #14)

Filed the "DOM ranges intersect" suggestion as bug 5116.

Sorry for the over-retaining bug...

[Alexey Proskuryakov]

http://trac.webkit.org/projects/webkit/changeset/10612