Bug 209292

Summary: CSS hover and "pointer: fine" media queries do not evaluate to true with iOS 13.4 mouse support
Product: WebKit Reporter: Devon Govett <govett>
Component: CSSAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, bdakin, cdumez, eric.carlson, esprehn+autocc, ews-watchlist, glenn, graouts, gyuyoung.kim, hi, japhet, jer.noble, koivisto, macpherson, megan_gardner, menard, mjs, philipj, redux, ryuan.choi, sam, sergio, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh, wilander
Priority: P2 Keywords: InRadar, WebExposed
Version: Safari 13   
Hardware: iPhone / iPad   
OS: iOS 13   
See Also: https://bugs.webkit.org/show_bug.cgi?id=212580
https://bugs.webkit.org/show_bug.cgi?id=217413
https://bugs.webkit.org/show_bug.cgi?id=225672
Bug Depends on: 224529    
Bug Blocks:    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
thorton: review+
Patch none

Description Devon Govett 2020-03-19 10:17:45 PDT
In the iOS 13.4 beta, which supports external mouse/trackpad support, several media queries are not working properly. See linked examples on MDN for reproduction.

* `@media (hover: hover)` - https://developer.mozilla.org/en-US/docs/Web/CSS/@media/hover
* `@media (any-hover: hover)` - https://developer.mozilla.org/en-US/docs/Web/CSS/@media/any-hover
* `@media (pointer: fine)` - https://developer.mozilla.org/en-US/docs/Web/CSS/@media/any-pointer
* `@media (any-pointer: fine)` - https://developer.mozilla.org/en-US/docs/Web/CSS/@media/any-pointer

I would expect that when a mouse is connected, the `hover` media query would activate while the user is using the mouse as their primary input mechanism. This would enable hover effects to be disabled while using touch, but enabled when using a mouse pointer. I believe this should change dynamically when the user switches interaction modes. The `any-hover` media query should activate while a mouse is connected at all, even when touch is the primary input mechanism.

The `pointer: fine` media query should activate while the user is using the mouse as their primary input mechanism. The `any-pointer: fine` media query should activate when a mouse is connected. This would enable the sizes of UI elements to adjust depending on whether they are being interacted with by touch or mouse input. It's unclear to me whether this should change dynamically while the page is loaded, e.g. if the user switches from mouse to touch, as that might cause strange UI shifts. Perhaps `pointer: fine` should never match, and `any-pointer: fine` should when a mouse is connected?
Comment 1 Radar WebKit Bug Importer 2020-03-19 17:59:52 PDT
<rdar://problem/60663124>
Comment 2 Simon Fraser (smfr) 2020-03-19 20:00:37 PDT
Did we #ifdef out all the hover stuff for iOS?
Comment 3 Tim Horton 2020-03-19 21:07:59 PDT
Hover stuff in general works (the pseudoselector, mouseover/out, etc.), but the media queries don't.
Comment 4 Tim Horton 2020-03-19 21:08:39 PDT
They do appear to parse, we just return the wrong value.
Comment 5 Maciej Stachowiak 2020-03-20 01:11:51 PDT
Seems clear that `(any-hover: hover)` and `(any-pointer: fine)` should be true when a pointing device is attached. The non-any cases are less obvious (at least to me). 

On balance, I think it would be better if thoughtful sites kept their layout touch-suitable even when a pointing device is attached, which suggests those should be always false.

Media queries are in general supposed to be re-evaluated when their conditions change, so this model would suggest the `any-` versions should change when a pointing device is attached or detached.
Comment 6 Devon Govett 2020-03-20 10:00:53 PDT
> On balance, I think it would be better if thoughtful sites kept their layout touch-suitable even when a pointing device is attached, which suggests those should be always false.

I would agree for (pointer: fine) (it should always be false), but for (hover: hover) it would be nice if it changed dynamically depending on what the user is using. For example, if wrapped in a hover media query, you could apply hover effects only when the user is hovering over an element with a mouse and not when using touch.

