Bug 196264 – [iOS][WK2] Use a better concept to describe the reason we defer zooming a focused element: selectabiltiy

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Daniel Bates]

In iOS WebKit2 we have made a "thing" (or concept) out of deferred zooming of focused elements. We wrapped up this concept into a helper function called shouldDeferZoomingToSelectionWhenRevealingFocusedElement() that tells us whether we should defer zooming or not. The problem is that this seems like an indirect way to describe the actual concept the UI process is talking about: text/element selectability – whether the focused element has selectable text (like a text field). I think this is the concept we are trying to describe. The deferment of zooming is a side effect of an implementation detail that zooming of selectable elements reveals the selection and we may not have an up-to-date selection rect to reveal sometimes. When we don't have an up-to-date selection rect then we need to defer zooming until we do.

[Daniel Bates]

Created attachment 365982 [details]
Patch

[Daniel Bates]

Comment on attachment 365982 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4825
> +    case WebKit::InputType::DateTime:

Note that this is a new case. Old code didn't consider DataTime, but we should. It acts like a text field (or at least does on the Mac).

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4835
> +    case WebKit::InputType::Week:

Note that this is a new case. Old code didn't consider Week, but we should. It acts like a text field (or at least does on the Mac). This type appears to behave weirdly on iOS currently. We need to fix bug #158327.

[Daniel Bates]

Comment on attachment 365982 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4808
> +static bool mayBeSelectable(WebKit::InputType type)

Wenson wants something more precise, mayContainSelectableText.

[Wenson Hsieh]

Comment on attachment 365982 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4808
> +static bool mayBeSelectable(WebKit::InputType type)

Nit - mayContainSelectableText?

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4835
>> +    case WebKit::InputType::Week:
> 
> Note that this is a new case. Old code didn't consider Week, but we should. It acts like a text field (or at least does on the Mac). This type appears to behave weirdly on iOS currently. We need to fix bug #158327.

Nit - maybe leave a FIXME referencing the bug number (158327) here/

[Daniel Bates]

(In reply to Wenson Hsieh from comment #4)
> >> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4835
> >> +    case WebKit::InputType::Week:
> > 
> > Note that this is a new case. Old code didn't consider Week, but we should. It acts like a text field (or at least does on the Mac). This type appears to behave weirdly on iOS currently. We need to fix bug #158327.
> 
> Nit - maybe leave a FIXME referencing the bug number (158327) here/

I am not going to do this because it does not make sense to me to do this. This was just a remark to the reader, you, of this patch to point out that the existing code did not handle <input type="week"> – that in and of itself was a bug in the old code. I mentioned bug #158327 because it turns out (coincidentally?) that there are more bugs around <input type="week"> on iOS. So, the omission of WebKit::InputType::Week turns out not to be the thing that broke the <input type="week">, but it still wasn't correct.

[Daniel Bates]

Created attachment 366010 [details]
To Land

[Daniel Bates]

Comment on attachment 366010 [details]
To Land

Clearing flags on attachment: 366010

Committed r243520: <https://trac.webkit.org/changeset/243520>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49294878>