Bug 196264 - [iOS][WK2] Use a better concept to describe the reason we defer zooming a focused element: selectabiltiy
Summary: [iOS][WK2] Use a better concept to describe the reason we defer zooming a foc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 12
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-26 11:48 PDT by Daniel Bates
Modified: 2019-03-26 21:16 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.50 KB, patch)
2019-03-26 12:01 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land (6.54 KB, patch)
2019-03-26 15:07 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2019-03-26 11:48:02 PDT
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.
Comment 1 Daniel Bates 2019-03-26 12:01:24 PDT
Created attachment 365982 [details]
Patch
Comment 2 Daniel Bates 2019-03-26 12:04:08 PDT
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.
Comment 3 Daniel Bates 2019-03-26 12:09:05 PDT
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.
Comment 4 Wenson Hsieh 2019-03-26 12:13:12 PDT
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/
Comment 5 Daniel Bates 2019-03-26 15:03:53 PDT
(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.
Comment 6 Daniel Bates 2019-03-26 15:07:14 PDT
Created attachment 366010 [details]
To Land
Comment 7 Daniel Bates 2019-03-26 15:08:55 PDT
Comment on attachment 366010 [details]
To Land

Clearing flags on attachment: 366010

Committed r243520: <https://trac.webkit.org/changeset/243520>
Comment 8 Daniel Bates 2019-03-26 15:08:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-03-26 15:11:48 PDT
<rdar://problem/49294878>