Bug 190119 – [Cocoa] Add SPI to expose typing attributes at the current selection on WKWebView

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

<rdar://problem/44767062>

[Wenson Hsieh]

Created attachment 351524 [details]
Patch

[Alex Christensen]

Comment on attachment 351524 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:124
> +- (void)_webView:(WKWebView *)webView fontAttributesDidChange:(NSDictionary<NSString *, id> *)fontAttributes WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

Why not use a KVO observable property on the WKWebView?  This sounds exactly like what KVO is for.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1264
> +    if ([uiDelegate respondsToSelector:@selector(_webView:fontAttributesDidChange:)]) {

We cache the results of all the respondsToSelector booleans in UIDelegate.h for performance.  Reading 1 bit is much faster than respondsToSelector.

[Wenson Hsieh]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 351524 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351524&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:124
> > +- (void)_webView:(WKWebView *)webView fontAttributesDidChange:(NSDictionary<NSString *, id> *)fontAttributes WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Why not use a KVO observable property on the WKWebView?  This sounds exactly
> like what KVO is for.

Yes, this would be an elegant interface, but I'd need to know whether or not to compute and propagate font attributes when computing editor state, so I'd need to override -addObserver: and -removeObserver: to know whether we need font attributes in the UI process. Given this, I don't think adding a new delegate method is worse than this KVO-able property.

> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1264
> > +    if ([uiDelegate respondsToSelector:@selector(_webView:fontAttributesDidChange:)]) {
> 
> We cache the results of all the respondsToSelector booleans in UIDelegate.h
> for performance.  Reading 1 bit is much faster than respondsToSelector.

Good call! Done.

[Wenson Hsieh]

Created attachment 351547 [details]
Patch

[Tim Horton]

Comment on attachment 351547 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Add support new WebKit2 SPI to notify the UI delegate about font attributes when the editor state changes (e.g.

"add support new"?

> Source/WebKit/Shared/EditorState.cpp:141
> +    encoder << !!fontAttributes;

Don't we have the ability to directly encode optionals now?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1255
> +    // FIXME: We should either rename -_webView:editorStateDidChange: to clarify that it's only intended for use when testing,
> +    // or remove it entirely and use -_webView:fontAttributesDidChange: instead once text alignment is supported in the set of

Are there any clients other than our testing code? If not, maybe just rename it right away?

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:156
> +    m_delegateMethods.webViewFontAttributesDidChange = [delegate respondsToSelector:@selector(_webView:fontAttributesDidChange:)];

I think "webView:didChangeFontAttributes:" is more traditional style, but I see you were copying an existing method.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/FontAttributes.mm:94
> +enum class Nullability : uint8_t { Null, NonNull };

Is this about nullability (can it be null) or nullity (is it null)

[Wenson Hsieh]

Comment on attachment 351547 [details]
Patch

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

>> Source/WebKit/ChangeLog:9
>> +        Add support new WebKit2 SPI to notify the UI delegate about font attributes when the editor state changes (e.g.
> 
> "add support new"?

I accidentally a word :P

>> Source/WebKit/Shared/EditorState.cpp:141
>> +    encoder << !!fontAttributes;
> 
> Don't we have the ability to directly encode optionals now?

Sounds good — I changed it to this on the encoding side:

    encoder << fontAttributes;

...and this on the decoding side:

    std::optional<std::optional<FontAttributes>> optionalFontAttributes;
    decoder >> optionalFontAttributes;
    if (!optionalFontAttributes)
        return false;

    result.fontAttributes = optionalFontAttributes.value();

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1255
>> +    // or remove it entirely and use -_webView:fontAttributesDidChange: instead once text alignment is supported in the set of
> 
> Are there any clients other than our testing code? If not, maybe just rename it right away?

There aren't any clients apart from our testing code (at least, from what I can tell using internal tooling). I'm going to clean this up in a followup patch, when I decide what I want to do with these. One idea I have is refactoring them to actually test user-facing state (e.g. calling -canPerformAction:withSender:, -textStylingAtPosition:inDirection:, etc.), but I could also repurpose those tests to be about font attributes. Or I could simply rename the delegate hook.

>> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:156
>> +    m_delegateMethods.webViewFontAttributesDidChange = [delegate respondsToSelector:@selector(_webView:fontAttributesDidChange:)];
> 
> I think "webView:didChangeFontAttributes:" is more traditional style, but I see you were copying an existing method.

Sounds good — renamed to `_webView:didChangeFontAttributes:`

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/FontAttributes.mm:94
>> +enum class Nullability : uint8_t { Null, NonNull };
> 
> Is this about nullability (can it be null) or nullity (is it null)

It is indeed about Nullity — renamed!

[Wenson Hsieh]

Created attachment 351644 [details]
Patch for EWS

[WebKit Commit Bot]

Comment on attachment 351644 [details]
Patch for EWS

Clearing flags on attachment: 351644

Committed r236865: <https://trac.webkit.org/changeset/236865>