The :hover pseudo selector is sticky with touch: tap once to get the hover effect, tap again to remove it. In cases where the hover effect isn't important for touch, we can use the hover media query to enable it only for mouse input. For that reason, I think it's important that the media query match while using a mouse, and not match while using touch.
Comment 7 Maciej Stachowiak 2020-03-20 10:47:31 PDT
None of these media queries can tell you if input was actually performed with a mouse, rather than with touch. On an iPad, the best they can do is tell you that a mouse or trackpad was attached, and thus potentially could have been the source of input.

For truly optional hover effects that are desired for pointer input but not mouse, the only method I know of to do them conditionally is to build them using pointer events. You can use the `pointerenter`/`pointerleave` pair, or the `pointerover`/`pointerout` pair, and then look at the `pointerType` property. https://www.w3.org/TR/pointerevents2/#dom-pointerevent-pointertype

It would be nice if there was a way to use pure CSS to get hover that only reacts to mouse events, and not to touch event hover simulation, but I don't think there is a way.
Comment 8 Tim Horton 2020-04-07 13:08:01 PDT
*** Bug 210024 has been marked as a duplicate of this bug. ***
Comment 9 Patrick H. Lauke 2020-04-07 13:18:53 PDT
(In reply to Maciej Stachowiak from comment #5)
> Seems clear that `(any-hover: hover)` and `(any-pointer: fine)` should be
> true when a pointing device is attached. The non-any cases are less obvious
> (at least to me). 

to crowbar this in from my recent duplicate:

pointer/hover relate to whatever the device/browser considers to be the "primary" input mechanism. So it comes down to deciding if iPad should treat the mouse as primary or not (i.e. if, even with mouse/trackpad present, it should consider the touchscreen its primary)

> On balance, I think it would be better if thoughtful sites kept their layout
> touch-suitable even when a pointing device is attached, which suggests those
> should be always false.


ideally, authors should not just take their cue about whether or not to make their content touch-suitable from `pointer:coarse` evaluating to true / `pointer:fine` evaluating to false, but from whether or not `any-pointer:coarse` is true - indicating that there is at least one coarse/touch input, be it "primary" or not. (I make kind of the reverse argument in some of the notes in https://www.w3.org/TR/mediaqueries-4/#any-input but the principle is the same).
Comment 10 Patrick H. Lauke 2020-05-31 06:07:25 PDT
Cross-referencing a similar/milder situation with any-pointer:fine and Pencil https://bugs.webkit.org/show_bug.cgi?id=212580
Comment 11 Devin Rousso 2020-09-30 16:19:51 PDT
Created attachment 410167 [details]
Patch
Comment 12 Devin Rousso 2020-09-30 17:14:49 PDT
Created attachment 410176 [details]
Patch

fix copypasta oops
Comment 13 Devin Rousso 2020-10-01 11:47:09 PDT
Created attachment 410251 [details]
Patch
Comment 14 Devin Rousso 2020-10-01 12:03:04 PDT
Created attachment 410255 [details]
Patch
Comment 15 Devin Rousso 2020-10-01 15:11:58 PDT
Created attachment 410274 [details]
Patch
Comment 16 Tim Horton 2020-10-02 14:22:12 PDT
Comment on attachment 410274 [details]
Patch

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

> Source/WebCore/css/MediaQueryEvaluator.cpp:719
> +#if !(HAVE(UIKIT_WITH_MOUSE_SUPPORT) && PLATFORM(MACCATALYST))

I have a crazy thought to make this whole thing less chaos: get the catalyst-always-has-a-mouse behavior out of here, replace all the "HAVE(UIKIT_WITH_MOUSE_SUPPORT) && PLATFORM(IOS)" ifdefs elsewhere with just "HAVE(UIKIT_WITH_MOUSE_SUPPORT)", and make it WebKit2's job to say (via hasMouseDevice) that catalyst has a mouse.

The way you have it now, I think, changes UIWebView behavior in macCatalyst, which is not right. Also would be easier to read, and the less #ifdefs the better.
Comment 17 Devin Rousso 2020-10-02 15:01:35 PDT
Comment on attachment 410274 [details]
Patch

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

>> Source/WebCore/css/MediaQueryEvaluator.cpp:719
>> +#if !(HAVE(UIKIT_WITH_MOUSE_SUPPORT) && PLATFORM(MACCATALYST))
> 
> I have a crazy thought to make this whole thing less chaos: get the catalyst-always-has-a-mouse behavior out of here, replace all the "HAVE(UIKIT_WITH_MOUSE_SUPPORT) && PLATFORM(IOS)" ifdefs elsewhere with just "HAVE(UIKIT_WITH_MOUSE_SUPPORT)", and make it WebKit2's job to say (via hasMouseDevice) that catalyst has a mouse.
> 
> The way you have it now, I think, changes UIWebView behavior in macCatalyst, which is not right. Also would be easier to read, and the less #ifdefs the better.

As discussed offline, my goal with all these "shenanigans" was to try to avoid unnecessary messages and member variables where possible.  We decided to keep the existing `HAVE(UIKIT_WITH_MOUSE_SUPPORT) && PLATFORM(IOS)` in the UIProcess and just change the WebProcess to have a `WebProcess::hasMouseDevice() const { return false; }` variant when `PLATFORM(MACCATALYST)`.
Comment 18 Devin Rousso 2020-10-02 15:03:56 PDT
Created attachment 410375 [details]
Patch
Comment 19 Sam Weinig 2020-10-03 10:02:50 PDT
Comment on attachment 410375 [details]
Patch

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

> Source/WebCore/css/MediaQueryEvaluator.cpp:723
> +#if HAVE(UIKIT_WITH_MOUSE_SUPPORT)
> +    auto* page = frame.page();
> +    if (page && page->chrome().client().hasMouseDevice())
> +        return true;
> +#endif // HAVE(UIKIT_WITH_MOUSE_SUPPORT)

I think the HAVE(UIKIT_WITH_MOUSE_SUPPORT) check would make more sense in the implementation of hasMouseDevice(). The more we reduce platform #ifdefs in core code like this the better.

> Source/WebCore/page/ChromeClient.h:183
> +#if HAVE(UIKIT_WITH_MOUSE_SUPPORT)
> +    virtual bool hasMouseDevice() = 0;
> +#endif

It seems like all platforms could implement this, so I would prefer if it were not within HAVE(UIKIT_WITH_MOUSE_SUPPORT).
Comment 20 Devin Rousso 2020-10-05 11:26:10 PDT
Comment on attachment 410375 [details]
Patch

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

>> Source/WebCore/css/MediaQueryEvaluator.cpp:723
>> +#endif // HAVE(UIKIT_WITH_MOUSE_SUPPORT)
> 
> I think the HAVE(UIKIT_WITH_MOUSE_SUPPORT) check would make more sense in the implementation of hasMouseDevice(). The more we reduce platform #ifdefs in core code like this the better.

I generally agree that less `#if` is better, but in this case I think the context of `HAVE(UIKIT_WITH_MOUSE_SUPPORT)` is actually somewhat desirable, as right now `hasMouseDevice` will only be `true` when a trackpad/mouse is connected to an iPad (via BackBoardServices).  Yes we could have `hasMouseDevice` be `true` in other cases too, but I think having extra logic for non-iOS is unnecessary as other ports all support (and expect) mouse devices to be connected.  Previously this was only determined by `screenHasTouchDevice()` (if supported) and otherwise assumed to have a mouse.
Comment 21 Sam Weinig 2020-10-05 13:11:04 PDT
(In reply to Devin Rousso from comment #20)
> Comment on attachment 410375 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410375&action=review
> 
> >> Source/WebCore/css/MediaQueryEvaluator.cpp:723
> >> +#endif // HAVE(UIKIT_WITH_MOUSE_SUPPORT)
> > 
> > I think the HAVE(UIKIT_WITH_MOUSE_SUPPORT) check would make more sense in the implementation of hasMouseDevice(). The more we reduce platform #ifdefs in core code like this the better.
> 
> I generally agree that less `#if` is better, but in this case I think the
> context of `HAVE(UIKIT_WITH_MOUSE_SUPPORT)` is actually somewhat desirable,
> as right now `hasMouseDevice` will only be `true` when a trackpad/mouse is
> connected to an iPad (via BackBoardServices).  Yes we could have
> `hasMouseDevice` be `true` in other cases too, but I think having extra
> logic for non-iOS is unnecessary as other ports all support (and expect)
> mouse devices to be connected.  Previously this was only determined by
> `screenHasTouchDevice()` (if supported) and otherwise assumed to have a
> mouse.

WebCore should really not care or know about the platform as much as possible. Adding more specific logic about UIKit here is the wrong direction to go. Please move the #ifdefs into the implementation of the ChromeClient.
Comment 22 Devin Rousso 2020-10-05 15:33:17 PDT
Comment on attachment 410375 [details]
Patch

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

>>>> Source/WebCore/css/MediaQueryEvaluator.cpp:723
>>>> +#endif // HAVE(UIKIT_WITH_MOUSE_SUPPORT)
>>> 
>>> I think the HAVE(UIKIT_WITH_MOUSE_SUPPORT) check would make more sense in the implementation of hasMouseDevice(). The more we reduce platform #ifdefs in core code like this the better.
>> 
>> I generally agree that less `#if` is better, but in this case I think the context of `HAVE(UIKIT_WITH_MOUSE_SUPPORT)` is actually somewhat desirable, as right now `hasMouseDevice` will only be `true` when a trackpad/mouse is connected to an iPad (via BackBoardServices).  Yes we could have `hasMouseDevice` be `true` in other cases too, but I think having extra logic for non-iOS is unnecessary as other ports all support (and expect) mouse devices to be connected.  Previously this was only determined by `screenHasTouchDevice()` (if supported) and otherwise assumed to have a mouse.
> 
> WebCore should really not care or know about the platform as much as possible. Adding more specific logic about UIKit here is the wrong direction to go. Please move the #ifdefs into the implementation of the ChromeClient.

I realize that I should've provided more info in my last comment.  I apologize for not doing so.  To be clear, I am not "set in stone" or anything like that about this, but I have a hard time seeing a good way forward for moving this logic to WebKit that doesn't _add_ complexity/confusion on top of what's already there.  Based on what I've written below, if you have a good idea for how to do so I am very interested and more than happy to make a change.

This code really only exists for the purposes of one specific feature: dispatching mouse events on an iPad with a connected trackpad/mouse.  One of the concerns I had was that for WK1 catalyst, we are intentionally "lying" about supporting mouse devices (macOS assumes that there is always a mouse, which is perhaps a bad assumption, but is such nonetheless) because WK1 does not have support for this feature.  As such, having something general like `hasMouseDevice` seems like a bad idea (when not guarded by this `#if`) because someone in the future may use `hasMouseDevice` thinking that it would give them the right value in all situations.  This can be assuaged by having a better name (e.g. `hasMouseDeviceForDispatchingMouseEvents`), but the issue still remains.  I personally felt that wrapping the method inside an `#if` alleviated most of this concern as it made it explicitly clear that this method existed for this particular feature/support not something general/global (although to be fair one could argue that even then the concern still exists).

There's also the question of "how does this interplay with `screenHasTouchDevice`?" (previously, `screenHasTouchDevice` was the way that mouse/touch support was determined for these media queries (e.g. "if touch events are enabled and a touch device exists, then there is no mouse")) but this is only really needed by GTK/WPE so I'd likely just need to ask which `ChromeClient` to move that call into in order to make it work.  If the rest of the logic moves, however, then I think this must also move as well, because if a non-iOS `ChromeClient` say `true` then this function would return before we've had a chance to evaluate `screenHasTouchDevice`.

Putting this on the `ChromeClient` may be slightly problematic though as I don't think it's a good idea to assume that `frame.page()` is valid, meaning that we'd need some way of determining the fallback value.  Right now, `screenHasTouchDevice` takes care of that , but I'd preferably not want to have to put that in more places than it already is now.  This is perhaps more of an academic concern, though, and we could just say `false` if there is no `Page`.

FWIW I think that the naming of `UIKIT_WITH_MOUSE_SUPPORT` is not entirely ideal and perhaps could've been named differently so as to not be as platform specific.

As I mentioned, if you feel strongly about not having platform code inside WebCore then I can go figure out better names and get the answers to these questions, but I personally feel like this is doing a lot more work and headache than is really necessary for such a small and specifically "targeted" feature.
Comment 23 Sam Weinig 2020-10-05 16:17:16 PDT
(In reply to Devin Rousso from comment #22)

The things you are describing here are examples of things that should be handled by the ChromeClient. The lack of page at times is endemic to WebCore, but we must utilize the chromeClient non-the-less. I also think the current behavior where ports assume there is always a mouse is not necessarily the right long term behavior, it's just the current behavior.

Let's take a step back and think about what we might want this to look like in an idealized model. There are four media queries of interest in this general area:

"hover" - does the *primary* pointing device support hover?
"any-hover" - does *any* attached pointing device support hover?
"pointer" - what are the pointer characteristics of the *primary* pointing device?
"any-pointer" - what pointer characteristics are supported by *any* pointing device? (in this one, we may want to match on more than one value). 

To answer these questions I could ask the client a few different sets of questions. The most basic would be one for each:

  bool ChromeClient::hoverSupportedByPrimaryPointingDevice() const;
  bool ChromeClient::hoverSupportedByAnyAttachedPointingDevice() const

  enum PointerCharacteristics { None, Coarse, Fine };
  
  PointerCharacteristics ChromeClient::pointerCharacteristicsOfPrimaryPointingDevice() const
  OptionSet<PointerCharacteristics> ChromeClient::pointerCharacteristicsOfAllAttachedPointingDevices() const

(syntax and whatnot, not necessarily correct, just there for illustrative purposes).

This is a bit verbose, but I think ok. You could probably reduce it to two, and have a struct for describing the "primary pointing device" and one for "all attached pointing devices".


So, let's address some of your concerns.

What about ports that want to always return true even if there is no mouse attached?

I'm not sure if we should, but if you want this behavior, the way I would recommend going about it would be to add something to Settings to "forceMediaQueryForPointerAlwaysEvaluatesTrueEnabled" (or something less verbose), and then allow that platform to enable that setting by default. Doing this as a setting allows us to test the behavior on all platforms, which is helpful since many of us don't run the tests in all configurations all the time.

What about when there is no Page?

We already bail on MediaQuery evaluation if there is no Document, no Frame or no FrameView. It seems reasonable to also bail if there is no Page. 


So, in summary, from looking more closely at the code, I think more should move in ChromeClient, not less.
Comment 24 Devin Rousso 2020-10-05 17:30:21 PDT
(In reply to Sam Weinig from comment #23)
Ah, I think I understand more clearly what you are describing.  That sounds much more reasonable than the image I had in my mind.

FWIW I phrased some of the things I said in my last response somewhat badly (which is usually why I prefer to speak in person), in that I meant them more as questions rather than statements (e.g. instead of "I don't think it's a good idea to assume that `frame.page()` is valid, meaning that we'd need some way of determining the fallback value" I really meant something more like "can I assume that `frame.page()` is valid, and if not what should the fallback be?").  Apologies for that.

I'll work on reorganizing this :)
Comment 25 Devin Rousso 2020-10-05 21:13:34 PDT
Created attachment 410607 [details]
Patch
Comment 26 Patrick H. Lauke 2020-10-06 01:17:33 PDT
(In reply to Sam Weinig from comment #23)
[...]
> "any-pointer" - what pointer characteristics are supported by *any* pointing
> device? (in this one, we may want to match on more than one value). 

Just about the parenthetical, per spec yes you would match more than one when there are 2 or more devices, and they have different pointer precision/characteristics. For touch AND mouse scenarios, both any-pointer:coarse AND any-pointer:fine should evaluate to true.

also, as an aside, work here *may* cross/synergise with https://bugs.webkit.org/show_bug.cgi?id=212580 (about possibility of detecting/exposing Pencil)
Comment 27 Devin Rousso 2020-10-06 10:30:49 PDT
Created attachment 410655 [details]
Patch
Comment 28 Devin Rousso 2020-10-06 10:44:46 PDT
Created attachment 410658 [details]
Patch
Comment 29 Tim Horton 2020-10-06 10:47:11 PDT
Comment on attachment 410655 [details]
Patch

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

> Source/WebCore/css/MediaQueryEvaluator.cpp:-733
> -static bool anyHoverEvaluate(CSSValue* value, const CSSToLengthConversionData&, Frame&, MediaFeaturePrefix)

Noiceee

> Source/WebKit/UIProcess/WebProcessProxy.cpp:216
> +#if !PLATFORM(IOS)

To save someone some heartache in the future (likely, someone adding something to platformInitialize in WebProcessProxyIOS and then finding out the hard way it isn't run for other iOS-y platforms, unlike most of the code in that file), I would make these IOS_FAMILY and make the WebProcessProxyIOS implementation unconditional (and put the PLATFORM(IOS) check inside the functions with the UIKIT_WITH_MOUSE_SUPPORT check). Up to you.

> Source/WebKit/UIProcess/ios/WKMouseDeviceObserver.mm:59
> +    _token = [[BKSMousePointerService sharedInstance] addPointerDeviceObserver:self];

Little bit confused about the lifetime of _token. The method /sounds/ like it would return an autoreleased object (no `create` or `new`), but you're storing it as a bare pointer. What's keeping it alive?

> Source/WebKit/UIProcess/ios/WKMouseDeviceObserver.mm:75
> +    BOOL hasMouseDevice = [mousePointerDevices count] > 0;

We prefer dots for properties where possible.

> Source/WebKit/WebProcess/WebProcess.cpp:410
> +    m_hasMouseDevice = parameters.hasMouseDevice;

Possible this could shuffle off into platformInitializeWebProcess?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/iOSMouseSupport.mm:305
> +static BOOL evaluateMediaQuery(RetainPtr<TestWKWebView>& webView, NSString *query)

This almost might be useful as a TestWKWebView method! I can /totally/ imagine other tests using it.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/iOSMouseSupport.mm:354
> +    [mouseDeviceObserver start];

Isn't creating the web view going to start it again (because of the way you implemented the observer above, always starting when the first process comes alive)? Maybe it's not a problem, though?
Comment 30 Devin Rousso 2020-10-06 11:56:13 PDT
Comment on attachment 410655 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessProxy.cpp:216
>> +#if !PLATFORM(IOS)
> 
> To save someone some heartache in the future (likely, someone adding something to platformInitialize in WebProcessProxyIOS and then finding out the hard way it isn't run for other iOS-y platforms, unlike most of the code in that file), I would make these IOS_FAMILY and make the WebProcessProxyIOS implementation unconditional (and put the PLATFORM(IOS) check inside the functions with the UIKIT_WITH_MOUSE_SUPPORT check). Up to you.

good call

>> Source/WebKit/WebProcess/WebProcess.cpp:410
>> +    m_hasMouseDevice = parameters.hasMouseDevice;
> 
> Possible this could shuffle off into platformInitializeWebProcess?

did not know that existed :P

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/iOSMouseSupport.mm:305
>> +static BOOL evaluateMediaQuery(RetainPtr<TestWKWebView>& webView, NSString *query)
> 
> This almost might be useful as a TestWKWebView method! I can /totally/ imagine other tests using it.

ooo good idea!

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/iOSMouseSupport.mm:354
>> +    [mouseDeviceObserver start];
> 
> Isn't creating the web view going to start it again (because of the way you implemented the observer above, always starting when the first process comes alive)? Maybe it's not a problem, though?

yes `start` will get called again, but because we already have a `_token` we shouldn't re-add the observer, so it should be fine
Comment 31 Devin Rousso 2020-10-06 12:02:07 PDT
Created attachment 410675 [details]
Patch
Comment 32 Tim Horton 2020-10-06 12:08:07 PDT
Comment on attachment 410675 [details]
Patch

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

> Source/WebCore/page/PointerCharacteristics.h:45
> +template<> struct EnumTraits<WebCore::PointerCharacteristics> {
> +    using values = EnumValues<
> +        WebCore::PointerCharacteristics,
> +        WebCore::PointerCharacteristics::Coarse,
> +        WebCore::PointerCharacteristics::Fine
> +    >;
> +};

I feel like this isn't going to work right. The enum is actually flags so you can set multiple bits...  these traits specify the valid values for decoding, but if you had both bits set, it isn't one of these valid values. Am I crazy?
Comment 33 Devin Rousso 2020-10-06 12:23:38 PDT
Comment on attachment 410675 [details]
Patch

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

>> Source/WebCore/page/PointerCharacteristics.h:45
>> +};
> 
> I feel like this isn't going to work right. The enum is actually flags so you can set multiple bits...  these traits specify the valid values for decoding, but if you had both bits set, it isn't one of these valid values. Am I crazy?

`Coarse` and `Fine` really are mutually exclusive (I don't know of a pointer device that is both), so they shouldn't really be used in conjunction (unless inside an `OptionSet`).  The only reason I even added a specific value for each item was because `any-pointer` is supposed to represent the union of all pointer characteristics, and an `OptionSet` seemed the most simple option for that.

Also, I don't think I actually need `WTF::EnumTraits<WebCore::PointerCharacteristics>` for this patch, so I could just remove it.  I was following a pattern I saw from other things (e.g. `IPC::MessageFlags`, `WebCore::AutoplayEventFlags`, `WebCore::DragApplicationFlags`, etc.).

How would you suggest I change this?
Comment 34 Tim Horton 2020-10-06 12:29:20 PDT
Comment on attachment 410675 [details]
Patch

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

>>> Source/WebCore/page/PointerCharacteristics.h:45
>>> +};
>> 
>> I feel like this isn't going to work right. The enum is actually flags so you can set multiple bits...  these traits specify the valid values for decoding, but if you had both bits set, it isn't one of these valid values. Am I crazy?
> 
> `Coarse` and `Fine` really are mutually exclusive (I don't know of a pointer device that is both), so they shouldn't really be used in conjunction (unless inside an `OptionSet`).  The only reason I even added a specific value for each item was because `any-pointer` is supposed to represent the union of all pointer characteristics, and an `OptionSet` seemed the most simple option for that.
> 
> Also, I don't think I actually need `WTF::EnumTraits<WebCore::PointerCharacteristics>` for this patch, so I could just remove it.  I was following a pattern I saw from other things (e.g. `IPC::MessageFlags`, `WebCore::AutoplayEventFlags`, `WebCore::DragApplicationFlags`, etc.).
> 
> How would you suggest I change this?

Oh, you're right, you don't need it, because you never encode it (I missed that). Delete it! No reason to add it prematurely.
Comment 35 Devin Rousso 2020-10-06 12:48:01 PDT
Created attachment 410681 [details]
Patch
Comment 36 Tim Horton 2020-10-06 14:27:59 PDT
Comment on attachment 410681 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKMouseDeviceObserver.mm:39
> ++ (WKMouseDeviceObserver *)sharedInstance {

This brace should be on the next line :)
Comment 37 Devin Rousso 2020-10-06 14:37:48 PDT
Created attachment 410694 [details]
Patch
Comment 38 EWS 2020-10-06 16:44:52 PDT
Committed r268086: <https://trac.webkit.org/changeset/268086>

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