Created attachment 65898 [details] caret color test When editing a text in an editable element with black background color, it is hard to find where the caret is in WebKit. Because the color of caret was decided through the CSS color value of the root of the editable element. As a result, If we doesn't set a specific color for the root of the editable element, WebKit just uses black color for displaying a caret. Therefore, even if an editable element has black background color and white text color, the color of the caret is still black. In case of other browsers, Firefox uses the text color of an editable element, and Opera uses white color for displaying a caret. The test file attached shows this problem.
Created attachment 65911 [details] Proposed Patch This patch makes the color of caret to be derived from the CSS color of the current editable element instead of its editable root element.
Comment on attachment 65911 [details] Proposed Patch Does this match what other browsers do? Is the behavior platform-dependent? This patch will change the behavior in clients like Mac OS X Mail, which is not desirable, so r-.
Created attachment 66020 [details] [Video] How each browser displays a caret You can find the difference of displaying a caret among the browsers. I think Firefox displays a caret reasonably, so this patch makes WebKit display a caret like Firefox.
(In reply to comment #2) > (From update of attachment 65911 [details]) > Does this match what other browsers do? Please check the video attached. > Is the behavior platform-dependent? No, this behavior is platform-independent. > This patch will change the behavior in clients like Mac OS X Mail, which is not desirable, so r-. Yes, this patch affects on all clients which use WebKit engine. Why is it not desirable? I couldn't find the black caret easily in the black background.
Can we use the text color instead of basing it off the background color? What does IE do? Adding Ryosuke because he recently added some code to get the background color by walking up the tree in the case of transparent backgrounds. This might be useful if we base the cursor color off the background color.
(In reply to comment #4) > > This patch will change the behavior in clients like Mac OS X Mail, which is not desirable, so r-. > Yes, this patch affects on all clients which use WebKit engine. Why is it not desirable? I couldn't find the black caret easily in the black background. There's a difference between using the text color at the start of selection and white for dark background. For the markup below: <div style="background-color: black; color: white;">hello</div <div style="background-color: black; color: red;">world</div> <div style="background-color: black; color: black;">world</div> Internet Explorer gives me white caret for all three, and that is most reasonable result to me. I personally don't want to see my caret being red, blue, or etc... or at least that is not the behavior you'd see on other Windows applications.
> Internet Explorer gives me white caret for all three, and that is most reasonable result to me. I personally don't want to see my caret being red, blue, or etc... or at least that is not the behavior you'd see on other Windows applications. It seems to make sense that the caret color has to be black or white. In this case, we have to change the caret color to white in order to distinguish a caret in dark background. What background color is proper for changing the caret color? I think that it would be good to use the lightness value of HSL color space in the Color class.
(In reply to comment #7) > It seems to make sense that the caret color has to be black or white. > In this case, we have to change the caret color to white in order to distinguish a caret in dark background. Right. Note that TextEdit always uses black for caret even when text is inside a table cell and its background color is black. Since we usually match the editing behavior of WebKit to that of TextEdit, we may need to keep the current behavior for Mac. However, that doesn't sound too user-friendly. > What background color is proper for changing the caret color? Not sure. > I think that it would be good to use the lightness value of HSL color space in the Color class. I think that's reasonable but it's probably better to match Internet Explorer's behavior because we don't want to force Web developers to having to test both on MSIE and WebKit in order to determine which color scheme results in white caret. See http://support.microsoft.com/kb/84054 for example. But It's best to test at least Web 256 colors. Once we narrowed it down to some smaller range of colors, we can then test against the full 24-bit colors and see for which color MSIE produces black / white caret. But this might be rather challenging when alpha value in background color is considered, or when some element is absolutely positioned and background color comes from element behind the element. It's even more cumbersome when the element has position: fixed. Or image the case where background color is white but has a very dark image. etc... See the bug 21680 for more discussions of this sort.
Created attachment 66349 [details] Proposed Patch2 This patch makes the caret color to be changed to black or white depends on the lightness of background.
Comment on attachment 66349 [details] Proposed Patch2 > + This patch makes the caret color to be changed to black or white depends on the lightness of background. > + > + No new tests needed. I think we need tests. It's just that we cannot test it. > + Element* parentElement = element; > + bool isTransparentPage = false; > + while (!parentElement->renderer()->style()->hasBackground()) { > + if (parentElement->hasTagName(bodyTag)) { > + isTransparentPage = true; > + break; > + } > + parentElement = parentElement->parentElement(); > + } Some element may not have renderer so you should check that. Also, did you consider the case where the document is not HTML? We may never hit body element in that case. > + if (!isTransparentPage) { > + backgroundColor = parentElement->renderer()->style()->visitedDependentColor(CSSPropertyBackgroundColor); > + if (backgroundColor.lightness() < 0.5) > + caretColor = Color::white; > + } else > + caretColor = Color::black; LGTM as long as you add the null check for renderer in the loop. > +double Color::lightness() const > +{ > + double r = static_cast<double>(red()) / 255.0; > + double g = static_cast<double>(green()) / 255.0; > + double b = static_cast<double>(blue()) / 255.0; > + double max = std::max(std::max(r, g), b); > + double min = std::min(std::min(r, g), b); > + return 0.5 * (max + min); > +} This code looks rather arbitrary to me. Does this match other browser's behavior, namely that of MSIE? Also, I'm not sure if it's a good idea to add this to Color since Color is used everywhere in WebKit and we want to keep it as small as possible.
Created attachment 66361 [details] caret color test
After the second thought, I don't think MSIE is doing black / white. They're taking the XOR on the screen. Consider the following HTML: <body style="padding: 10px; margin: 0px; background-color: black;" contenteditable> <div style="width: 100%; height: 50px; background-color: white;"></div> <p style="position: absolute; top: 15px; left: 15px;">inside P</p> </body> When you move the caret to inside P, you see black caret but with your patch, the caret will be white. What we really need to do here is to obtain the bitmap from the screen and XOR it to make caret's bitmap. At least that's what MSIE and other applications do on Windows. This probably needs to be done at platform level rather than in WebCore because each platform has its own way of creating caret.
I had offline discussion with Tony, and he agrees that we need platform-specific behavior here. We can add a new callback to editing delegates, and have each editing delegate paint the caret, create image, or whatever the method appropriate abstraction to create platform-specific result.
> When you move the caret to inside P, you see black caret but with your patch, the caret will be white. What we really need to do here is to obtain the bitmap from the screen and XOR it to make caret's bitmap. At least that's what MSIE and other applications do on Windows. This probably needs to be done at platform level rather than in WebCore because each platform has its own way of creating caret. IE has a problem in the gray background. <div style="background-color: gray; color: black;">background-color: gray</div> The caret color becomes gray in the above case, so we can't find the caret. In other cases, IE seems well, including the image background. If we go to platform specific-behavior, should we follow the Windows way for all platforms?
(In reply to comment #14) > > When you move the caret to inside P, you see black caret but with your patch, the caret will be white. What we really need to do here is to obtain the bitmap from the screen and XOR it to make caret's bitmap. At least that's what MSIE and other applications do on Windows. This probably needs to be done at platform level rather than in WebCore because each platform has its own way of creating caret. > > IE has a problem in the gray background. > > <div style="background-color: gray; color: black;">background-color: gray</div> > > The caret color becomes gray in the above case, so we can't find the caret. > In other cases, IE seems well, including the image background. Right, but a lot of developers will probably know to avoid that. > If we go to platform specific-behavior, should we follow the Windows way for all platforms? No, that's not what I mean. Our behavior on each platform should be natural to each platform (i.e. de facto standard). From what I've checked so far: 1. Linux (maybe different on Qt): use text color 2. Mac: always black 3. Windows: XOR with the painted screen But we need to do further research making any decision here because caret color would affect vast majority of users, and will have a significant impact on usability.
I'm not sure being platform-correct is very important here. I doubt many users are aware of the cursor color unless seeing the cursor is difficult. The important thing here is just making sure that we don't have an invisible cursor. It seems to me that the Firefox behavior of matching the text color is simple and meets this need almost all the time. I think we should just match Firefox. Background-color is too complicated.
(In reply to comment #16) > I'm not sure being platform-correct is very important here. I doubt many users are aware of the cursor color unless seeing the cursor is difficult. The important thing here is just making sure that we don't have an invisible cursor. Right, not having an invisible cursor is important. But as mitz commented on #c2, this change will affect all WebKit clients and we should make sure it improves user experience. > It seems to me that the Firefox behavior of matching the text color is simple and meets this need almost all the time. I think we should just match Firefox. Background-color is too complicated. I think Firefox's matching caret color to text color hinders accessibility. I for one have color blindness, and red caret in red text blends too well and makes it harder to recognize. There is a huge benefit in taking XOR or always using black or white because then gray-tone difference is still maintained. I can see them clearly it was black or white, or XORed with the text & background like MSIE does although XORed caret is still hard to see when the background or text is gray.
>then gray-tone difference is still maintained. By gray-tone difference, I meant lightness difference as in http://www.lighthouse.org/accessibility/design/accessible-print-design/effective-color-contrast.
> > It seems to me that the Firefox behavior of matching the text color is simple and meets this need almost all the time. I think we should just match Firefox. Background-color is too complicated. > > I think Firefox's matching caret color to text color hinders accessibility. I for one have color blindness, and red caret in red text blends too well and makes it harder to recognize. There is a huge benefit in taking XOR or always using black or white because then gray-tone difference is still maintained. I can see them clearly it was black or white, or XORed with the text & background like MSIE does although XORed caret is still hard to see when the background or text is gray. If the text color is hard to see on the background color, then that's a webpage design problem that won't be fixed by having a more visible caret. If we decide not to do a black caret in some cases, it seems simplest to just match the text color.
(In reply to comment #19) > If the text color is hard to see on the background color, then that's a webpage design problem that won't be fixed by having a more visible caret. If we decide not to do a black caret in some cases, it seems simplest to just match the text color. No, that's not what I meant. What I'm afraid is that matching the caret color to text color would hinder user's ability to easily spot the caret. For example: <span style="font-weight: bold; color: #FF9966;">hello</span> is perfectly visible text on white background but caret next to l is really hard to see on Firefox because Firefox sets its color to #FF9966. In my opinion, recognition is much easier on WebKit because the caret color is black.
I agree with Ryosuke, because caret color is usually black, white, and gray. However, there seems various ways of how to display a caret for each platform. Therefore, we need to make decisions as follows before updating the patch attached; 1) Do we need to display a caret at platform level rather than in WebCore? Yes, because each platform has it own way of creating a caret. 2) How to display a caret for each platform - Windows => XOR with the painted screen like IE - Mac => Black or White (but, Pages always uses gray color) - Linux => Black or White (but, OpenOffice uses XOR with the painted screen) 3) How to change the caret color on the dark/bright background in Mac/Linux. - Use the lightness value of HSL color space? Do you have any idea?
Created attachment 69587 [details] Proposed Patch3 I updated the patch with the changes commented by Ryosuke(comment #10). Also, I will try to revise the patch for having each editing delegate paint the caret.
Created attachment 69588 [details] Proposed Patch4 Updated a bit more.
(In reply to comment #21) > 1) Do we need to display a caret at platform level rather than in WebCore? > Yes, because each platform has it own way of creating a caret. Agreed. > 2) How to display a caret for each platform > - Windows => XOR with the painted screen like IE > - Mac => Black or White (but, Pages always uses gray color) > - Linux => Black or White (but, OpenOffice uses XOR with the painted screen) This sounds very reasonable to me. Although TextEdit always uses black caret (insert a table and change the cell background to test this), I don't think it's user-friendly enough for us to copy this behavior on Mac. OpenOffice is probably just mimicking the behavior of Microsoft Office, and your proposal sounds reasonable to me. +tonikitoo, +ossy for gtk / qt. > 3) How to change the caret color on the dark/bright background in Mac/Linux. > - Use the lightness value of HSL color space? Yes, that sounds reasonable to me. But you should use Color::getHSL instead in your patch instead of manually converting RGB values to HSL. You can add lightless function that ignores hue and saturation if you're so inclined.
I am updating this patch and will apply this patch to WebKit and Blink together. https://code.google.com/p/chromium/issues/detail?id=232188
Comment on attachment 69588 [details] Proposed Patch4 View in context: https://bugs.webkit.org/attachment.cgi?id=69588&action=review > WebCore/ChangeLog:10 > + Reviewed by NOBODY (OOPS!). > + > + It's hard to find a caret in an editable element with black background color > + https://bugs.webkit.org/show_bug.cgi?id=44862 > + > + This patch makes the caret color to be changed to black or white depends on the lightness of background. > + > + No new tests needed. All these lines were ordered incorrectly. Should look like this: Bug title Bug URL Reviewed by ~ Description No new tests...
Created attachment 438020 [details] Patch
Created attachment 438022 [details] Patch
Created attachment 438052 [details] Patch
Created attachment 438053 [details] Patch
Created attachment 438213 [details] Fix IOS layout test failure
Created attachment 438254 [details] caret-black-on-light-background.html
Created attachment 438255 [details] color-span-inside-editable-background.html
Created attachment 438257 [details] Skip ios-wk2 layout tests failure
Comment on attachment 438257 [details] Skip ios-wk2 layout tests failure View in context: https://bugs.webkit.org/attachment.cgi?id=438257&action=review > Source/WebCore/editing/FrameSelection.cpp:1819 > + while (renderer) { > + if (renderer->style().hasBackground()) { > + parentStyle = &renderer->style(); > + hasBackground = true; > + break; > + } > + renderer = renderer->parent(); > + } > + > + if (hasBackground) { > + Color backgroundColor = parentStyle->visitedDependentColor(CSSPropertyBackgroundColor); > + Color caretColor = Color::black; > + if (backgroundColor.lightness() <= 0.5) > + caretColor = Color::gray; > + > + return caretColor; This is pretty naive. It knows nothing about css z-index or positioning. It assumes background color is visible, and not hidden by background-image.
Created attachment 450958 [details] [WIP] check the bg color of the layer behind the caret
Created attachment 451020 [details] “check
Created attachment 451021 [details] check the bg color of the layer behind the caret
Created attachment 451027 [details] check the bg color of the layer behind the caret
Created attachment 451029 [details] fix ios build errors
Created attachment 451050 [details] Fix build errors and layout test failures
Created attachment 451348 [details] add RenderLayer::getLayerBehindRect()
Created attachment 451429 [details] rebase on master
Created attachment 451457 [details] rebase on master
Created attachment 451787 [details] check z-index
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
The updated patch checks if there is any positioned RenderLayer behind the editing element. It also checks if the z-index of the RenderLayer is smaller than the enclosing RenderLayer of the editing element. We also need to check if the RenderLayers are transformed, but this patch doesn't consider it yet.
Created attachment 451935 [details] Patch
Comment on attachment 451935 [details] Patch Before we go further down this road, I think we have to re-think the approach. Ultimately, the answer of "what is the color of this pixel" means re-running the entire painting logic, which you're starting to implement here. Implementing a 2nd version of an algorithm that already exists is not a sustainable approach. If we really think this is important enough to fix, I think it either needs to use the (possibly expensive) snapshotting approach, or do something very simple.
Yes, the current approach is unable to find the proper caret color in CSS image and gradient backgrounds. So we need to get the pixels in the caret area and then get the average brightness of the pixels to find the appropriate caret color(black or gray or white).