Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(54)

Issue 14098003: Change the caret color according to the lightness of the background color. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 2 months ago by joone
Modified:
4 years ago
CC:
blink-reviews, jamesr, Stephen Chennney, jeez, ojan, aboxhall, Julie Parent
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

We need to change the caret color according to the lightness of the background color. BUG=232188 TEST=Follow the bug description. Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152064

Patch Set 1 #

Total comments: 5

Patch Set 2 : updated patch #

Total comments: 3

Patch Set 3 : getting the caret color from the color of the element containing the text #

Patch Set 4 : updated patch using toElement #

Patch Set 5 : included updated rebaselines #

Patch Set 6 : a new test case is added #

Patch Set 7 : Patch for landing #

Unified diffs Side-by-side diffs Delta from patch set Stats (+83 lines, -1 line) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 1 chunk +18 lines, -0 lines 0 comments Download
A LayoutTests/editing/caret/caret-color.html View 1 2 3 4 5 1 chunk +38 lines, -0 lines 0 comments Download
A LayoutTests/editing/caret/caret-color-expected.txt View 1 2 3 4 5 6 1 chunk +20 lines, -0 lines 0 comments Download
M Source/core/editing/FrameSelection.cpp View 1 2 3 4 5 6 1 chunk +7 lines, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 40 (0 generated)
joone
Hello, I'd like you to review my patch.
4 years, 2 months ago (2013-04-17 17:38:52 UTC) #1
eseidel
Do other browsers do this? :)
4 years, 2 months ago (2013-04-18 07:11:20 UTC) #2
joone
On 2013/04/18 07:11:20, Eric Seidel (Google) wrote: > Do other browsers do this? :) The ...
4 years, 2 months ago (2013-04-18 15:42:06 UTC) #3
eseidel
So this brings us closer to IE, I presume?
4 years, 2 months ago (2013-04-18 16:00:36 UTC) #4
eseidel
https://codereview.chromium.org/14098003/diff/1/Source/WebCore/editing/FrameSelection.cpp File Source/WebCore/editing/FrameSelection.cpp (left): https://codereview.chromium.org/14098003/diff/1/Source/WebCore/editing/FrameSelection.cpp#oldcode1449 Source/WebCore/editing/FrameSelection.cpp:1449: caretColor = element->renderer()->style()->visitedDependentColor(CSSPropertyColor); I'm confused. This is clearly not ...
4 years, 2 months ago (2013-04-18 16:03:16 UTC) #5
joone
On 2013/04/18 16:00:36, Eric Seidel (Google) wrote: > So this brings us closer to IE, ...
4 years, 2 months ago (2013-04-18 16:13:12 UTC) #6
joone
https://chromiumcodereview.appspot.com/14098003/diff/1/Source/WebCore/editing/FrameSelection.cpp File Source/WebCore/editing/FrameSelection.cpp (left): https://chromiumcodereview.appspot.com/14098003/diff/1/Source/WebCore/editing/FrameSelection.cpp#oldcode1449 Source/WebCore/editing/FrameSelection.cpp:1449: caretColor = element->renderer()->style()->visitedDependentColor(CSSPropertyColor); On 2013/04/18 16:03:16, Eric Seidel (Google) ...
4 years, 2 months ago (2013-04-19 00:25:21 UTC) #7
joone
I've updated the patch. Please take a look at it again. https://codereview.chromium.org/14098003/diff/1/Source/WebCore/editing/FrameSelection.cpp File Source/WebCore/editing/FrameSelection.cpp (right): ...
4 years, 2 months ago (2013-04-26 19:12:55 UTC) #8
eseidel
https://codereview.chromium.org/14098003/diff/9001/Source/core/editing/FrameSelection.cpp File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/14098003/diff/9001/Source/core/editing/FrameSelection.cpp#newcode1445 Source/core/editing/FrameSelection.cpp:1445: bool isBackgroundWhite = true; This doesn't mean "isBackgroundWhite" either. ...
4 years, 2 months ago (2013-04-27 05:10:42 UTC) #9
eseidel
by "outside of it's container" I meant "outside of it's parent". This is walking the ...
4 years, 2 months ago (2013-04-27 05:12:57 UTC) #10
joone
https://codereview.chromium.org/14098003/diff/9001/Source/core/editing/FrameSelection.cpp File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/14098003/diff/9001/Source/core/editing/FrameSelection.cpp#newcode1447 Source/core/editing/FrameSelection.cpp:1447: while (renderer) { On 2013/04/27 05:10:42, Eric Seidel (Google) ...
4 years, 2 months ago (2013-04-29 19:17:27 UTC) #11
eseidel
4 years, 2 months ago (2013-04-29 19:29:29 UTC) #12
eseidel
lgtm If the various AX frameworks have hooks to tell us what our caret color ...
4 years, 2 months ago (2013-04-29 19:33:07 UTC) #13
eseidel
not lgtm. Wrong button.
4 years, 2 months ago (2013-04-29 19:33:25 UTC) #14
eseidel
I think the behavior of the caret being the same color as the text (as ...
4 years, 2 months ago (2013-04-29 19:34:37 UTC) #15
dmazzoni
I'm actually not convinced this is the right solution. I just checked Firefox, and it ...
4 years, 2 months ago (2013-04-29 19:36:52 UTC) #16
dmazzoni
On 2013/04/29 19:33:07, Eric Seidel (Google) wrote: > If the various AX frameworks have hooks ...
4 years, 2 months ago (2013-04-29 19:39:46 UTC) #17
joone
On 2013/04/29 19:34:37, Eric Seidel (Google) wrote: > I think the behavior of the caret ...
4 years, 2 months ago (2013-04-29 19:56:56 UTC) #18
joone
On 2013/04/29 19:36:52, Dominic Mazzoni wrote: > I'm actually not convinced this is the right ...
4 years, 2 months ago (2013-04-29 20:15:37 UTC) #19
joone
On 2013/04/29 19:34:37, Eric Seidel (Google) wrote: > I think the behavior of the caret ...
4 years, 2 months ago (2013-04-30 16:29:04 UTC) #20
joone
This patch allows to get the caret color from the color of the element containing ...
4 years, 1 month ago (2013-05-03 23:46:07 UTC) #21
dmazzoni
It's not obvious to me that the Node is always an Element, so it's not ...
4 years, 1 month ago (2013-05-04 06:32:45 UTC) #22
Mike West
On 2013/05/04 06:32:45, Dominic Mazzoni wrote: > It's not obvious to me that the Node ...
4 years, 1 month ago (2013-05-04 08:04:23 UTC) #23
joone
On 2013/05/04 08:04:23, Mike West (chromium) wrote: > On 2013/05/04 06:32:45, Dominic Mazzoni wrote: > ...
4 years, 1 month ago (2013-05-06 20:03:06 UTC) #24
dmazzoni
The latest patch looks reasonable to me. Eric, what do you think? How should the ...
4 years, 1 month ago (2013-05-07 05:46:16 UTC) #25
eseidel
Yup, the patch looks reasonable. Needs a test. I believe it's possible to test cursor ...
4 years, 1 month ago (2013-05-07 07:19:05 UTC) #26
eseidel
lgtm Looking through our layout tests, I see no easy way to test this.
4 years, 1 month ago (2013-05-07 07:31:31 UTC) #27
joone
On 2013/05/07 07:31:31, Eric Seidel (Google) wrote: > lgtm > > Looking through our layout ...
4 years, 1 month ago (2013-05-08 20:03:09 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joone.hur@intel.com/14098003/31001
4 years, 1 month ago (2013-05-08 20:13:07 UTC) #29
commit-bot: I haz the power
Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layout_rel&number=7236
4 years, 1 month ago (2013-05-08 20:53:29 UTC) #30
joone
15 test case are required to be rebaselined due to the caret color change, so ...
4 years, 1 month ago (2013-05-08 22:37:56 UTC) #31
eseidel
Woh. Crazy, so now when we're inside a link the cursor will be blue? Do ...
4 years, 1 month ago (2013-05-09 07:42:29 UTC) #32
joone
On 2013/05/09 07:42:29, Eric Seidel wrote: > Woh. Crazy, so now when we're inside a ...
4 years, 1 month ago (2013-05-09 09:01:34 UTC) #33
joone
Is this okay with my patch for rebaselining? or Is there any issue I should ...
4 years, 1 month ago (2013-05-13 20:25:48 UTC) #34
dmazzoni
The blue insertion point is a little odd but I think it's overall an improvement ...
4 years, 1 month ago (2013-05-15 19:05:39 UTC) #35
joone
On 2013/05/15 19:05:39, Dominic Mazzoni wrote: > The blue insertion point is a little odd ...
4 years, 1 month ago (2013-05-15 20:48:12 UTC) #36
joone
I've updated the patch with a new test case. Please take a look at the ...
4 years ago (2013-06-05 00:25:24 UTC) #37
dmazzoni
lgtm Thank you! I will land this soon if there are no other comments or ...
4 years ago (2013-06-05 15:31:45 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joone.hur@intel.com/14098003/81001
4 years ago (2013-06-07 22:26:34 UTC) #39
commit-bot: I haz the power
4 years ago (2013-06-08 00:06:27 UTC) #40
Message was sent while issue was closed.
Change committed as 152064
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 23e94e589