Make preserve and restore focus more likely to be symmetrical

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Make preserve and restore focus more likely to be symetrical

[Megan Gardner]

Created attachment 332612 [details]
Patch

[Megan Gardner]

<rdar://problem/36948473>

[Tim Horton]

Comment on attachment 332612 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:174
> +    RetainPtr<NSMutableArray> _focusStateStack;

Vector<bool> is likely a better plan/more efficient/more WebKitty

[Tim Horton]

Comment on attachment 332612 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Make preserve and restore focus more likely to be symetrical

Symmetrical

[Tim Horton]

Comment on attachment 332612 [details]
Patch

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

> Source/WebKit/ChangeLog:5
> +

There should be a radar here.

[Megan Gardner]

Created attachment 332614 [details]
Patch

[Wenson Hsieh]

Comment on attachment 332614 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Make preserve and restore focus more likely to be symetrical

symetrical => symmetrical

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4173
> +    BOOL shouldDecrement = _focusStateStack[lastIndex];

Nit - if (_focusStateStack.takeLast()) ... would read a bit more nicely here.

We should also bail early before trying to take the last index (with an assertion, perhaps) if the list is empty, to avoid crashing just in case we get an extra call to _restore.

[Wenson Hsieh]

Comment on attachment 332614 [details]
Patch

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

> LayoutTests/fast/forms/ios/ipad/select-form-run-twice.html:38
> +        function getSingleTapUIScript(x, y)

It doesn't look like we use this function anywhere.

[Megan Gardner]

Created attachment 332667 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 332667 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        Keep a stack of if we actually increment the focusState, so that
> +        changes to the web content do not result in asymmetric decrements to the focus state. 

Is this actually working around a UIKit bug? If so, can we reference that in a comment so we know if we can remove it in future?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4175
>          --_webView->_activeFocusedStateRetainCount;

This should also assert that _activeFocusedStateRetainCount isn't going to underflow.

[Megan Gardner]

Comment on attachment 332667 [details]
Patch

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

>> Source/WebKit/ChangeLog:10
>> +        changes to the web content do not result in asymmetric decrements to the focus state. 
> 
> Is this actually working around a UIKit bug? If so, can we reference that in a comment so we know if we can remove it in future?

It is, but even if they "fix" it, I don't know if we want to removed this, since it's small, and guards against bad behavior.

[Megan Gardner]

Created attachment 332675 [details]
Patch

[Tim Horton]

Comment on attachment 332675 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4176
>          --_webView->_activeFocusedStateRetainCount;

Should we not let this go negative?

[Megan Gardner]

https://trac.webkit.org/r227855