WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(9.21 KB, patch)
2020-02-14 16:16 PST
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(9.10 KB, patch)
2020-02-14 16:29 PST
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(9.30 KB, patch)
2020-02-17 15:31 PST
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(9.31 KB, patch)
2020-02-18 13:58 PST
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(9.42 KB, patch)
2020-02-18 14:34 PST
,
Megan Gardner
thorton
: review+
Details
Formatted Diff
Diff
Patch
(9.37 KB, patch)
2020-02-18 15:26 PST
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Megan Gardner
Comment 1
2020-02-14 14:16:51 PST
Created
attachment 390816
[details]
Patch
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
Created
attachment 390832
[details]
Patch
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
Created
attachment 390833
[details]
Patch
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
Created
attachment 390994
[details]
Patch
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
Created
attachment 391086
[details]
Patch
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
Created
attachment 391093
[details]
Patch
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
Created
attachment 391105
[details]
Patch
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
<
rdar://problem/59592008
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug