Currently, event modifiers are not available through the API.
Created attachment 349766 [details] Patch
rdar://problem/44437279
I don't think this is the right approach - it would be better to add new client callbacks that include modifiers.
Created attachment 350029 [details] Patch
(In reply to Anders Carlsson from comment #3) > I don't think this is the right approach - it would be better to add new > client callbacks that include modifiers. Thanks for reviewing :) I have updated the patch.
Comment on attachment 350029 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350029&action=review > Source/WebKit/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=189621 <Radar #?> > Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h:116 > +} WKBundlePageOverlayClientV2; It seems like this change requires clients to adopt a new Client API. Since this is a fix for a regression, it seems like this is not the best approach. Could we do something on the WebKit side (only) to fix this?
(In reply to Brent Fulgham from comment #6) > Comment on attachment 350029 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=350029&action=review > > > Source/WebKit/ChangeLog:4 > > + https://bugs.webkit.org/show_bug.cgi?id=189621 > > <Radar #?> > > > Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h:116 > > +} WKBundlePageOverlayClientV2; > > It seems like this change requires clients to adopt a new Client API. Since > this is a fix for a regression, it seems like this is not the best approach. > > Could we do something on the WebKit side (only) to fix this? See https://bugs.webkit.org/show_bug.cgi?id=189621#c3 for why I thought a new client API was necessary.
(In reply to Anders Carlsson from comment #7) > (In reply to Brent Fulgham from comment #6) > > Could we do something on the WebKit side (only) to fix this? > > See https://bugs.webkit.org/show_bug.cgi?id=189621#c3 for why I thought a > new client API was necessary. I don't think that comment helps me understand (except for reiterating that you think a new client API is better). But, if you prefer the API addition version, I'm fine with that. Could you r+ the patch so we could get it into a build soon?
Created attachment 358441 [details] Patch
(In reply to Brent Fulgham from comment #6) > Comment on attachment 350029 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=350029&action=review > > > Source/WebKit/ChangeLog:4 > > + https://bugs.webkit.org/show_bug.cgi?id=189621 > > <Radar #?> > > > Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h:116 > > +} WKBundlePageOverlayClientV2; > > It seems like this change requires clients to adopt a new Client API. Since > this is a fix for a regression, it seems like this is not the best approach. > > Could we do something on the WebKit side (only) to fix this? I have updated the patch to avoid changing any client code by swizzling [NSEvent modifierFlags]. Thanks for reviewing!
Dan, can you look at this change and give us your thoughts?
Comment on attachment 358441 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358441&action=review > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:389 > + // Swizzle [NSEvent modiferFlags], since it always returns 0 when the WindowServer is blocked. Grep the code to ensure we don’t have an existing Swizzler class or helpers. Maybe we only have for TestWebKitAPI? > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:390 > + Method method = class_getClassMethod(objc_getClass("NSEvent"), @selector(modifierFlags)); [NSEvent class]? > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:391 > + method_setImplementation(method, (IMP)currentModifierFlags); Reinterpret_cast<>?
Comment on attachment 358441 [details] Patch Something about the placement of this code seems a bit weird. Not sure what it is weird though. Grep the code for examples.
(In reply to Daniel Bates from comment #12) > Comment on attachment 358441 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=358441&action=review > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:389 > > + // Swizzle [NSEvent modiferFlags], since it always returns 0 when the WindowServer is blocked. > > Grep the code to ensure we don’t have an existing Swizzler class or helpers. > Maybe we only have for TestWebKitAPI? > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:390 > > + Method method = class_getClassMethod(objc_getClass("NSEvent"), @selector(modifierFlags)); > > [NSEvent class]? > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:391 > > + method_setImplementation(method, (IMP)currentModifierFlags); > > Reinterpret_cast<>? Thanks for reviewing, Dan! I will update the patch.
Created attachment 362309 [details] Patch
(In reply to Per Arne Vollan from comment #15) > Created attachment 362309 [details] > Patch I moved the swizzling to InjectedBundle::initialize, since [NSEvent currentModifiers] is only used by injected bundles.
Comment on attachment 362309 [details] Patch Clearing flags on attachment: 362309 Committed r241737: <https://trac.webkit.org/changeset/241737>