Bug 212580 - CSS "any-pointer:fine" media query false on iPad/Pencil
Summary: CSS "any-pointer:fine" media query false on iPad/Pencil
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari 13
Hardware: iPhone / iPad iOS 13
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-31 06:04 PDT by Patrick H. Lauke
Modified: 2020-10-13 17:03 PDT (History)
5 users (show)

See Also:


Attachments
Patch (36.92 KB, patch)
2020-10-08 10:02 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (37.09 KB, patch)
2020-10-08 10:07 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (36.06 KB, patch)
2020-10-08 14:24 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (37.25 KB, patch)
2020-10-09 13:49 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (37.27 KB, patch)
2020-10-09 14:10 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (38.43 KB, patch)
2020-10-12 10:58 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (38.37 KB, patch)
2020-10-12 14:14 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (37.95 KB, patch)
2020-10-12 18:07 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick H. Lauke 2020-05-31 06:04:34 PDT
Related to https://bugs.webkit.org/show_bug.cgi?id=209292 - testing iPad / iOS 13.5 / Pencil, "any-pointer:fine" evaluates to false when it should evaluate to true (if the Pencil's presence can be detected)

https://patrickhlauke.github.io/touch/pointer-hover-any-pointer-any-hover/results/index.html
Comment 1 Radar WebKit Bug Importer 2020-05-31 17:44:07 PDT
<rdar://problem/63813283>
Comment 2 Devin Rousso 2020-10-08 10:02:14 PDT
Created attachment 410855 [details]
Patch
Comment 3 Devin Rousso 2020-10-08 10:07:13 PDT
Created attachment 410856 [details]
Patch

add log when `changeTimeInterval` is overridden by `NSUserDefaults`
Comment 4 Sam Weinig 2020-10-08 10:41:39 PDT
Comment on attachment 410856 [details]
Patch

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

> Source/WebKit/ChangeLog:16
> +        Use `+[UIScribbleInteraction isPencilInputExpected]` as a proxy for the presence of stylus
> +        devices as there's no way of directly querying for connected stylus devices (especially if
> +        there is no active connection). When the value is changed to `YES`, notify all WebProcess
> +        immediately. When the value is changed to `NO`, use an `NSTimer` to delay notifying all
> +        WebProcess for 10min in case the user starts using their stylus again.
> +
> +        For testing/debugging purposes this 10min timeout can be adjusted before the UIProcess is
> +        created with the `WKStylusDeviceObserverChangeTimeInterval` key in `NSUserDefaults`.

I think this can and should be generalized for all platforms that support styluses. While the implementation to check for a stylus on iOS can't be shared, it would be good to create a more general pipeline for passing this information down to the WebProcess that is not iOS specific like this.

Additionally, please avoid using NSUserDefaults directly for testing. Rather, please add a new preference to the WebPreferences yaml files for your needs. This allows us to better keep track of all the settings in one place, rather than having them scattered around in different places.
Comment 5 Devin Rousso 2020-10-08 11:44:44 PDT
Comment on attachment 410856 [details]
Patch

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

>> Source/WebKit/ChangeLog:16
>> +        created with the `WKStylusDeviceObserverChangeTimeInterval` key in `NSUserDefaults`.
> 
> I think this can and should be generalized for all platforms that support styluses. While the implementation to check for a stylus on iOS can't be shared, it would be good to create a more general pipeline for passing this information down to the WebProcess that is not iOS specific like this.
> 
> Additionally, please avoid using NSUserDefaults directly for testing. Rather, please add a new preference to the WebPreferences yaml files for your needs. This allows us to better keep track of all the settings in one place, rather than having them scattered around in different places.

Could you give an example of what you mean by "generalized for all platforms"?  My thinking is that any other platform that wants to add this UIProcess->WebProcess communication of "we now have a stylus" (and the opposite) would adjust the `#if HAVE(PENCILKIT_TEXT_INPUT) && PLATFORM(IOS)` around `WebProcessProxy::notifyHasStylusDeviceChanged`, `WebProcess::setHasStylusDevice`, etc. and likely have it's own way of calling those functions based on it's own platform-specific way of knowing "we now have a stylus" (and the opposite).

