Bug 56271 - focus() call inside input event handler is ignored
Summary: focus() call inside input event handler is ignored
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Minor
Assignee: Alice Boxhall
URL:
Keywords: HasReduction
Depends on: 38696
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-13 14:33 PDT by Ryosuke Niwa
Modified: 2017-07-18 08:29 PDT (History)
8 users (show)

See Also:


Attachments
demo (377 bytes, text/html)
2011-03-13 14:34 PDT, Ryosuke Niwa
no flags Details
Work in progress (10.65 KB, patch)
2011-03-16 17:35 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-03-13 14:33:11 PDT
When an input element is created and focus() is called on that element inside an input event handler of another input element, the focus stays on the input element from which the input event was fired and focus() call is effectively ignored.

http://crbug.com/75863
Comment 1 Ryosuke Niwa 2011-03-13 14:34:46 PDT
Created attachment 85623 [details]
demo

Here's a reduction attached on the Chromium bug.  To reproduce the bug, type in any character into the input element.  The focus be moved to the new input element when new one appears but it doesn't on ToT WebKit.
Comment 2 Tony Chang 2011-03-14 10:11:15 PDT
Isn't this because of http://trac.webkit.org/changeset/58829 ?  There was some additional discussion on bug 39791.
Comment 3 Ryosuke Niwa 2011-03-14 10:30:31 PDT
(In reply to comment #2)
> Isn't this because of http://trac.webkit.org/changeset/58829 ?  There was some additional discussion on bug 39791.

As far as I read the change, it only affects the case where the event handler sets the focus on a different frame.
Comment 4 Levi Weintraub 2011-03-14 10:42:43 PDT
Here is the backtrace for the call that blows away the javascript focus() call back to the original input.

#0	0x10192f249 in WebCore::FocusController::setFocusedNode at FocusController.cpp:353
#1	0x1021521ca in WebCore::SelectionController::setFocusedNodeIfNeeded at SelectionController.cpp:1686
#2	0x102156e28 in WebCore::SelectionController::setSelection at SelectionController.cpp:182
#3	0x1018cd70d in WebCore::Editor::changeSelectionAfterCommand at Editor.cpp:3077
#4	0x1018d7f72 in WebCore::Editor::appliedEditing at Editor.cpp:1071
#5	0x1022adc07 in WebCore::TypingCommand::typingAddedToOpenCommand at TypingCommand.cpp:349
#6	0x1022af405 in WebCore::TypingCommand::insertTextRunWithoutNewlines at TypingCommand.cpp:401
#7	0x1022af4ba in WebCore::TypingCommand::insertText at TypingCommand.cpp:375
#8	0x1022af668 in WebCore::TypingCommand::doApply at TypingCommand.cpp:299
#9	0x1018c4aa7 in WebCore::EditCommand::apply at EditCommand.cpp:92
#10	0x1018c4b44 in WebCore::applyCommand at EditCommand.cpp:224
#11	0x1022b039e in WebCore::TypingCommand::insertText at TypingCommand.cpp:200
#12	0x1018d6bb8 in WebCore::Editor::insertTextWithoutSendingTextEvent at Editor.cpp:1198
#13	0x1018d6d8c in WebCore::Editor::handleTextEvent at Editor.cpp:208
#14	0x1018ef3fc in WebCore::EventHandler::defaultTextInputEventHandler at EventHandler.cpp:2762
#15	0x101ee4597 in WebCore::Node::defaultEventHandler at Node.cpp:3033
#16	0x101a56370 in WebCore::HTMLFormControlElementWithState::defaultEventHandler at HTMLFormControlElement.cpp:538
#17	0x101a6b8b9 in WebCore::HTMLInputElement::defaultEventHandler at HTMLInputElement.cpp:1074
#18	0x101ee4e94 in WebCore::Node::dispatchGenericEvent at Node.cpp:2727
#19	0x101ee5139 in WebCore::Node::dispatchEvent at Node.cpp:2625
#20	0x1019069f6 in WebCore::EventTarget::dispatchEvent at EventTarget.cpp:297
#21	0x1018f4ed8 in WebCore::EventHandler::handleTextInputEvent at EventHandler.cpp:2727
#22	0x1018d1739 in WebCore::Editor::insertText at Editor.cpp:1149
Comment 5 Levi Weintraub 2011-03-14 10:48:10 PDT
It seems to me that Editor::appliedEditing or Editor::changeSelectionAfterCommand shouldn't change the focused node, so it should perhaps ensure it's trying to update the selection on the currently focused node and bail if it's not? I'll see if that behavior matches FFX before assuming that doesn't break anything else.
Comment 6 Levi Weintraub 2011-03-14 15:38:22 PDT
(In reply to comment #5)
> It seems to me that Editor::appliedEditing or Editor::changeSelectionAfterCommand shouldn't change the focused node, so it should perhaps ensure it's trying to update the selection on the currently focused node and bail if it's not? I'll see if that behavior matches FFX before assuming that doesn't break anything else.

I tried a basic fix in Editor::changeSelectionAfterCommand, where I only change selection if the new selection occurs in the same context as the currently focused node. This does cause some changes to selection behavior in our already present layout tests. Aside from EditingDelegate changes, selections when dragging content changes: the content dragged in isn't selected.

This is like FFX (though notably FFX doesn't support dragging from one editable context to another... the original contents are lost and not pasted in!). 

IE9 highlights the dragged text in the destination, but doesn't support the onInput event listener. Trying to use "addEventListener" as they describe in their documentation (http://msdn.microsoft.com/en-us/library/gg592978(v=vs.85).aspx) results in a javascript exception stating that the input tag doesn't support "addEventListener".

I'm inclined towards changing our behavior, but understand that these changes may make it less than desirable. Opinions?
Comment 7 Levi Weintraub 2011-03-14 15:49:09 PDT
(In reply to comment #6)
> IE9 highlights the dragged text in the destination, but doesn't support the onInput event listener. Trying to use "addEventListener" as they describe in their documentation (http://msdn.microsoft.com/en-us/library/gg592978(v=vs.85).aspx) results in a javascript exception stating that the input tag doesn't support "addEventListener".

Ryosuke pointed out that I ran into this because I was in Quirks mode (oops!). IE9 then passes the attached test case.
Comment 8 Levi Weintraub 2011-03-14 17:39:09 PDT
After some more testing, I've identified that IE9 and Firefox do not set the focus when setting the selection, which SelectionController::setFocus does indiscriminately. Fixing this behavior should also fix this bug. The fix should be easy, but call sites that expect the focus to be set along with selection will need to be updated to respect the new contract, and lots of layout tests will have to be rebaselined with the changed editing delegates.

I'm happy to rename the bug and take it, but I'll defer to Alice if she still wants to fix it.
Comment 9 Alice Boxhall 2011-03-14 17:45:25 PDT
(In reply to comment #8)
> After some more testing, I've identified that IE9 and Firefox do not set the focus when setting the selection, which SelectionController::setFocus does indiscriminately. Fixing this behavior should also fix this bug. The fix should be easy, but call sites that expect the focus to be set along with selection will need to be updated to respect the new contract, and lots of layout tests will have to be rebaselined with the changed editing delegates.
> 
> I'm happy to rename the bug and take it, but I'll defer to Alice if she still wants to fix it.

I had a look at this bug with Ojan yesterday and we found the same thing.

I'm unwell today so I wouldn't get to it till tomorrow at the earliest, so I'm just as happy for you to take it if you'd like.
Comment 10 Levi Weintraub 2011-03-16 17:35:28 PDT
Created attachment 86008 [details]
Work in progress

This affects the editing delegates for tons of tests, so it'll be a large rebaselining effort. I'll also add a test case.

This has the possibility of breaking compatibility with sites that rely on WebKit's current behavior. I'd love to get confirmation that we think this change is the right way to go. Darin? AP?