Bug 20254 – didBeginEditing / didEndEditing not always called on focus change

• Component: WebKit API
• Status: NEW

[Joe Mason]

To diagnose this bug, add debug output to your platform's implementation of EditorClient::didBeginEditing and EditorClient::didEndEditing.  (I'm using the Qt port, which helpfully has debug output there already if dumpEditingCallbacks is turned on.)

Expected: didBeginEditing is called when a textarea gains focus, and didEndEditing is called when it loses it.  textFieldDidBeginEditing is called when an input element gains focus, and textFieldDidEndEditing is called when it loses it.

Observed: didBeginEditing is never called.  didEndEditing is sometimes called.

The Wikipedia sandbox is a good page to use to test this, since it contains both types of text entry.

These methods are mainly used by the Qt port to send Qt signals that clients can catch, but WebEditorClient also implements textFieldDidBeginEditing and textFieldDidEndEditing to do domething with form delegates that I can't really follow.  (I *think* it's just passing the signal on.)

In the safari-3.1 branch textFieldDidBeginEditing and textFieldDidEndEditing were called consistently, but didBeginEditing and didEndEditing were not called when moving from a textarea to an input field, or a textarea to another textarea.  In the most recent version it doesn't work at all.

[Joe Mason]

Created attachment 22598 [details]
Patch to fix this bug

The attached patch fixes this as follows:

Added Frame::textAreaDidBeginEditing and Frame::textAreaDidEndEditing to
parallel textFieldDidBeginEditing and textFieldDidEndEditing.  (Now
textAreaDidBeginEditing calls EditorClient::didBeginEditing, as before, while
textFieldDidBeginEditing calls EditorClient::textFieldDidBeginEditing.)

Remove the calls to didBeginEditing() and didEndEditing() in
Document::setFocusedNode.  They were not being called consistently. 
didBeginEditing and didEndEditing are now called from a DOM node's
dispatchFocusEvent or dispatchBlurEvent, the same as textFieldDidBeginEditing
and textFieldDidEndEditing.  (I think this change is safe because no
EditorClient implementation overrode these functions except EditorClientQt,
which just uses it to send the startInput and stopInput signals, which I've tested extensively.)

Add a missed call to textFieldDidBeginEditing in HTMLInputElement::dispatchFocusEvent.

Add calls to the new Frame::textAreaDidBeginEditing and
Frame::textAreaDidEndEditing to HTMLTextAreaElement to parallel the calls to
textFieldDidBeginEditing and textFieldDidEndEditing in HTMLInputElement.

Add a call to the new Frame::textAreaDidBeginEditing to
RenderTextControl::subtreeHasChanged to parallel the textFieldBeginEditing call
that's already there.


With these changes, EditorClient::didEditing and EditorClient::endEditing
should be called in exactly the same circumstances that
EditorClient::textFieldBeginEditing and EditorClient::textFieldEndEditing are,
except for textarea nodes instead of textfield nodes.

[Joe Mason]

Forgot to mention, the above patch also adds a call to the inherited aboutToUnload() in HTMLInputElement::aboutToUnload().  This is just because most other methods involved (ie. dispatchFocusEvent and dispatchBlurEvent) call the inherited implementation, so I'm assuming this call was just missed.  The inherited aboutToUnload() is empty, so this doesn't do anything now, but it's future-proofing in case someone adds code higher in the inheritance chain and expects it to be called.

[Joe Mason]

Created attachment 22639 [details]
Patch that applies cleanly

The patch above didn't apply cleanly.  This one does.

[Joe Mason]

Created attachment 22641 [details]
Same patch with ChangeLog

Added ChangeLog entries to patch

[Joe Mason]

Comment on attachment 22641 [details]
Same patch with ChangeLog

Sigh, I can't read - that's requestEE, not requestER.

[Eric Seidel (no email)]

Comment on attachment 22641 [details]
Same patch with ChangeLog

Justin should look at this.

[Eric Seidel (no email)]

Ping, justin?

[Eric Seidel (no email)]

Ping, Justin?

[Justin Garcia]

(In reply to comment #8)
> Ping, Justin?

Did you run the editing layout test?  I don't think you call didBegin/EndEditing when the editable region was a contentEditable element.
Could you explain why calling didBegin/EndEditing from setFocusedNode isn't good enough?  In what situations do we fail to call didBegin/EndEditing on a focus change?
Before we called editor()->didBegin/EndEditing, now we call editor()->client()->didBegin/EndEditing(), is that OK/intended?
Why are we naming this new functions textAreaBegin/EndEditing?  It isn't always in a text area that we are doing editing.

[Eric Seidel (no email)]

Comment on attachment 22641 [details]
Same patch with ChangeLog

Since this is now waiting pending comments from Joe, I'm marking this r- to remove it from the queue.