[webkit-reviews] review denied: [Bug 118680] AX: VoiceOver not working with data detection page overlays : [Attachment 207400] patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied chris fleizach <cfleizach at apple.com>'s
request for review:
Bug 118680: AX: VoiceOver not working with data detection page overlays
https://bugs.webkit.org/show_bug.cgi?id=118680

Attachment 207400: patch
https://bugs.webkit.org/attachment.cgi?id=207400&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=207400&action=review


> WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h:51
> +typedef WKTypeRef
(*WKAccessibilityAttributeValueCallback)(WKBundlePageOverlayRef pageOverlay,
WKStringRef attribute, WKTypeRef parameter, const void* clientInfo);

This should go right above the WKBundlePageOverlayAccessibilityClient.

> WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h:65
>  };
> +
>  typedef struct WKBundlePageOverlayClient WKBundlePageOverlayClient;

This space is not necessary..

> WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h:75
>  enum { kWKBundlePageOverlayClientCurrentVersion = 0 };
> +enum { kWKBundlePageOverlayAccessibilityClientCurrentVersion = 0 };

The enum with kWKBundlePageOverlayClientCurrentVersion should go next to the
WKBundlePageOverlayClient.

> WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h:80
>  WK_EXPORT WKBundlePageOverlayRef
WKBundlePageOverlayCreate(WKBundlePageOverlayClient* client);
> +WK_EXPORT WKBundlePageOverlayRef
WKBundlePageOverlayCreateWithAccessibilityClient(WKBundlePageOverlayClient*
client, WKBundlePageOverlayAccessibilityClient* accessibilityClient);

I would prefer if this was a WKBundlePageOverlaySetAccessibilityClient()
function.

> WebProcess/WebPage/mac/WKAccessibilityWebPageObject.mm:49
> +static NSString *NSAccessibilityDataDetectorExistsAtPointKey        =
@"AXDataDetectorExistsAtPoint";
> +static NSString *NSAccessibilityDidShowDataDetectorMenuAtPointKey   =
@"AXDidShowDataDetectorMenuAtPoint";
> +static NSString *NSAccessibilityDataDetectorTypeAtPointKey	       =
@"AXDataDetectorTypeAtPoint";

Please put a FIXME here, about removing knowledge of data detectors in the
future. 

 I am not clear why this is necessary, can we structure this in a way the
client, the only party that knows the overlay is being used for data detectors,
is the one with this knowledge?