Bug 135904 – Text caret changes to color of text in Mail and Notes

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Text caret changes to color of text in Mail and Notes

[Myles C. Maxfield]

Created attachment 236551 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/17892863>

Comment on attachment 236551 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Text caret changes to color of text in Mail and Notes

Since this is a regression and it’s know which change introduced it, it’s worth including this information in the bug title.

> Source/WebCore/ChangeLog:11
> +        No new tests because this is a rollout.

You’re actually removing a test! I don’t think that’s the best thing to do here. Ideally we’d have a test to catch future changes to this behavior that could break Mail and Notes again. If the existing test is similar enough to the situation in those apps, I think we should keep it, perhaps modify the text and colors a little to explain what it’s testing, and update the expected results. If the Mail and Notes case is substantially different, then we should add a regression test that resembles that case.

[Myles C. Maxfield]

Comment on attachment 236551 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Text caret changes to color of text in Mail and Notes
> 
> Since this is a regression and it’s know which change introduced it, it’s worth including this information in the bug title.

Done.

>> Source/WebCore/ChangeLog:11
>> +        No new tests because this is a rollout.
> 
> You’re actually removing a test! I don’t think that’s the best thing to do here. Ideally we’d have a test to catch future changes to this behavior that could break Mail and Notes again. If the existing test is similar enough to the situation in those apps, I think we should keep it, perhaps modify the text and colors a little to explain what it’s testing, and update the expected results. If the Mail and Notes case is substantially different, then we should add a regression test that resembles that case.

Done.

[Myles C. Maxfield]

Created attachment 236572 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 236572 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:1511
> -    Element* element = node->isElementNode() ? toElement(node) : node->parentElement();
> +    Element* element = node->rootEditableElement();

I don't think this is the right fix.

r152612 fixed the problem that the caret becomes invisible if the background color of the current node matches that of the root editable element's text color.

Our current behavior matches that of Firefox.

[Ryosuke Niwa]

The right behavior can't be that the user can't see the caret just for the sake of matching TextEdit.  Adding a new CSS property, etc... wouldn't work either because mail contents, etc... can't those that property as they're authored by other engines and there will always be old mail clients.

We might need to do something like only changing the caret color when the effective background color is different than the root editable element so that the caret stays black in white background.

That'll match the behavior of TextEdit in most cases while still allowing the caret to be visible when the background color is changed.

[Myles C. Maxfield]

The idea behind https://bugs.webkit.org/show_bug.cgi?id=135920 is that Mail + Notes can specify this in their own user stylesheet (it's an inherited property). I think that would solve the problem nicely. Thoughts?

(In reply to comment #7)
> The right behavior can't be that the user can't see the caret just for the sake of matching TextEdit.  Adding a new CSS property, etc... wouldn't work either because mail contents, etc... can't those that property as they're authored by other engines and there will always be old mail clients.
> 
> We might need to do something like only changing the caret color when the effective background color is different than the root editable element so that the caret stays black in white background.
> 
> That'll match the behavior of TextEdit in most cases while still allowing the caret to be visible when the background color is changed.

[Ryosuke Niwa]

(In reply to comment #8)
> The idea behind https://bugs.webkit.org/show_bug.cgi?id=135920 is that Mail + Notes can specify this in their own user stylesheet (it's an inherited property). I think that would solve the problem nicely. Thoughts?

That would mean that Notes and Mail would continue to suffer from invisible caret when the background color of text is changed to a dark color like black.

Again, I don't think that matching TextEdit is an improvement given that not being able to see the caret is never desirable for users.

[Myles C. Maxfield]

This is a case of conflicting desires. Consider the following two examples of markup:

<div contenteditable="true"><span style="color: white; background-color: black">[caret goes here]</span></div>

<div contenteditable="true"><span style="color: red;">[caret goes here]</span></div>

1) In the first example, we don't want to draw the caret as the foreground color of the <div> because it would be invisible

2) In the second example, we don't want to draw the caret as the foreground color of the <span> because a red cursor on white background is bad, too. (See <rdar://problem/17892863>)

[Ryosuke Niwa]

(In reply to comment #10)
> This is a case of conflicting desires. Consider the following two examples of markup:
> 
> <div contenteditable="true"><span style="color: white; background-color: black">[caret goes here]</span></div>
> 
> <div contenteditable="true"><span style="color: red;">[caret goes here]</span></div>
> 
> 1) In the first example, we don't want to draw the caret as the foreground color of the <div> because it would be invisible
> 
> 2) In the second example, we don't want to draw the caret as the foreground color of the <span> because a red cursor on white background is bad, too. (See <rdar://problem/17892863>)

That's why I suggested to check the background color before changing the caret color in comment #7.

[Myles C. Maxfield]

Created attachment 237586 [details]
Patch

[WebKit Commit Bot]

Attachment 237586 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/FrameSelection.cpp:1515:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 237586 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:1512
> +    auto* element = node->isElementNode() ? toElement(node) : node->parentElement();
> +    auto* rootEditableElement = node->rootEditableElement();

I don't think auto is useful here.

> Source/WebCore/editing/FrameSelection.cpp:1522
> +        if (rootEditableElement && rootEditableElement->renderer()
> +            && rootEditableElement->renderer()->style().visitedDependentColor(CSSPropertyBackgroundColor)
> +            == element->renderer()->style().visitedDependentColor(CSSPropertyBackgroundColor)) {
> +            caretColor = rootEditableElement->renderer()->style().visitedDependentColor(CSSPropertyColor);
> +            colorSpace = rootEditableElement->renderer()->style().colorSpace();
> +        } else {
> +            caretColor = element->renderer()->style().visitedDependentColor(CSSPropertyColor);
> +            colorSpace = element->renderer()->style().colorSpace();

Please put rootEditableElement->renderer()->style() into a local variable, maybe element->renderer()->style() too.

[Myles C. Maxfield]

Created attachment 237589 [details]
Patch

[Build Bot]

Comment on attachment 237589 [details]
Patch

Attachment 237589 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4594795984977920

New failing tests:
editing/caret/color-span-inside-editable.html

[Build Bot]

Created attachment 237600 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Myles C. Maxfield]

http://trac.webkit.org/changeset/173246

[Darin Adler]

Comment on attachment 237589 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:1518
> +            const auto& rootEditableStyle = rootEditableElement->renderer()->style();
> +            const auto& elementStyle = element->renderer()->style();

const auto& is really wordy and doesn’t seem helpful. I would prefer either auto* or auto here.