Bug 198922

Summary:

REGRESSION (r240757): Cannot dismiss the keyboard on http://apple.com/apple-tv-plus

Product:

WebKit

Reporter:

Daniel Bates <dbates>

Component:

WebKit Misc.

Assignee:

Daniel Bates <dbates>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar, Regression

Version:

WebKit Local Build

Hardware:

iPhone / iPad

OS:

iOS 12

URL:

http://apple.com/apple-tv-plus

See Also:

https://bugs.webkit.org/show_bug.cgi?id=198928

Bug Depends on:

Bug Blocks:

190571

Attachments:

Description	Flags
For the Bots	none
Patch	none
Simpler fix, but no insurance policy.... let's see what happens.	none
Patch	none
To land	none

Description Daniel Bates 2019-06-17 10:24:00 PDT

Steps to reproduce:

Ensure no hardware keyboard is attached.

1. Visit <http://apple.com/apple-tv-plus>
2. Tap Notify me.
3. Tap email address field to focus.
4. Tap X to close “Notify me” overlay.
5. Tap the Done button (iPhone) or keyboard dismissal button (iPad).

Then keyboard stays on screen. But the keyboard should have been dismissed.

Workaround: Click some editable element on the page or form field. Then you can dismiss the keyboard.

Comment 1 Daniel Bates 2019-06-17 10:24:12 PDT

<rdar://problem/50300056>

Comment 2 Daniel Bates 2019-06-17 10:26:53 PDT

Created attachment 372253 [details]
For the Bots

Comment 3 Daniel Bates 2019-06-17 12:06:08 PDT

Need to reduce <http://apple.com/apple-tv-plus>.

Comment 4 Daniel Bates 2019-06-17 12:06:39 PDT

Created attachment 372262 [details]
Patch

Comment 5 Daniel Bates 2019-06-17 13:39:37 PDT

Created attachment 372268 [details]
Simpler fix, but no insurance policy.... let's see what happens.

Comment 6 Daniel Bates 2019-06-18 13:29:44 PDT

Created attachment 372374 [details]
Patch

Comment 7 Daniel Bates 2019-06-18 13:32:24 PDT

Simpler fix going into bug #198928.

Comment 8 Wenson Hsieh 2019-06-18 13:34:28 PDT

Comment on attachment 372374 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372374&action=review

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1228
> +            [self _elementDidBlur];

I think the name “elementDidBlur” is a bit misleading now. When invoked from this call site, it’s more like “elementWillBlur”. But when invoked via IPC from the web process, it’s still “elementDidBlur”.

Maybe we could use a more generic name. handleElementBlur? Or perhaps cleanUpAfterElementBlur?

Comment 9 Daniel Bates 2019-06-18 13:35:33 PDT

Reduction:

<!DOCTYPE html>
<html>
<body>
<p>Tap the text field. Then tap the main page's background. Then dismiss the keyboard.</p>
<iframe src="data:text/html,<input>"></iframe>
</body>
</html>

Comment 10 Daniel Bates 2019-06-18 14:44:12 PDT

Comment on attachment 372374 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372374&action=review

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1228
>> +            [self _elementDidBlur];
> 
> I think the name “elementDidBlur” is a bit misleading now. When invoked from this call site, it’s more like “elementWillBlur”. But when invoked via IPC from the web process, it’s still “elementDidBlur”.
> 
> Maybe we could use a more generic name. handleElementBlur? Or perhaps cleanUpAfterElementBlur?

I hope you do not mind that I leave this for now. I could add an inline function called handleElementBlur that turns around and calls _elementDidBlur, but I don't think that improves the code much. Same for cleanUpAfterElementBlur, except that name raises more questions that it answers if I keep the implementation in _elementDidBlur. I could go another way at this and make _elementDidBlur be a one-line function that turns around and calls -cleanUpAfterElementBlur, which has the original impl. 🤷‍♂️. I keep thinking about this though.

Maybe I add a comment above _elementDidBlur call:

// Don't wait for the round-trip to clear out the focused element and hide the keyboard as the user explicitly instructed us to hide.

Comment 11 Daniel Bates 2019-06-18 14:47:35 PDT

Wrote comment:

Don't wait for WebPageProxy::blurFocusedElement() to round-trip back to us to hide the keyboard because we know that the user explicitly requested us to do so.

Comment 12 Daniel Bates 2019-06-18 14:49:39 PDT

Created attachment 372386 [details]
To land

Comment 13 Daniel Bates 2019-06-18 14:50:27 PDT

Committed r246570: <https://trac.webkit.org/changeset/246570>

Comment 14 Wenson Hsieh 2019-06-18 15:29:17 PDT

(In reply to Daniel Bates from comment #11)
> Wrote comment:
> 
> Don't wait for WebPageProxy::blurFocusedElement() to round-trip back to us
> to hide the keyboard because we know that the user explicitly requested us
> to do so.

👍🏻