RESOLVED FIXED 135904
Text caret changes to color of text in Mail and Notes
https://bugs.webkit.org/show_bug.cgi?id=135904
Summary Text caret changes to color of text in Mail and Notes
Myles C. Maxfield
Reported 2014-08-13 14:06:28 PDT
Text caret changes to color of text in Mail and Notes
Attachments
Patch (59.76 KB, patch)
2014-08-13 14:08 PDT, Myles C. Maxfield
no flags
Patch (76.01 KB, patch)
2014-08-13 18:46 PDT, Myles C. Maxfield
no flags
Patch (4.51 KB, patch)
2014-09-03 15:05 PDT, Myles C. Maxfield
no flags
Patch (4.69 KB, patch)
2014-09-03 15:22 PDT, Myles C. Maxfield
simon.fraser: review+
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (780.92 KB, application/zip)
2014-09-03 16:32 PDT, Build Bot
no flags
Myles C. Maxfield
Comment 1 2014-08-13 14:08:23 PDT
Myles C. Maxfield
Comment 2 2014-08-13 14:10:26 PDT
mitz
Comment 3 2014-08-13 14:28:58 PDT
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 4 2014-08-13 18:46:32 PDT
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
Comment 5 2014-08-13 18:46:51 PDT
Ryosuke Niwa
Comment 6 2014-08-15 11:44:42 PDT
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
Comment 7 2014-08-15 11:50:38 PDT
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
Comment 8 2014-08-15 13:16:20 PDT
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
Comment 9 2014-08-16 02:18:01 PDT
(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
Comment 10 2014-08-16 10:41:35 PDT
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
Comment 11 2014-08-16 11:08:00 PDT
(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
Comment 12 2014-09-03 15:05:28 PDT
WebKit Commit Bot
Comment 13 2014-09-03 15:07:20 PDT
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 14 2014-09-03 15:08:16 PDT
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
Comment 15 2014-09-03 15:22:38 PDT
Build Bot
Comment 16 2014-09-03 16:32:22 PDT
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
Comment 17 2014-09-03 16:32:26 PDT
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
Comment 18 2014-09-03 17:57:18 PDT
Darin Adler
Comment 19 2014-09-03 23:40:55 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.