Bug 163581 - REGRESSION (r201471): Keyboard remains visible when swiping back on twitter.com
Summary: REGRESSION (r201471): Keyboard remains visible when swiping back on twitter.com
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-10-17 19:25 PDT by Ryosuke Niwa
Modified: 2016-10-18 13:42 PDT (History)
2 users (show)

See Also:


Attachments
Fixes the bug (2.26 KB, patch)
2016-10-17 20:22 PDT, Ryosuke Niwa
simon.fraser: review-
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Added a test (5.94 KB, patch)
2016-10-17 23:29 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Reverted unrelated change (5.25 KB, patch)
2016-10-17 23:53 PDT, Ryosuke Niwa
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 2016-10-17 19:25:40 PDT
Reproduction steps:
1. Login to twitter.com
2. Click "reply" on any tweet
3. Tap on the text field to bring up keyboard
4. Swipe back

Expected result: Keyboard goes away
Actual result: Keyboard stays open
Comment 1 Ryosuke Niwa 2016-10-17 19:27:00 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].
Comment 2 Ryosuke Niwa 2016-10-17 20:22:32 PDT
Created attachment 291912 [details]
Fixes the bug
Comment 3 Simon Fraser (smfr) 2016-10-17 21:18:14 PDT
Comment on attachment 291912 [details]
Fixes the bug

This is testable, via UIScriptController. Please ad a test.
Comment 4 Ryosuke Niwa 2016-10-17 21:58:55 PDT
(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?
Comment 5 Simon Fraser (smfr) 2016-10-17 22:03:42 PDT
Yes, and you get callbacks when the keyboard appears and disappears. See tests in fast/forms/ios
Comment 6 Ryosuke Niwa 2016-10-17 22:06:48 PDT
(In reply to comment #5)
> Yes, and you get callbacks when the keyboard appears and disappears. See
> tests in fast/forms/ios

Oh, neat!
Comment 7 Ryosuke Niwa 2016-10-17 22:59:01 PDT
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 :(
Comment 8 Ryosuke Niwa 2016-10-17 23:29:04 PDT
Created attachment 291922 [details]
Added a test
Comment 9 Ryosuke Niwa 2016-10-17 23:53:26 PDT
Created attachment 291926 [details]
Reverted unrelated change
Comment 10 Simon Fraser (smfr) 2016-10-18 08:37:51 PDT
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 11 Ryosuke Niwa 2016-10-18 12:11:08 PDT
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 12 WebKit Commit Bot 2016-10-18 13:42:46 PDT
Comment on attachment 291926 [details]
Reverted unrelated change

Clearing flags on attachment: 291926

Committed r207486: <http://trac.webkit.org/changeset/207486>
Comment 13 WebKit Commit Bot 2016-10-18 13:42:50 PDT
All reviewed patches have been landed.  Closing bug.