Bug 217148 - [macCatalyst] Crash when attempting to display a select dropdown in a UIWebView
Summary: [macCatalyst] Crash when attempting to display a select dropdown in a UIWebView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Other
Hardware: Other Unspecified
: P2 Normal
Assignee: Aditya Keerthi
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-30 15:27 PDT by Aditya Keerthi
Modified: 2020-10-01 11:32 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.15 KB, patch)
2020-09-30 15:51 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (3.57 KB, patch)
2020-09-30 19:31 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2020-09-30 15:27:30 PDT
...
Comment 1 Aditya Keerthi 2020-09-30 15:28:11 PDT
<rdar://problem/64004677>
Comment 2 Aditya Keerthi 2020-09-30 15:51:11 PDT
Created attachment 410160 [details]
Patch
Comment 3 Darin Adler 2020-09-30 15:56:57 PDT
Comment on attachment 410160 [details]
Patch

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

Seems OK, but could be even better.

> Source/WebKitLegacy/mac/ChangeLog:33
> +        This change is only made for Catalyst, in order to avoid compatibility
> +        issues.

Could you elaborate on this? I understand there may be compatibility risk, but aside from the risk, could this be helpful on Mac outside Catalyst?

> Source/WebKitLegacy/mac/ChangeLog:35
> +        No new tests as the functionality is dependent on a UIKit change. 

I am concerned about something so subtle not being covered by a test.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:4655
> +#if PLATFORM(MACCATALYST)

In a case like this where there is no obvious reason for a platform difference, I think we need a comment explaining the reason we chose a different behavior between platforms. This is not just leaving out some code that deals with a platform-specific concept (like the secure input state and completion controller on Mac and dictation markers on iOS above).

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:4657
> +            if ([[self _webView] _isPerformingProgrammaticFocus])
> +                return resign;

It seems a slightly unsafe pattern to use an early return here; does not seem like the "null frame" and "null page" cases above. Could we instead just "skip the setFocused call" even if that makes the patch bigger? Like a boolean "shouldClearFocus" that is a constant outside PLATFORM(MACCATALYST).
Comment 4 Aditya Keerthi 2020-09-30 19:31:20 PDT
Created attachment 410192 [details]
Patch
Comment 5 Aditya Keerthi 2020-09-30 19:39:57 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 410160 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410160&action=review
> 
> Seems OK, but could be even better.
> 
> > Source/WebKitLegacy/mac/ChangeLog:33
> > +        This change is only made for Catalyst, in order to avoid compatibility
> > +        issues.
> 
> Could you elaborate on this? I understand there may be compatibility risk,
> but aside from the risk, could this be helpful on Mac outside Catalyst?

After looking at the existing cases where [WebView _isPerformingProgrammaticFocus] is true, I decided to apply the change to all platforms. The explanation is in the Changelog, but I will give a brief one here too:

There's currently only one codepath where [WebView _isPerformingProgrammaticFocus] is true and its when the first responder is resigned as a result of a call to WebChromeClient::makeFirstResponder. That method calls a WebUIDelegate method which guarantees the new responder "is in the view subhierarchy of the top-level web view for this WebView". This means that `nextResponderIsInWebView` is always true when [WebView _isPerformingProgrammaticFocus] is true.
Comment 6 EWS 2020-10-01 11:32:14 PDT
Committed r267845: <https://trac.webkit.org/changeset/267845>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410192 [details].