Bug 18510 - Text controls only emit the start editing signal after the first keypress
Summary: Text controls only emit the start editing signal after the first keypress
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-15 04:22 PDT by iain
Modified: 2008-06-09 07:56 PDT (History)
2 users (show)

See Also:


Attachments
Patch to call textFieldDidStartEditing whenever a text control receives focus (2.00 KB, patch)
2008-04-15 04:24 PDT, iain
darin: review-
Details | Formatted Diff | Diff
Patch to call textFieldDidStartEditing whenever a text control receives focus (1.70 KB, patch)
2008-06-06 05:19 PDT, iain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description iain 2008-04-15 04:22:44 PDT
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?
Comment 1 iain 2008-04-15 04:24:10 PDT
Created attachment 20549 [details]
Patch to call textFieldDidStartEditing whenever a text control receives focus
Comment 2 Alexey Proskuryakov 2008-04-15 08:08:24 PDT
Comment on attachment 20549 [details]
Patch to call textFieldDidStartEditing whenever a text control receives focus

Marking for review, so this doesn't get lost.
Comment 3 Adele Peterson 2008-04-27 22:31:56 PDT
CCing Justin.  He was looking into this a while ago...
Comment 4 Darin Adler 2008-05-24 23:20:10 PDT
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.
Comment 5 iain 2008-06-06 05:19:40 PDT
Created attachment 21524 [details]
Patch to call textFieldDidStartEditing whenever a text control receives focus
Comment 6 iain 2008-06-06 05:23:15 PDT
> 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.
Comment 7 iain 2008-06-09 07:56:06 PDT
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.