Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14098003:
Change the caret color according to the lightness of the background color. (Closed)
|
Created:
7 years, 8 months ago by joone Modified:
7 years, 6 months ago CC:
blink-reviews, jamesr, Stephen Chennney, jeez, ojan, aboxhall, Julie Parent Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionWe 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 #
Messages
Total messages: 40 (0 generated)
Hello, I'd like you to review my patch.
Do other browsers do this? :)
On 2013/04/18 07:11:20, Eric Seidel (Google) wrote: > Do other browsers do this? :) The caret color is different among browsers as follows: 1. Firefox: use text color 2. Chrome/Safari: always black 3. IE: XOR with the painted screen
So this brings us closer to IE, I presume?
https://codereview.chromium.org/14098003/diff/1/Source/WebCore/editing/FrameS... File Source/WebCore/editing/FrameSelection.cpp (left): https://codereview.chromium.org/14098003/diff/1/Source/WebCore/editing/FrameS... Source/WebCore/editing/FrameSelection.cpp:1449: caretColor = element->renderer()->style()->visitedDependentColor(CSSPropertyColor); I'm confused. This is clearly not always black, as you noted above? https://codereview.chromium.org/14098003/diff/1/Source/WebCore/editing/FrameS... File Source/WebCore/editing/FrameSelection.cpp (right): https://codereview.chromium.org/14098003/diff/1/Source/WebCore/editing/FrameS... Source/WebCore/editing/FrameSelection.cpp:1452: if (renderer->style()->hasBackground()) { I'm not sure what it means to be a transparent page. RenderView always paints a background, no?
On 2013/04/18 16:00:36, Eric Seidel (Google) wrote: > So this brings us closer to IE, I presume? Yes, but this allows us to use only black or white color for caret.
https://chromiumcodereview.appspot.com/14098003/diff/1/Source/WebCore/editing... File Source/WebCore/editing/FrameSelection.cpp (left): https://chromiumcodereview.appspot.com/14098003/diff/1/Source/WebCore/editing... Source/WebCore/editing/FrameSelection.cpp:1449: caretColor = element->renderer()->style()->visitedDependentColor(CSSPropertyColor); On 2013/04/18 16:03:16, Eric Seidel (Google) wrote: > I'm confused. This is clearly not always black, as you noted above? Yes, the caret color is not always black. I misunderstood how the caret color is painted. The caret color is determined by the color of the root editable element(the topmost parent or the element itself) https://chromiumcodereview.appspot.com/14098003/diff/1/Source/WebCore/editing... File Source/WebCore/editing/FrameSelection.cpp (right): https://chromiumcodereview.appspot.com/14098003/diff/1/Source/WebCore/editing... Source/WebCore/editing/FrameSelection.cpp:1452: if (renderer->style()->hasBackground()) { On 2013/04/18 16:03:16, Eric Seidel (Google) wrote: > I'm not sure what it means to be a transparent page. RenderView always paints a > background, no? Yes, RenderView paints the background with FrameView's background color that is white, so the variable name should be changed to isWhitePage or isWhiteBackground.
I've updated the patch. Please take a look at it again. https://codereview.chromium.org/14098003/diff/1/Source/WebCore/editing/FrameS... File Source/WebCore/editing/FrameSelection.cpp (right): https://codereview.chromium.org/14098003/diff/1/Source/WebCore/editing/FrameS... Source/WebCore/editing/FrameSelection.cpp:1452: if (renderer->style()->hasBackground()) { On 2013/04/18 16:03:16, Eric Seidel (Google) wrote: > I'm not sure what it means to be a transparent page. RenderView always paints a > background, no? How about isBackgroundWhite instead of isTransparentPage?
https://codereview.chromium.org/14098003/diff/9001/Source/core/editing/FrameS... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/14098003/diff/9001/Source/core/editing/FrameS... Source/core/editing/FrameSelection.cpp:1445: bool isBackgroundWhite = true; This doesn't mean "isBackgroundWhite" either. :) I guess your previous usage was more correct, now that I'm understanding what this does. https://codereview.chromium.org/14098003/diff/9001/Source/core/editing/FrameS... Source/core/editing/FrameSelection.cpp:1447: while (renderer) { I would re-write this as a helper function: RenderObject* closestAncestorWithBackground(RenderObject*); Then this becomes: RenderObject* opaqueAncestor = closestAncestorWithBackground(element->renderer()); That said, I dont' think this code will actually work in all cases. Take for example an absolutely positioned div. It may be positioned outside of it's container and may end up with the "wrong" caret color. I think to do this correctly, you'd have to actually hit-test the caret location. But I'm not sure this is really worth the complexity. Do we have compatibility concerns driving this? Sites which depend on this behavior? Or is this just a UI preference?
by "outside of it's container" I meant "outside of it's parent". This is walking the parent chain in the rendering tree, which may not be the things which are painting "behind" you. Knowing what's painting behind you is not a question you can answer by walking up your parent chain in the rendering tree. There are lots of reasons (position, z-order, text-position, line-height, etc) why what paints behind you may not be anything resembling your parent. :)
https://codereview.chromium.org/14098003/diff/9001/Source/core/editing/FrameS... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/14098003/diff/9001/Source/core/editing/FrameS... Source/core/editing/FrameSelection.cpp:1447: while (renderer) { On 2013/04/27 05:10:42, Eric Seidel (Google) wrote: > I would re-write this as a helper function: > > RenderObject* closestAncestorWithBackground(RenderObject*); > > Then this becomes: > > RenderObject* opaqueAncestor = > closestAncestorWithBackground(element->renderer()); > Okay, I will add this helper function. > That said, I dont' think this code will actually work in all cases. Take for > example an absolutely positioned div. It may be positioned outside of it's > container and may end up with the "wrong" caret color. > > I think to do this correctly, you'd have to actually hit-test the caret > location. But I'm not sure this is really worth the complexity. > I didn't think of this case. Can we get the actual background color from Skia? I think we can change the caret color according to the screen color if this is easier than hit-testing the caret color. > Do we have compatibility concerns driving this? Sites which depend on this > behavior? Or is this just a UI preference? There seems no compatibility issues because each browser engine works differently. I think this is an accessibility issue or usability issue. I heard that black/white caret is more visible to people who have color blindness
lgtm If the various AX frameworks have hooks to tell us what our caret color should be, we should definitely respect those. But I don't think that this patch-as-is gets us more accessibility?
not lgtm. Wrong button.
I think the behavior of the caret being the same color as the text (as we currently seem to) seems reasonable. I'm not sure why we'd want to change this. The patch as proposed, seems to add complexity for an incomplete implementation.
I'm actually not convinced this is the right solution. I just checked Firefox, and it appears to set the caret based on the foreground color of the text, not based on the background color. Is there any reason we should behave differently? Or am I misunderstanding?
On 2013/04/29 19:33:07, Eric Seidel (Google) wrote: > If the various AX frameworks have hooks to tell us what our caret color should > be, we should definitely respect those. For web content, we can't use the system caret color unless we also respected all of the system colors. Other browsers provide this option, but realistically tons of sites are completely broken when you do this - it's basically just custom style sheets. Users can install software like ZoomText that uses accessibility APIs to add extra highlighting to the cursor. See this example: http://www.aisquared.com/zoomtextexpress/more/cursor_enhancements/
On 2013/04/29 19:34:37, Eric Seidel (Google) wrote: > I think the behavior of the caret being the same color as the text (as we > currently seem to) seems reasonable. I'm not sure why we'd want to change this. > The patch as proposed, seems to add complexity for an incomplete > implementation. Please take a look at Ryosuke's comment: https://bugs.webkit.org/show_bug.cgi?id=44862#c17 Also, I want to avoid the case that the caret color is the same as the background color. In this case, there is no way to find contentEditable elements.
On 2013/04/29 19:36:52, Dominic Mazzoni wrote: > I'm actually not convinced this is the right solution. > > I just checked Firefox, and it appears to set the caret based on the foreground > color of the text, not based on the background color. Is there any reason we > should behave differently? Or am I misunderstanding? You're right. The way of getting the caret color in Webkit is different from Firefox. At the least, we need to follow the Firefox behavior in order to make the caret visible from the background.
On 2013/04/29 19:34:37, Eric Seidel (Google) wrote: > I think the behavior of the caret being the same color as the text (as we > currently seem to) seems reasonable. I'm not sure why we'd want to change this. > The patch as proposed, seems to add complexity for an incomplete > implementation. If we add a table in Google Docs (or contentEditable <body> element) and then change the cell background color to black and the font color to white, the caret becomes invisible because the caret color is black, which is getting from its contentEditable parent's foreground color. So we need to change the caret color with the current focused element's foreground color at least if you think that it is too complicated to hit-test the caret.
This patch allows to get the caret color from the color of the element containing the text, not the contentEditable parent element. This is the same as Firefox. It makes the caret more visible in some cases like black background with white font color: http://jsfiddle.net/cubix/xDZJY/
It's not obvious to me that the Node is always an Element, so it's not safe to just cast it - and you'll need to figure out what to do if it is a node - get its parentElement? However, I like the direction is going. You've definitely identified a real bug and something along these lines should be the right fix.
On 2013/05/04 06:32:45, Dominic Mazzoni wrote: > It's not obvious to me that the Node is always an Element, so it's > not safe to just cast it - and you'll need to figure out what to do if > it is a node - get its parentElement? > > However, I like the direction is going. You've definitely identified a real > bug and something along these lines should be the right fix. To strengthen Dominic's claim, please never blindly static_cast. We've been hit by various security issues caused by casting something into something it's not. not lgtm for that. There's a 'toElement' method in Element.h[1] that will make the proper assertions before casting. Please use that instead. As a side benefit, those assertions will help you determine if and when your Node isn't actually an Element if you build and run the tests in debug. :) [1]: https://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Element.h#l724
On 2013/05/04 08:04:23, Mike West (chromium) wrote: > On 2013/05/04 06:32:45, Dominic Mazzoni wrote: > > It's not obvious to me that the Node is always an Element, so it's > > not safe to just cast it - and you'll need to figure out what to do if > > it is a node - get its parentElement? > > > > However, I like the direction is going. You've definitely identified a real > > bug and something along these lines should be the right fix. > > To strengthen Dominic's claim, please never blindly static_cast. We've been hit > by various security issues caused by casting something into something it's not. > not lgtm for that. > > There's a 'toElement' method in Element.h[1] that will make the proper > assertions before casting. Please use that instead. As a side benefit, those > assertions will help you determine if and when your Node isn't actually an > Element if you build and run the tests in debug. :) > > [1]: https://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Element.h#l724 @Dominic, @Mike Thanks for the review. As Dominic mentioned, the Node is not always Element. It is mostly Text node. So, there are two case of finding the Element that we are getting the the caret color. 1) When editing a text, get the caret color from the parent Element of the Text node. 2) When editing an Element, get the caret color from the Element itself. My latest patch allows to get the caret color like this. Anyway, we need to re-baseline some test cases in editing because the caret color is changed.
The latest patch looks reasonable to me. Eric, what do you think? How should the rebaselines be handled?
Yup, the patch looks reasonable. Needs a test. I believe it's possible to test cursor painting with DumpRenderTree, but maybe I'm remembering wrong?
lgtm Looking through our layout tests, I see no easy way to test this.
On 2013/05/07 07:31:31, Eric Seidel (Google) wrote: > lgtm > > Looking through our layout tests, I see no easy way to test this. There are 15 test failures due to the caret color change. Regressions: Unexpected image-only failures (15) editing/deleting/delete-4083333-fix.html [ ImageOnlyFailure ] editing/deleting/type-delete-after-quote.html [ ImageOnlyFailure ] editing/inserting/4840662.html [ ImageOnlyFailure ] editing/inserting/4959067.html [ ImageOnlyFailure ] editing/inserting/5156401-2.html [ ImageOnlyFailure ] editing/inserting/insert-paragraph-05.html [ ImageOnlyFailure ] editing/pasteboard/5071074-2.html [ ImageOnlyFailure ] editing/pasteboard/5071074.html [ ImageOnlyFailure ] editing/pasteboard/5156401-1.html [ ImageOnlyFailure ] editing/pasteboard/5601583-1.html [ ImageOnlyFailure ] editing/pasteboard/paste-blockquote-after-blockquote.html [ ImageOnlyFailure ] editing/selection/editable-links.html [ ImageOnlyFailure ] editing/selection/end-of-document.html [ ImageOnlyFailure ] editing/style/5065910.html [ ImageOnlyFailure ] editing/style/5084241.html [ ImageOnlyFailure ] I will rebaseline the test cases.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joone.hur@intel.com/14098003/31001
Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
15 test case are required to be rebaselined due to the caret color change, so I added new rebaselines to my updated patch.
Woh. Crazy, so now when we're inside a link the cursor will be blue? Do other browsers/editors do that? I guess that's OK and lest you know if you're inside the link or not?
On 2013/05/09 07:42:29, Eric Seidel wrote: > Woh. Crazy, so now when we're inside a link the cursor will be blue? Do other > browsers/editors do that? > Yes, Firefox does, but editors(Open Office, Google Docs) don't. > I guess that's OK and lest you know if you're inside the link or not? I think that the users already know whether the cursor is inside the link or not, so following the link color seems more natural. Actually, it's hard for me to distinguish whether the color is blue or black because the cursor width is just 2 pixel(?).
Is this okay with my patch for rebaselining? or Is there any issue I should fix?
The blue insertion point is a little odd but I think it's overall an improvement to fix cases where it was completely invisible. Would it be too much to ask to add a new layout test that demonstrates a case that was totally broken before? That way we can try to improve this further but be confident it won't regress.
On 2013/05/15 19:05:39, Dominic Mazzoni wrote: > The blue insertion point is a little odd but I think it's overall an improvement > to fix cases where it was completely invisible. > > Would it be too much to ask to add a new layout test that demonstrates a case > that was totally broken before? That way we can try to improve this further but > be confident it won't regress. Ok, I will add a new layout test that demonstrates the case that the caret becomes invisible.
I've updated the patch with a new test case. Please take a look at the test case.
lgtm Thank you! I will land this soon if there are no other comments or objections raised.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joone.hur@intel.com/14098003/81001
Message was sent while issue was closed.
Change committed as 152064 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
