RESOLVED FIXED 207789
Ensure that contenteditable carets on macCatalyst are the right color, especially in Dark Mode
https://bugs.webkit.org/show_bug.cgi?id=207789
Summary Ensure that contenteditable carets on macCatalyst are the right color, especi...
Megan Gardner
Reported 2020-02-14 13:54:33 PST
Fix contenteditable carets on MacCatalyst
Attachments
Patch (8.80 KB, patch)
2020-02-14 14:16 PST, Megan Gardner
no flags
Patch (9.21 KB, patch)
2020-02-14 16:16 PST, Megan Gardner
no flags
Patch (9.10 KB, patch)
2020-02-14 16:29 PST, Megan Gardner
no flags
Patch (9.30 KB, patch)
2020-02-17 15:31 PST, Megan Gardner
no flags
Patch (9.31 KB, patch)
2020-02-18 13:58 PST, Megan Gardner
no flags
Patch (9.42 KB, patch)
2020-02-18 14:34 PST, Megan Gardner
thorton: review+
Patch (9.37 KB, patch)
2020-02-18 15:26 PST, Megan Gardner
no flags
Megan Gardner
Comment 1 2020-02-14 14:16:51 PST
Tim Horton
Comment 2 2020-02-14 14:24:13 PST
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 3 2020-02-14 14:26:42 PST
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
Comment 4 2020-02-14 16:16:56 PST
Tim Horton
Comment 5 2020-02-14 16:20:30 PST
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
Comment 6 2020-02-14 16:29:21 PST
Tim Horton
Comment 7 2020-02-14 16:59:16 PST
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 8 2020-02-15 12:01:01 PST
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
Comment 9 2020-02-17 15:31:33 PST
Tim Horton
Comment 10 2020-02-17 15:44:10 PST
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
Comment 11 2020-02-18 13:58:42 PST
Tim Horton
Comment 12 2020-02-18 14:15:17 PST
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
Comment 13 2020-02-18 14:34:47 PST
Tim Horton
Comment 14 2020-02-18 15:19:21 PST
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
Comment 15 2020-02-18 15:26:24 PST
WebKit Commit Bot
Comment 16 2020-02-19 08:57:20 PST
Comment on attachment 391105 [details] Patch Clearing flags on attachment: 391105 Committed r256921: <https://trac.webkit.org/changeset/256921>
WebKit Commit Bot
Comment 17 2020-02-19 08:57:22 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 18 2020-02-19 08:58:16 PST
Note You need to log in before you can comment on or make changes to this bug.