When I said "testing" I mean this more as a QA form of testing (i.e. "live on"), where we could set the default and see how it feels (I chose 10min as a sort of gut guesstimate rather than a value backed by some collected metric/statistic).  The API tests included in this patch do not actually use `NSUserDefaults` for testing this behavior.  I initially went with `NSUserDefaults` because I wanted something global as we only really expect `WKStylusDeviceObserver` to be used from it's `sharedInstance`.  Is it possible for `WebPreferences` (or something similar) to be global and configurable like this, or is there perhaps another approach to getting the same outcome?
Comment 6 Tim Horton 2020-10-08 11:59:54 PDT
(In reply to Devin Rousso from comment #5)
> Comment on attachment 410856 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410856&action=review
> 
> >> Source/WebKit/ChangeLog:16
> >> +        created with the `WKStylusDeviceObserverChangeTimeInterval` key in `NSUserDefaults`.
> > 
> > I think this can and should be generalized for all platforms that support styluses. While the implementation to check for a stylus on iOS can't be shared, it would be good to create a more general pipeline for passing this information down to the WebProcess that is not iOS specific like this.
> > 
> > Additionally, please avoid using NSUserDefaults directly for testing. Rather, please add a new preference to the WebPreferences yaml files for your needs. This allows us to better keep track of all the settings in one place, rather than having them scattered around in different places.
> 
> Could you give an example of what you mean by "generalized for all
> platforms"?  My thinking is that any other platform that wants to add this
> UIProcess->WebProcess communication of "we now have a stylus" (and the
> opposite) would adjust the `#if HAVE(PENCILKIT_TEXT_INPUT) && PLATFORM(IOS)`
> around `WebProcessProxy::notifyHasStylusDeviceChanged`,
> `WebProcess::setHasStylusDevice`, etc. and likely have it's own way of
> calling those functions based on it's own platform-specific way of knowing
> "we now have a stylus" (and the opposite).

Pretty sure he means "leave the plumbing in place on all platforms, so they just have to add /callers/ of it". The plumbing will all be the same.
Comment 7 Devin Rousso 2020-10-08 14:24:26 PDT
Created attachment 410884 [details]
Patch
Comment 8 Sam Weinig 2020-10-09 10:42:40 PDT
Comment on attachment 410884 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:45
> +    if (WebProcessProxy::allProcesses().size() == 1) {
>  #if HAVE(UIKIT_WITH_MOUSE_SUPPORT) && PLATFORM(IOS)
> -    if (WebProcessProxy::allProcesses().size() == 1)
>          [[WKMouseDeviceObserver sharedInstance] start];
>  #endif
> +#if HAVE(PENCILKIT_TEXT_INPUT) && PLATFORM(IOS)

I don't think the PLATFORM(IOS) checks are needed here, since this file should only be compiled on iOS platforms.

> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:59
> +#if HAVE(PENCILKIT_TEXT_INPUT) && PLATFORM(IOS)
> +        [[WKStylusDeviceObserver sharedInstance] stop];
> +#endif

This seems like an error prone way to use a shared observer. If any other client ever get's created, it will be confusing that this call site completely stops it.

A common pattern for things like this is to have the shared instance maintain a reference count, and rather than explicitly starting and stoping the observer, the clients increment / decrement the count, and when it reaches 0, it stops observing.
Comment 9 Devin Rousso 2020-10-09 12:22:00 PDT
Comment on attachment 410884 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:45
>> +#if HAVE(PENCILKIT_TEXT_INPUT) && PLATFORM(IOS)
> 
> I don't think the PLATFORM(IOS) checks are needed here, since this file should only be compiled on iOS platforms.

Is there a way to connect a stylus/pencil to a simulator?  I didn't think there was, so I figured it wouldn't be desirable, but I suppose it couldn't hurt to have it.

>> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:59
>> +#endif
> 
> This seems like an error prone way to use a shared observer. If any other client ever get's created, it will be confusing that this call site completely stops it.
> 
> A common pattern for things like this is to have the shared instance maintain a reference count, and rather than explicitly starting and stoping the observer, the clients increment / decrement the count, and when it reaches 0, it stops observing.

Wow yeah I dunno why I didn't do that :|

I'll adjust `WKMouseDeviceObserver` too while im at it :)
Comment 10 Devin Rousso 2020-10-09 12:29:54 PDT
Comment on attachment 410884 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:45
>>> +#if HAVE(PENCILKIT_TEXT_INPUT) && PLATFORM(IOS)
>> 
>> I don't think the PLATFORM(IOS) checks are needed here, since this file should only be compiled on iOS platforms.
> 
> Is there a way to connect a stylus/pencil to a simulator?  I didn't think there was, so I figured it wouldn't be desirable, but I suppose it couldn't hurt to have it.

err, s/simulator/catalyst
Comment 11 Devin Rousso 2020-10-09 13:49:00 PDT
Created attachment 410970 [details]
Patch
Comment 12 Sam Weinig 2020-10-09 14:01:26 PDT
(In reply to Devin Rousso from comment #9)
> Comment on attachment 410884 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410884&action=review
> 
> >> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:45
> >> +#if HAVE(PENCILKIT_TEXT_INPUT) && PLATFORM(IOS)
> > 
> > I don't think the PLATFORM(IOS) checks are needed here, since this file should only be compiled on iOS platforms.
> 
> Is there a way to connect a stylus/pencil to a simulator?  I didn't think
> there was, so I figured it wouldn't be desirable, but I suppose it couldn't
> hurt to have it.

PLATFORM(IOS) is true for both the simulator and hardware.
Comment 13 Devin Rousso 2020-10-09 14:10:47 PDT
Created attachment 410972 [details]
Patch

rebase
Comment 14 Devin Rousso 2020-10-12 10:58:28 PDT
Created attachment 411132 [details]
Patch

add another test for when the stylus disconnected timeout should be cancelled (e.g. a stylus is reconnected within the 10min window)
Comment 15 Tim Horton 2020-10-12 12:50:18 PDT
Comment on attachment 411132 [details]
Patch

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

> Source/WebKit/Shared/WebProcessCreationParameters.cpp:91
> +    encoder << hasStylusDevice;

Probably can drop the word "device" everywhere :)

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:43
>  #import "WKMouseDeviceObserver.h"

But you've got it for mouse too so maybe not

> Source/WebKit/UIProcess/ios/WKMouseDeviceObserver.mm:50
> +    while (_startCount)

Looping in dealloc is /very weird/. Just shut it down!

> Source/WebKit/UIProcess/ios/WKStylusDeviceObserver.mm:36
> +static Seconds s_changeTimeInterval { 10_min };

I think the s_ rule from the style guide (which you might be following here) is about static data members; this should just be 'changeTimeInterval'

> Source/WebKit/UIProcess/ios/WKStylusDeviceObserver.mm:69
> +    while (_startCount)

Ditto from the other place

> Source/WebKit/UIProcess/ios/WKStylusDeviceObserver.mm:77
> +- (void)_setHasStylusDevice:(BOOL)hasStylusDevice

Don't need leading underscores for methods on internal classes, only exported ones
Comment 16 Wenson Hsieh 2020-10-12 13:19:45 PDT
Comment on attachment 411132 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKMouseDeviceObserver.mm:50
>> +    while (_startCount)
> 
> Looping in dealloc is /very weird/. Just shut it down!

Also, do we ever expect this -dealloc method to be hit?
Comment 17 Devin Rousso 2020-10-12 14:06:41 PDT
Comment on attachment 411132 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/WKMouseDeviceObserver.mm:50
>>> +    while (_startCount)
>> 
>> Looping in dealloc is /very weird/. Just shut it down!
> 
> Also, do we ever expect this -dealloc method to be hit?

I was trying to avoid duplicating the unregister logic, but yeah this is maybe a bit too odd.  I'll just copy it :)

No, this is not expected to be called (at least so long as the UIProcess exists).
Comment 18 Devin Rousso 2020-10-12 14:14:03 PDT
Created attachment 411154 [details]
Patch
Comment 19 Wenson Hsieh 2020-10-12 16:39:39 PDT
Comment on attachment 411154 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKStylusDeviceObserver.mm:67
> +- (void)dealloc

I still think we should just remove these `-dealloc`s, since it's dead code.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/iOSStylusSupport.mm:173
> +    WKStylusDeviceObserver *stylusDeviceObserver = [NSClassFromString(@"WKStylusDeviceObserver") sharedInstance];
> +    [stylusDeviceObserver start];
> +    stylusDeviceObserver.hasStylusDevice = YES;
> +
> +    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
> +
> +    [webView synchronouslyLoadHTMLString:@""];
> +
> +    [stylusDeviceObserver startChangeTimer:0.01];

Nit - it might be a bit cleaner if you pulled this test setup logic out into a separate helper function.
Comment 20 Devin Rousso 2020-10-12 17:31:15 PDT
Comment on attachment 411154 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKStylusDeviceObserver.mm:67
>> +- (void)dealloc
> 
> I still think we should just remove these `-dealloc`s, since it's dead code.

oh yeah that's fine I can remove this

i'll add this too
```
    + (instancetype)new NS_UNAVAILABLE;
    - (instancetype)init NS_UNAVAILABLE;
```
to be extra clear :)

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/iOSStylusSupport.mm:173
>> +    [stylusDeviceObserver startChangeTimer:0.01];
> 
> Nit - it might be a bit cleaner if you pulled this test setup logic out into a separate helper function.

good idea
Comment 21 Devin Rousso 2020-10-12 18:07:32 PDT
Created attachment 411184 [details]
Patch
Comment 22 EWS 2020-10-12 19:15:04 PDT
Committed r268384: <https://trac.webkit.org/changeset/268384>

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