Bug 44862 - Can not find a caret in an editable element with black background color
Summary: Can not find a caret in an editable element with black background color
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joone Hur
URL:
Keywords:
Depends on: 117493
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-30 02:45 PDT by Joone Hur
Modified: 2017-07-18 08:27 PDT (History)
10 users (show)

See Also:


Attachments
caret color test (435 bytes, text/html)
2010-08-30 02:45 PDT, Joone Hur
no flags Details
Proposed Patch (1.49 KB, patch)
2010-08-30 07:21 PDT, Joone Hur
mitz: review-
Details | Formatted Diff | Diff
[Video] How each browser displays a caret (1.53 MB, video/ogg)
2010-08-31 01:22 PDT, Joone Hur
no flags Details
Proposed Patch2 (3.91 KB, patch)
2010-09-02 03:46 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
caret color test (1.77 KB, text/html)
2010-09-02 05:00 PDT, Joone Hur
no flags Details
Proposed Patch3 (4.05 KB, patch)
2010-10-03 08:11 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Proposed Patch4 (4.02 KB, patch)
2010-10-03 08:49 PDT, Joone Hur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joone Hur 2010-08-30 02:45:54 PDT
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.
Comment 1 Joone Hur 2010-08-30 07:21:35 PDT
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 2 mitz 2010-08-30 10:03:56 PDT
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-.
Comment 3 Joone Hur 2010-08-31 01:22:45 PDT
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.
Comment 4 Joone Hur 2010-08-31 01:49:15 PDT
(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.
Comment 5 Tony Chang 2010-08-31 10:11:02 PDT
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.
Comment 6 Ryosuke Niwa 2010-08-31 11:38:58 PDT
(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.
Comment 7 Joone Hur 2010-09-01 05:59:14 PDT
> 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.
Comment 8 Ryosuke Niwa 2010-09-01 08:54:07 PDT
(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.
Comment 9 Joone Hur 2010-09-02 03:46:01 PDT
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 10 Ryosuke Niwa 2010-09-02 04:01:54 PDT
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.
Comment 11 Joone Hur 2010-09-02 05:00:14 PDT
Created attachment 66361 [details]
caret color test
Comment 12 Ryosuke Niwa 2010-09-02 11:23:10 PDT
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.
Comment 13 Ryosuke Niwa 2010-09-02 11:52:56 PDT
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.
Comment 14 Joone Hur 2010-09-02 17:08:49 PDT
> 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?
Comment 15 Ryosuke Niwa 2010-09-02 17:18:53 PDT
(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.
Comment 16 Ojan Vafai 2010-09-02 22:37:06 PDT
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.
Comment 17 Ryosuke Niwa 2010-09-03 00:02:31 PDT
(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.
Comment 18 Ryosuke Niwa 2010-09-03 00:08:59 PDT
>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.
Comment 19 Adele Peterson 2010-09-03 10:58:25 PDT
> > 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.
Comment 20 Ryosuke Niwa 2010-09-03 11:11:38 PDT
(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.
Comment 21 Joone Hur 2010-09-06 09:05:20 PDT
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?
Comment 22 Joone Hur 2010-10-03 08:11:54 PDT
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.
Comment 23 Joone Hur 2010-10-03 08:49:52 PDT
Created attachment 69588 [details]
Proposed Patch4

Updated a bit more.
Comment 24 Ryosuke Niwa 2010-10-04 12:28:51 PDT
(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.
Comment 25 Joone Hur 2013-04-17 16:07:12 PDT
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