Bug 177489 - [iOS] Respect the "caret-color" CSS property when editing
Summary: [iOS] Respect the "caret-color" CSS property when editing
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar, WebExposed
Depends on: 166572
Blocks:
  Show dependency treegraph
 
Reported: 2017-09-26 10:22 PDT by Aishwarya Nirmal
Modified: 2017-10-06 17:38 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.30 KB, patch)
2017-10-04 17:06 PDT, Aishwarya Nirmal
no flags Details | Formatted Diff | Diff
Patch (7.74 KB, patch)
2017-10-05 12:01 PDT, Aishwarya Nirmal
thorton: review+
Details | Formatted Diff | Diff
Patch (7.38 KB, patch)
2017-10-06 13:14 PDT, Aishwarya Nirmal
no flags Details | Formatted Diff | Diff
Patch (7.22 KB, patch)
2017-10-06 14:18 PDT, Aishwarya Nirmal
thorton: commit-queue-
Details | Formatted Diff | Diff
Patch (7.24 KB, patch)
2017-10-06 16:25 PDT, Aishwarya Nirmal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aishwarya Nirmal 2017-09-26 10:22:59 PDT
<rdar://problem/34600419>
Comment 1 Aishwarya Nirmal 2017-10-04 17:06:52 PDT
Created attachment 322744 [details]
Patch
Comment 2 Andy Estes 2017-10-04 17:17:03 PDT
Comment on attachment 322744 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        This change adds support for the caret-color property on iOS. 

This line should contain the title of the bug ("[iOS] Respect the "caret-color" CSS property when editing").

You can put a description of your change below the "Reviewed by" line.
Comment 3 Wenson Hsieh 2017-10-04 17:24:38 PDT
Comment on attachment 322744 [details]
Patch

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

Looks good overall! Just some minor nits here and there.

> Source/WebKit/ChangeLog:3
> +        This change adds support for the caret-color property on iOS. 

This should be the title of the bug ([iOS] Respect the "caret-color" CSS property when editing)

> Source/WebKit/Shared/EditorState.h:105
> +        WebCore::Color caretColor { WebCore::Color::black };

Hm...black seems like a strange default value to have here. This should probably be an invalid Color, so that we'll use the default [UIColor insertionPointColor]. Perhaps the default Color constructor fulfills this already, so we might not need an explicit initial value here!

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2028
> +    WebCore::Color caretColor = _page->editorState().postLayoutData().caretColor;

I don't think PostLayoutData is guaranteed here -- we should probably add an early return with the default insertionPointColor if the EditorState is missing post layout data (there's a flag on EditorState for this).

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2030
> +        return [[UIColor alloc] initWithCGColor:cachedCGColor(caretColor)];

We should return an autoreleased object UIColor here to avoid leaking.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:234
> +            Color caretColor = m_assistedNode->renderer()->style().caretColor();

Nit - I don't think the local variable for caretColor adds much -- `postLayoutData.caretColor = m_assistedNode->renderer()->style().caretColor();` would be a bit cleaner.

> Tools/ChangeLog:3
> +        Adds test for iOS caret color support.

Title of the bug :)

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm:311
> +    CGFloat ipcRed = 0, ipcGreen = 0, ipcBlue = 0, ipcAlpha = 0; // insertion point color components

We generally use non-abbreviated variable names in WebKit (so this would be insertionPointColorRed, etc.).. There's more info about it here: https://webkit.org/code-style-guidelines
Comment 4 Wenson Hsieh 2017-10-04 17:28:35 PDT
Comment on attachment 322744 [details]
Patch

Whoops, my comment's collided with Andy's :P

Restoring Andy's r+, but adding cq- for some of the above comments.
Comment 5 Wenson Hsieh 2017-10-04 19:29:19 PDT
Comment on attachment 322744 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm:304
> +    TestWKWebView *webView = [[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)];

Looks like TestWKWebView leaks here. We can fix this in a few ways -- one is to just call -release at the end of this test, and another is to autorelease here right after initializing webView. But in WebKit, we often use RetainPtr and adoptNS instead to tie -retain/-release calls to the lifetime of C++ objects (RetainPtrs) so we don't have to explicitly call -retain or -release, so this line could look like this:

    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);

...and then RetainPtr automatically takes care of releasing when it gets destroyed. The `auto` keyword here expands to the return type of adoptNS, which in this case is RetainPtr<TestWKWebView>.
Comment 6 Aishwarya Nirmal 2017-10-05 12:01:23 PDT
Created attachment 322878 [details]
Patch
Comment 7 Tim Horton 2017-10-05 12:06:22 PDT
Comment on attachment 322878 [details]
Patch

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

> Source/WebKit/Shared/EditorState.h:105
> +        WebCore::Color caretColor { WebCore::Color::Color() };

You don't need the braces or anything inside, the default constructor will be invoked regardless.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2031
> +            return [[[UIColor alloc] initWithCGColor:cachedCGColor(caretColor)] autorelease];

UIColor has colorWithCGColor if you want to avoid the autorelease.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm:314
> +    [redColor getRed:&redColorRed green:&redColorGreen blue:&redColorBlue alpha:&redColorAlpha];

I wonder, can we not just use UIColor equality instead? Like EXPECT_TRUE([insertionPointColor isEqual:redColor]);?

You're already going to fail if they're the same color in different color spaces, which is the usual gotcha with isEqual, so that's not a reason not to.

Thoughts?
Comment 8 Aishwarya Nirmal 2017-10-06 13:14:45 PDT
Created attachment 323040 [details]
Patch
Comment 9 Aishwarya Nirmal 2017-10-06 14:18:34 PDT
Created attachment 323044 [details]
Patch
Comment 10 Tim Horton 2017-10-06 15:23:50 PDT
Comment on attachment 323044 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm:310
> +    CGColorSpaceRef colorSpace = CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB);

This leaks.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm:311
> +    CGColorRef cgInsertionPointColor = CGColorCreateCopyByMatchingToColorSpace(colorSpace, kCGRenderingIntentDefault, insertionPointColor.CGColor, NULL);

This leaks.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm:312
> +    CGColorRef cgRedColor = CGColorCreateCopyByMatchingToColorSpace(colorSpace, kCGRenderingIntentDefault, redColor.CGColor, NULL);

This leaks.
Comment 11 Aishwarya Nirmal 2017-10-06 16:25:31 PDT
Created attachment 323063 [details]
Patch
Comment 12 WebKit Commit Bot 2017-10-06 17:09:52 PDT
Comment on attachment 323063 [details]
Patch

Clearing flags on attachment: 323063

Committed r223012: <http://trac.webkit.org/changeset/223012>