Summary: | Text controls only emit the start editing signal after the first keypress | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | iain <iain> | ||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED INVALID | ||||||||
Severity: | Normal | CC: | adele, justin.garcia | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | Linux | ||||||||
Attachments: |
|
Description
iain
2008-04-15 04:22:44 PDT
Created attachment 20549 [details]
Patch to call textFieldDidStartEditing whenever a text control receives focus
Comment on attachment 20549 [details]
Patch to call textFieldDidStartEditing whenever a text control receives focus
Marking for review, so this doesn't get lost.
CCing Justin. He was looking into this a while ago... 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.
Created attachment 21524 [details]
Patch to call textFieldDidStartEditing whenever a text control receives focus
> 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. 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. |