Bug 117493

Summary:

Caret should respect text background color

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

HTML Editing

Assignee:

Joone Hur <joone>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, buildbot, cdumez, commit-queue, darin, enrica, ews-watchlist, joone, mifenton, rniwa, webkit-bug-importer

Priority:

Keywords:

BlinkMergeCandidate, InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

44862

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion	none
Archive of layout-test-results from APPLE-EWS-6 for win-future	none
Patch	none
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2	none
Archive of layout-test-results from APPLE-EWS-3 for win-future	none
Archive of layout-test-results from APPLE-EWS-2 for win-future	none
Patch	none
Patch	none
caret color test	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	ews-feeder: commit-queue-
Patch	none
Updated ChangeLog	none
Patch	none

Description Ryosuke Niwa 2013-06-10 23:01:07 PDT

Consider merging https://chromium.googlesource.com/chromium/blink/+/3f7fa25f27ce8e067136626f6fbef2e9341aab2c.

diff --git a/Source/core/editing/FrameSelection.cpp b/Source/core/editing/FrameSelection.cpp
index 5a5f03c..229f473 100644
--- a/Source/core/editing/FrameSelection.cpp
+++ b/Source/core/editing/FrameSelection.cpp
@@ -1458,7 +1458,13 @@
         return;
 
     Color caretColor = Color::black;
-    Element* element = node->rootEditableElement();
+
+    Element* element;
+    if (node->isElementNode())
+        element = toElement(node);
+    else
+        element = node->parentElement();
+
     if (element && element->renderer())
         caretColor = element->renderer()->style()->visitedDependentColor(CSSPropertyColor);

Comment 1 Joone Hur 2013-06-17 21:46:26 PDT

Hi Ryosoke,

I made this patch so I will prepare the patch for WebKit.

Thanks,
Joone

Comment 2 Joone Hur 2013-06-17 21:50:48 PDT

