Bug 207789 – Ensure that contenteditable carets on macCatalyst are the right color, especially in Dark Mode

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Fix contenteditable carets on MacCatalyst

[Megan Gardner]

Created attachment 390816 [details]
Patch

[Tim Horton]

Comment on attachment 390816 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:1802
>      if (element && element->renderer()) {

Remove the extraneous curly braces

> Source/WebCore/editing/FrameSelection.cpp:1803
> +        caretColor = CaretBase::computeCaretColor(element->renderer()->style());

You don't pass a node here? Also, maybe just pass the element and have computeCaretColor find what it wants?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3222
> +    return [self getInteractionTintColor];

This isn't how we use the `get` prefix. I would go with `_interactionTintColor`.

[Tim Horton]

Comment on attachment 390816 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:277
> +#if PLATFORM(MACCATALYST)

Should have a comment here about why iOS is different (on iOS, by default, we want to fall back to the tintColor (default blue), so only want to override if caretColor was /explicitly/ specified by the content, instead of using the smarts to always decide a color that we use on Mac and in macCatalyst).

[Megan Gardner]

Created attachment 390832 [details]
Patch

[Tim Horton]

Comment on attachment 390832 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Fix contenteditable carets on MacCatalyst

What are you fixing about them?

> Source/WebCore/editing/FrameSelection.cpp:1803
> +        caretColor = CaretBase::computeCaretColor(element->renderer()->style());

See my all my earlier comments!!

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3224
> +    return [self.textInputTraits insertionPoiWebPageIOS.mmntColor];

wat

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:278
> +            // On MacCatalyst, we need to calculate the caret color the same way that we do on mac to maintain a consistent look.

Capitalization: macCatalyst, Mac (or macOS, but not "mac")

[Megan Gardner]

Created attachment 390833 [details]
Patch

[Tim Horton]

Comment on attachment 390833 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Fix contenteditable carets on MacCatalyst

What are you fixing about them?? (make the title say the word "color" at least)

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3224
> +#if PLATFORM(MACCATALYST)
> +    return [self _interactionTintColor];
> +#endif
>      return [self.textInputTraits insertionPointColor];

It is very odd that there are two `return`s in the catalyst case. Can you #else?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3237
> +- (UIColor*)_interactionTintColor

Space before the *

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3239
> +    if (!self.webView.configuration._textInteractionGesturesEnabled)

(not a regression from this change, so you don't have to fix it here) -configuration makes a copy, which is not extremely cheap. We should directly read from WKWebView's copy instead!

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3253
> +    UIColor *tintColor = [self _interactionTintColor];
>      [_traits _setColorsToMatchTintColor:tintColor];

Why the local?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:278
> +            // On MacCatalyst, we need to calculate the caret color the same way that we do on mac to maintain a consistent look.

All of my previous comments about capitalization still apply

[Sam Weinig]

Comment on attachment 390833 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:278
>> +            // On MacCatalyst, we need to calculate the caret color the same way that we do on mac to maintain a consistent look.
> 
> All of my previous comments about capitalization still apply

Seems like this #if PLATFORM could be inside CaretBase::computeCaretColor() to keep caret logic all together.

[Megan Gardner]

Created attachment 390994 [details]
Patch

[Tim Horton]

Comment on attachment 390994 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Fix contenteditable carets on MacCatalyst

oO oO oO oO Please put the new title here.

> Source/WebKit/ChangeLog:9
> +        Because UIKit only uses label color for the caret in MacCatlayst, 

macCatalyst

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3221
> +    // On macCatalyst we need to explicitly return the color we have calculated, rather than rely on textTraits, as on macCatalyst, UIKit ignores text traits, and relies on AppKit's calulation, which cannot track dark mode correctly in web content.

calulation

Also, this part isn't about dark mode... they totally ignore ANY insertionPointColor change, even from caretColor. I would just stop after "ignores text traits" and slap a period on there and you're golden.

[Megan Gardner]

Created attachment 391086 [details]
Patch

[Tim Horton]

Comment on attachment 391086 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Fix contenteditable carets on MacCatalyst

This does not match the bugzilla title

[Megan Gardner]

Created attachment 391093 [details]
Patch

[Tim Horton]

Comment on attachment 391093 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Because UIKit only uses label color for the caret in macCatlayst, 

macCatalyst is not spelled correctly

> Source/WebCore/editing/FrameSelection.cpp:1777
> +#endif
> +    auto* rootEditableElement = node ? node->rootEditableElement() : nullptr;

You have two `returns` and lots of extra unreachable code on iOS; you should have an #else instead

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3226
> +    // On macCatalyst we need to explicitly return the color we have calculated, rather than rely on textTraits, as on macCatalyst, UIKit ignores text traits, and relies on AppKit's calulation, which cannot track dark mode correctly in web content.

"calulation" is spelled incorrectly (should be "calculation")

Also, this part isn't about dark mode... they totally ignore ANY insertionPointColor change, even from caretColor. I would just stop after "ignores text traits" and slap a period on there and you're golden.

[Megan Gardner]

Created attachment 391105 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391105 [details]
Patch

Clearing flags on attachment: 391105

Committed r256921: <https://trac.webkit.org/changeset/256921>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59592008>