| Summary: | [iOS] Crash when changing inputmode for certain types of focusable elements | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||
| Component: | WebCore Misc. | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bdakin, cdumez, commit-queue, rniwa, thorton, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | Other | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Wenson Hsieh
2019-03-30 17:16:06 PDT
Alarmingly easy to repro :/ Created attachment 366370 [details]
Fixes the bug
Comment on attachment 366370 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=366370&action=review > Source/WebKit/WebProcess/WebPage/WebPage.cpp:5317 > { > - if (!m_focusedElement) > + if (!m_focusedElementForUserInput) > return; I'm not sure this rename is totally necessary, especially given that you didn't rename all the things adjacent to it (like, say, this function). > Source/WebKit/WebProcess/WebPage/WebPage.cpp:5356 > + return is<HTMLSelectElement>(element) || isTextFormControlOrEditableContent(element); This seems ... like an oddly small set of cases? Or maybe the function name is wrong? Any element "may require user input" if it responds to events? Comment on attachment 366370 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=366370&action=review Thanks for the review! >> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5317 >> return; > > I'm not sure this rename is totally necessary, especially given that you didn't rename all the things adjacent to it (like, say, this function). Fair enough! I'll leave the renaming for a followup, then. I do think it's quite a footgun that Document has m_focusedElement and WebPage also has m_focusedElement, but they are about different concepts. >> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5356 >> + return is<HTMLSelectElement>(element) || isTextFormControlOrEditableContent(element); > > This seems ... like an oddly small set of cases? Or maybe the function name is wrong? Any element "may require user input" if it responds to events? Yeah, I think the function name could be better. I originally went with mayRequireInputView, but it seemed too iOS-specific, so I thought "user input" was sufficiently general. I'll leave this change out for now. Created attachment 366378 [details]
Patch for landing
Comment on attachment 366378 [details] Patch for landing Clearing flags on attachment: 366378 Committed r243684: <https://trac.webkit.org/changeset/243684> |