Bug 44862 – Make the caret more visible on any background

• Component: HTML Editing
• Status: NEW

[Joone Hur]

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.

[Joone Hur]

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-.

[Joone Hur]

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.

[Joone Hur]

(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.

[Tony Chang]

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.

[Ryosuke Niwa]

(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.

[Joone Hur]

> 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.

[Ryosuke Niwa]

(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.

[Joone Hur]

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.

[Ryosuke Niwa]

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.

[Joone Hur]

Created attachment 66361 [details]
caret color test

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Joone Hur]

> 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?

[Ryosuke Niwa]

(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.

[Ojan Vafai]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

>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.

[Adele Peterson]

> > 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.

[Ryosuke Niwa]

(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.

[Joone Hur]

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?

[Joone Hur]

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.

[Joone Hur]

Created attachment 69588 [details]
Proposed Patch4

Updated a bit more.

[Ryosuke Niwa]

(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.

[Joone Hur]

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

[Ryosuke Niwa]

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...

[Joone Hur]

Created attachment 438020 [details]
Patch

[Joone Hur]

Created attachment 438022 [details]
Patch

[Joone Hur]

Created attachment 438052 [details]
Patch

[Joone Hur]

Created attachment 438053 [details]
Patch

[Joone Hur]

Created attachment 438213 [details]
Fix IOS layout test failure

[Joone Hur]

Created attachment 438254 [details]
caret-black-on-light-background.html

[Joone Hur]

Created attachment 438255 [details]
color-span-inside-editable-background.html

[Joone Hur]

Created attachment 438257 [details]
Skip ios-wk2 layout tests failure

[Simon Fraser (smfr)]

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.

[Joone Hur]

Created attachment 450958 [details]
[WIP] check the bg color of the layer behind the caret

[Joone Hur]

Created attachment 451020 [details]
“check

[Joone Hur]

Created attachment 451021 [details]
check the bg color of the layer behind the caret

[Joone Hur]

Created attachment 451027 [details]
check the bg color of the layer behind the caret

[Joone Hur]

Created attachment 451029 [details]
fix ios build errors

[Joone Hur]

Created attachment 451050 [details]
Fix build errors and layout test failures

[Joone Hur]

Created attachment 451348 [details]
add RenderLayer::getLayerBehindRect()

[Joone Hur]

Created attachment 451429 [details]
rebase on master

[Joone Hur]

Created attachment 451457 [details]
rebase on master

[Joone Hur]

Created attachment 451787 [details]
check z-index

[EWS Watchlist]

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

[Joone Hur]

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.

[Joone Hur]

Created attachment 451935 [details]
Patch

[Simon Fraser (smfr)]

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.

[Joone Hur]

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).