Text caret changes to color of text in Mail and Notes
Created attachment 236551 [details] Patch
<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.
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.
Created attachment 236572 [details] Patch
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.
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.
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.
(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.
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>)
(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.
Created attachment 237586 [details] Patch
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.
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.
Created attachment 237589 [details] Patch
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
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
http://trac.webkit.org/changeset/173246
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.