Bug 207789

Created attachment 390816 [details] Patch

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`.

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).

Created attachment 390832 [details] Patch

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")

Created attachment 390833 [details] Patch

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

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.

Created attachment 390994 [details] Patch

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.

Created attachment 391086 [details] Patch

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

Created attachment 391093 [details] Patch

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.

Created attachment 391105 [details] Patch

Comment on attachment 391105 [details] Patch Clearing flags on attachment: 391105 Committed r256921: <https://trac.webkit.org/changeset/256921>

All reviewed patches have been landed. Closing bug.

<rdar://problem/59592008>