Bug 217148 – [macCatalyst] Crash when attempting to display a select dropdown in a UIWebView

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Aditya Keerthi]

...

[Aditya Keerthi]

<rdar://problem/64004677>

[Aditya Keerthi]

Created attachment 410160 [details]
Patch

[Darin Adler]

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).

[Aditya Keerthi]

Created attachment 410192 [details]
Patch

[Aditya Keerthi]

(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.

[EWS]

Committed r267845: <https://trac.webkit.org/changeset/267845>

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