Bug 18510 – Text controls only emit the start editing signal after the first keypress

• Component: Forms
• Status: RESOLVED INVALID

[iain]

I'm writing a backend for Clutter and I was hooking up an onscreen keyboard to WebKit so that it would area when the user selected a text control. I was using EditorClient::textFieldDidBeginEditing() to control when the keyboard appeared, but this method is only called once the user has entered something in the text area. There we get a chicken/egg problem, the keyboard can't appear until the user types something, but the user cant type something without the keyboard.

Attached is a patch that moves the place where EditorClient::textFieldDidBeginEditing() gets called from RenderTextControl::subtreeHasChanged() to HTMLTextFieldInnerTextElement::defaultEventHandler() although I don't know if this is the best way? Would it be preferable that there is an EditorClient::textFieldMaybeBeginEditing method as well as textFieldDidBeginEditing which would get called when the control gets focused and textFieldDidBeginEditing would be still called where it is now?

Or am I completely barking up the wrong tree here and completely missed something?

[iain]

Created attachment 20549 [details]
Patch to call textFieldDidStartEditing whenever a text control receives focus

[Alexey Proskuryakov]

Comment on attachment 20549 [details]
Patch to call textFieldDidStartEditing whenever a text control receives focus

Marking for review, so this doesn't get lost.

[Adele Peterson]

CCing Justin.  He was looking into this a while ago...

[Darin Adler]

Comment on attachment 20549 [details]
Patch to call textFieldDidStartEditing whenever a text control receives focus

Thanks for tackling this!

The indentation of the code looks wrong, and part of the patch is only this indentation change on some existing code.

+	    HTMLInputElement* input = static_cast<HTMLInputElement*>(shadowAncestor);

This cast is incorrect. As you can see above, the shadowAncestor might be a HTMLTextAreaElement.

+	    static_cast<RenderTextControl*>(shadowAncestor->renderer())->document()->frame()->textFieldDidBeginEditing(input);

Doing this without any null checks is almost certainly incorrect. It may be possible for events to be dispatched in a document that no longer is in a frame, for example. And the old code that's being replaced was doing the null checks.

As with any WebKit bug fix, the patch needs to include a regression test that demonstrates the problem. And a ChangeLog entry.

[iain]

Created attachment 21524 [details]
Patch to call textFieldDidStartEditing whenever a text control receives focus

[iain]

> The indentation of the code looks wrong, and part of the patch is only this
> indentation change on some existing code.

Ah, yes, I had emacs in indent tab mode, but WebKit uses spaces to indent.

> As with any WebKit bug fix, the patch needs to include a regression test that
> demonstrates the problem. And a ChangeLog entry.

The new patch includes a ChangeLog entry and fixes all your comments above.

I'm not entirely sure where regression tests go, and to be honest I'm not sure how one would go about writing an automated test for what is more a change of behaviour rather than a bug per say. But maybe if I see what the other regression tests look like I'll have an epiphany.

[iain]

I've just realised (well, alp told me) that what I'm looking for here is provided by the EditorClient::setInputMethodState() so I'm closing this as invalid. Apologies for not noticing it sooner.