WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(76.01 KB, patch)
2014-08-13 18:46 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(4.51 KB, patch)
2014-09-03 15:05 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(4.69 KB, patch)
2014-09-03 15:22 PDT
,
Myles C. Maxfield
simon.fraser
: review+
buildbot
: commit-queue-
Details
Formatted Diff
Diff
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
Details
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1
2014-08-13 14:08:23 PDT
Created
attachment 236551
[details]
Patch
Myles C. Maxfield
Comment 2
2014-08-13 14:10:26 PDT
<
rdar://problem/17892863
>
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
Created
attachment 236572
[details]
Patch
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
Created
attachment 237586
[details]
Patch
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
Created
attachment 237589
[details]
Patch
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
http://trac.webkit.org/changeset/173246
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.
Top of Page
Format For Printing
XML
Clone This Bug