Summary: | REGRESSION (r201471): Keyboard remains visible when swiping back on twitter.com | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
Component: | UI Events | Assignee: | Ryosuke Niwa <rniwa> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, simon.fraser | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Safari Technology Preview | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2016-10-17 19:25:40 PDT
<rdar://problem/27739558> 8/16/16, 1:54 PM Andy Estes: Hiding the keyboard is a side effect of dispatching the blur event. In Element::dispatchBlurEvent(), we call ChromeClient::elementDidBlur(), which sends the StopAssistingNode message to the UI process, which eventually calls -[WKContentView _stopAssistingNode]. Created attachment 291912 [details]
Fixes the bug
Comment on attachment 291912 [details]
Fixes the bug
This is testable, via UIScriptController. Please ad a test.
(In reply to comment #3) > Comment on attachment 291912 [details] > Fixes the bug > > This is testable, via UIScriptController. Please ad a test. Does it bring up a real keyboard? Yes, and you get callbacks when the keyboard appears and disappears. See tests in fast/forms/ios (In reply to comment #5) > Yes, and you get callbacks when the keyboard appears and disappears. See > tests in fast/forms/ios Oh, neat! Huh, apparently all the tests in fast/forms/ios are skipped at the moment because we don't override [ Skip ] expectation in LayoutTests/platform/ios-simulator-wk2/TestExpectations :( Created attachment 291922 [details]
Added a test
Created attachment 291926 [details]
Reverted unrelated change
Comment on attachment 291926 [details] Reverted unrelated change View in context: https://bugs.webkit.org/attachment.cgi?id=291926&action=review > Source/WebCore/dom/Document.cpp:3676 > + // Match the order in HTMLTextFormControlElement::dispatchBlurEvent. > if (is<HTMLInputElement>(*oldFocusedElement)) > downcast<HTMLInputElement>(*oldFocusedElement).endEditing(); > + if (page()) > + page()->chrome().client().elementDidBlur(oldFocusedElement.get()); This is pretty confusing. Element::dispatchBlurEvent() calls chrome().client().elementDidBlur() BEFORE dispatching the blur event. If I look at HTMLTextFormControlElement::dispatchBlurEvent(), it doesn't have any calls to chrome().client().elementDidBlur(), but calls through to TextFieldInputType::handleBlurEvent() (note the *handle*) which calls endEditing(). So I can't tell from the comment whether the ordering is correct. I wish there were a single choke point for this blur handling. Comment on attachment 291926 [details] Reverted unrelated change View in context: https://bugs.webkit.org/attachment.cgi?id=291926&action=review >> Source/WebCore/dom/Document.cpp:3676 >> + page()->chrome().client().elementDidBlur(oldFocusedElement.get()); > > This is pretty confusing. Element::dispatchBlurEvent() calls chrome().client().elementDidBlur() BEFORE dispatching the blur event. > > If I look at HTMLTextFormControlElement::dispatchBlurEvent(), it doesn't have any calls to chrome().client().elementDidBlur(), but calls through to TextFieldInputType::handleBlurEvent() (note the *handle*) which calls endEditing(). So I can't tell from the comment whether the ordering is correct. I wish there were a single choke point for this blur handling. We don't want to dispatch blur event here. That's the whole point of the original patch. I intend to refactor this code once this patch is landed but this needs to be merged into a branch so I'm not interested in doing that kind of a large code change here, Comment on attachment 291926 [details] Reverted unrelated change Clearing flags on attachment: 291926 Committed r207486: <http://trac.webkit.org/changeset/207486> All reviewed patches have been landed. Closing bug. |