(In reply to comment #1)
> Hi Ryosoke,

Ryosuke, I'm sorry that I misspelled your name. :-)

Comment 3 Joone Hur 2013-06-26 01:00:43 PDT

Created attachment 205460 [details]
Patch

Comment 4 Build Bot 2013-06-26 05:41:53 PDT

Comment on attachment 205460 [details]
Patch

Attachment 205460 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/912792

New failing tests:
editing/caret/caret-color.html

Comment 5 Build Bot 2013-06-26 05:41:54 PDT

Created attachment 205479 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

Comment 6 Build Bot 2013-06-26 08:11:32 PDT

Comment on attachment 205460 [details]
Patch

Attachment 205460 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/930463

New failing tests:
editing/caret/caret-color.html

Comment 7 Build Bot 2013-06-26 08:11:34 PDT

Created attachment 205493 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

Comment 8 Build Bot 2013-06-26 09:14:19 PDT

Comment on attachment 205460 [details]
Patch

Attachment 205460 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/967308

New failing tests:
fast/media/mq-transform-03.html
fast/forms/search-event-delay.html
fast/media/mq-transform-02.html
editing/caret/caret-color.html
platform/win/accessibility/multiple-select-element-role.html

Comment 9 Build Bot 2013-06-26 09:14:21 PDT

Created attachment 205502 [details]
Archive of layout-test-results from APPLE-EWS-6 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-6  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

Comment 10 Ryosuke Niwa 2013-06-26 09:50:26 PDT

Comment on attachment 205460 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205460&action=review

> Source/WebCore/ChangeLog:8
> +        Test: editing/caret/caret-color.html

This line should appear after the long description proceeded and succeeded by a single blank line.

> Source/WebCore/ChangeLog:13
> +

i.e. here.

> Source/WebCore/editing/FrameSelection.cpp:1477
> +    Element* element;
> +    if (node->isElementNode())
> +        element = toElement(node);
> +    else
> +        element = node->parentElement();

I would have preferred to use tertiary expression here as in:
Element* element = node->isElementNode() = toElement(node) : node->parentElement();

> LayoutTests/ChangeLog:14
> +        * editing/caret/caret-color-expected.txt: Added.
> +        * editing/caret/caret-color.html: Added.

We need .png file. Use run-webkit-tests --pixel.

> LayoutTests/editing/caret/caret-color.html:1
> +<html> 

Missing DOCTYPE.

> LayoutTests/editing/caret/caret-color.html:10
> +<style>
> +.editing { 
> +    border: 2px solid red; 
> +    padding: 12px; 
> +    font-size: 24px; 
> +}
> +</style>

This is really for old editing tests. We don't need these styles anymore.

Comment 11 Joone Hur 2013-06-27 23:32:29 PDT

(In reply to comment #10)
> (From update of attachment 205460 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205460&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Test: editing/caret/caret-color.html
> 
> This line should appear after the long description proceeded and succeeded by a single blank line.
> 
> > Source/WebCore/ChangeLog:13
> > +
> 
> i.e. here.

Done.

> 
> > Source/WebCore/editing/FrameSelection.cpp:1477
> > +    Element* element;
> > +    if (node->isElementNode())
> > +        element = toElement(node);
> > +    else
> > +        element = node->parentElement();
> 
> I would have preferred to use tertiary expression here as in:
> Element* element = node->isElementNode() = toElement(node) : node->parentElement();

Done.

> 
> > LayoutTests/ChangeLog:14
> > +        * editing/caret/caret-color-expected.txt: Added.
> > +        * editing/caret/caret-color.html: Added.
> 
> We need .png file. Use run-webkit-tests --pixel.

Done, but I am just able to add a png file for the EFL port.

> 
> > LayoutTests/editing/caret/caret-color.html:1
> > +<html> 
> 
> Missing DOCTYPE.
> 
> > LayoutTests/editing/caret/caret-color.html:10
> > +<style>
> > +.editing { 
> > +    border: 2px solid red; 
> > +    padding: 12px; 
> > +    font-size: 24px; 
> > +}
> > +</style>
> 
> This is really for old editing tests. We don't need these styles anymore.

Done.

I will upload an updated patch soon.
Thank you for the review.

Comment 12 Joone Hur 2013-06-28 01:44:25 PDT

Created attachment 205678 [details]
Patch

Comment 13 Build Bot 2013-06-28 02:48:50 PDT

Comment on attachment 205678 [details]
Patch

Attachment 205678 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/866481

New failing tests:
editing/caret/caret-color.html

Comment 14 Build Bot 2013-06-28 02:48:52 PDT

Created attachment 205685 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

Comment 15 Build Bot 2013-06-28 03:24:39 PDT

Comment on attachment 205678 [details]
Patch

Attachment 205678 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/998220

New failing tests:
editing/caret/caret-color.html

Comment 16 Build Bot 2013-06-28 03:24:41 PDT

Created attachment 205692 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

Comment 17 Build Bot 2013-06-29 08:13:32 PDT

Comment on attachment 205678 [details]
Patch

Attachment 205678 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/943665

New failing tests:
editing/caret/caret-color.html

Comment 18 Build Bot 2013-06-29 08:13:35 PDT

Created attachment 205768 [details]
Archive of layout-test-results from APPLE-EWS-3 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-3  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

Comment 19 Build Bot 2013-06-29 10:15:58 PDT

Comment on attachment 205678 [details]
Patch

Attachment 205678 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/866668

New failing tests:
fast/frames/seamless/seamless-nested-crash.html
fast/js/global-constructors-attributes-shared-worker.html
fast/forms/select/popup-closes-on-blur.html
fast/media/mq-transform-03.html
editing/selection/user-select-all-image-with-single-click.html
fast/forms/search-event-delay.html
fast/js/global-constructors-attributes-dedicated-worker.html
editing/selection/user-select-all-with-single-click.html
fast/media/mq-transform-02.html
editing/caret/caret-color.html
platform/win/accessibility/multiple-select-element-role.html

Comment 20 Build Bot 2013-06-29 10:16:00 PDT

Created attachment 205769 [details]
Archive of layout-test-results from APPLE-EWS-2 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-2  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

Comment 21 Joone Hur 2013-07-09 23:06:15 PDT

Created attachment 206369 [details]
Patch

This patch allows to skip the caret color test case on the Mac port

Comment 22 WebKit Commit Bot 2013-07-13 23:03:37 PDT

Comment on attachment 206369 [details]
Patch

Clearing flags on attachment: 206369

Committed r152612: <http://trac.webkit.org/changeset/152612>

Comment 23 WebKit Commit Bot 2013-07-13 23:03:43 PDT

All reviewed patches have been landed.  Closing bug.

Comment 24 Benjamin Poulain 2013-07-14 23:35:05 PDT

Don't forget to rebaseline.

Comment 25 Joone Hur 2013-07-22 12:49:56 PDT

(In reply to comment #24)
> Don't forget to rebaseline.

http://trac.webkit.org/changeset/152987

Thanks!

Comment 26 Joone Hur 2021-02-22 01:11:05 PST

Reopening to attach new patch.

Comment 27 Joone Hur 2021-02-22 01:11:07 PST

Created attachment 421168 [details]
Patch

Comment 28 Joone Hur 2021-02-22 01:14:14 PST

Created attachment 421169 [details]
caret color test

Comment 29 Joone Hur 2021-02-22 01:17:43 PST

The caret is not visible in the first line in the test file attached.

Comment 30 Joone Hur 2021-02-22 02:03:58 PST

Created attachment 421173 [details]
Patch

Comment 31 Ryosuke Niwa 2021-02-22 18:37:16 PST

Looks like the tests are failing. We need to fix that.

Comment 32 Joone Hur 2021-02-22 20:48:52 PST

Sure, I will fix them. Thanks!

Comment 33 Joone Hur 2021-03-09 00:06:34 PST

Created attachment 422670 [details]
Patch

Comment 34 Joone Hur 2021-03-09 09:45:46 PST

Created attachment 422719 [details]
Patch

Comment 35 Joone Hur 2021-08-24 01:54:52 PDT

Created attachment 436267 [details]
Patch

Comment 36 Ryosuke Niwa 2021-08-24 12:50:08 PDT

Comment on attachment 436267 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436267&action=review

> Source/WebCore/editing/FrameSelection.cpp:1793
> +    auto* parentElement = node ? node->parentElement() : nullptr;

Please use RefPtr

Comment 37 Joone Hur 2021-08-24 17:02:00 PDT

Created attachment 436348 [details]
Patch

Comment 38 Ryosuke Niwa 2021-08-24 17:54:59 PDT

Comment on attachment 436348 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436348&action=review

> Source/WebCore/ChangeLog:13
> +        the text, not the parent element that has the contentEditable attribute.

not *the root editable element*.

> LayoutTests/editing/caret/color-span-inside-editable-background-expected.html:-1
> -This test makes sure that carets in content editable divs with a background color specified remain black even if there is a span inside them with a foreground color specified.

You're missing the change log for LayoutTests.
Also, please add new tests instead of modifying existing test cases.

Comment 39 Joone Hur 2021-08-25 17:24:02 PDT

Created attachment 436446 [details]
Patch

Comment 40 Joone Hur 2021-08-25 18:06:26 PDT

Created attachment 436451 [details]
Patch

Comment 41 Joone Hur 2021-08-25 18:08:07 PDT

LayoutTests/editing/caret/caret-color.html is the original test case I added for this bug so I just updated it.

Comment 42 Darin Adler 2021-08-25 18:30:29 PDT

Comment on attachment 436451 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436451&action=review

Don’t want to interfere with someone else doing a review+, but I have a few comments.

> Source/WebCore/ChangeLog:9
> +        This bug was fixed in the below commit:
> +        https://trac.webkit.org/changeset/152612/webkit

This is a confusing comment. When you say "this bug" you are referring to another bug, not this bug.

> Source/WebCore/ChangeLog:13
> +        This patch allows the caret to become visible in the black background
> +        by getting the caret color from the element containing
> +        the text, not the root editable element that has the contentEditable attribute.

Does this work well if there are non-opaque colors involved? I’m worried we don’t have enough test cases. It seems logically that we’d need to have a loop in the implementation to handle transparent colors.

> Source/WebCore/editing/FrameSelection.cpp:1793
> +    RefPtr<Element> parentElement = node ? node->parentElement() : nullptr;

Should be able to write just:

    RefPtr parentElement = ...

Given modern C++, the compiler can deduce RefPtr<Element> analogous to how it deduced Element* when we wrote auto*. I think. Could give it a try if you have any other reason to try this again.

Comment 43 Joone Hur 2021-08-25 19:18:20 PDT

Created attachment 436456 [details]
Patch

Comment 44 Joone Hur 2021-08-25 19:40:14 PDT

(In reply to Darin Adler from comment #42)
> Comment on attachment 436451 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436451&action=review
> 
> Don’t want to interfere with someone else doing a review+, but I have a few
> comments.
> 
> > Source/WebCore/ChangeLog:9
> > +        This bug was fixed in the below commit:
> > +        https://trac.webkit.org/changeset/152612/webkit
> 
> This is a confusing comment. When you say "this bug" you are referring to
> another bug, not this bug.

Okay, I will change the comment like  "The bug was fixed..."

> 
> > Source/WebCore/ChangeLog:13
> > +        This patch allows the caret to become visible in the black background
> > +        by getting the caret color from the element containing
> > +        the text, not the root editable element that has the contentEditable attribute.
> 
> Does this work well if there are non-opaque colors involved? I’m worried we
> don’t have enough test cases. It seems logically that we’d need to have a
> loop in the implementation to handle transparent colors.
> 

This is not a perfect solution. There are edge cases like image background or transparent color you mentioned.

I tried to consider the lightness of background. See this bug: https://bugs.webkit.org/show_bug.cgi?id=44862


> > Source/WebCore/editing/FrameSelection.cpp:1793
> > +    RefPtr<Element> parentElement = node ? node->parentElement() : nullptr;
> 
> Should be able to write just:
> 
>     RefPtr parentElement = ...
> 
> Given modern C++, the compiler can deduce RefPtr<Element> analogous to how
> it deduced Element* when we wrote auto*. I think. Could give it a try if you
> have any other reason to try this again.

OK.

Comment 45 Joone Hur 2021-08-25 19:58:07 PDT

Created attachment 436460 [details]
Patch

Comment 46 Joone Hur 2021-08-25 22:05:45 PDT

Created attachment 436471 [details]
Patch

Comment 47 Joone Hur 2021-08-26 00:38:55 PDT

Created attachment 436478 [details]
Patch

Comment 48 Joone Hur 2021-08-26 09:23:32 PDT

Created attachment 436523 [details]
Patch

Comment 49 Joone Hur 2021-08-26 12:20:08 PDT

Created attachment 436544 [details]
Patch

Comment 50 Darin Adler 2021-08-26 12:30:04 PDT

Comment on attachment 436544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436544&action=review

> Source/WebCore/ChangeLog:9
> +        The bug was fixed in the below commit:
> +        https://trac.webkit.org/changeset/152612/webkit

If we end up with another pass at this, I suggest this language:

    An earlier fix for caret color, which we are now improving, was this commit:
    https://trac.webkit.org/changeset/152612

But I wouldn’t ask for a whole new patch just to improve the change log.

Comment 51 Joone Hur 2021-08-26 12:55:55 PDT

(In reply to Darin Adler from comment #50)
> Comment on attachment 436544 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436544&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        The bug was fixed in the below commit:
> > +        https://trac.webkit.org/changeset/152612/webkit
> 
> If we end up with another pass at this, I suggest this language:
> 
>     An earlier fix for caret color, which we are now improving, was this
> commit:
>     https://trac.webkit.org/changeset/152612
> 
> But I wouldn’t ask for a whole new patch just to improve the change log.

OK, I will update the ChangeLog. Thank you!

Comment 52 Joone Hur 2021-08-26 12:58:42 PDT

Created attachment 436551 [details]
Updated ChangeLog

Comment 53 Joone Hur 2021-08-26 21:05:44 PDT

Created attachment 436609 [details]
Patch

Comment 54 EWS 2021-08-26 21:59:02 PDT

Committed r281685 (241035@main): <https://commits.webkit.org/241035@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436609 [details].

Comment 55 Radar WebKit Bug Importer 2021-08-26 22:00:26 PDT

<rdar://problem/82422299>