117493 – Caret should respect text background color

RESOLVED FIXED117493

Caret should respect text background color

https://bugs.webkit.org/show_bug.cgi?id=117493

Summary Caret should respect text background color

Ryosuke Niwa

Reported 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);

Attachments
Patch (5.57 KB, patch) 2013-06-26 01:00 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (519.87 KB, application/zip) 2013-06-26 05:41 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (521.31 KB, application/zip) 2013-06-26 08:11 PDT, Build Bot	no flags	Details
Archive of layout-test-results from APPLE-EWS-6 for win-future (842.70 KB, application/zip) 2013-06-26 09:14 PDT, Build Bot	no flags	Details
Patch (19.80 KB, patch) 2013-06-28 01:44 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (613.90 KB, application/zip) 2013-06-28 02:48 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (666.50 KB, application/zip) 2013-06-28 03:24 PDT, Build Bot	no flags	Details
Archive of layout-test-results from APPLE-EWS-3 for win-future (858.32 KB, application/zip) 2013-06-29 08:13 PDT, Build Bot	no flags	Details
Archive of layout-test-results from APPLE-EWS-2 for win-future (858.31 KB, application/zip) 2013-06-29 10:16 PDT, Build Bot	no flags	Details
Patch (20.59 KB, patch) 2013-07-09 23:06 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Patch (1.96 KB, patch) 2021-02-22 01:11 PST, Joone Hur	no flags	Details Formatted Diff Diff
caret color test (1.77 KB, text/html) 2021-02-22 01:14 PST, Joone Hur	no flags	Details
Patch (3.08 KB, patch) 2021-02-22 02:03 PST, Joone Hur	no flags	Details Formatted Diff Diff
Patch (49.72 KB, patch) 2021-03-09 00:06 PST, Joone Hur	no flags	Details Formatted Diff Diff
Patch (50.26 KB, patch) 2021-03-09 09:45 PST, Joone Hur	no flags	Details Formatted Diff Diff
Patch (50.23 KB, patch) 2021-08-24 01:54 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Patch (50.24 KB, patch) 2021-08-24 17:02 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Patch (49.60 KB, patch) 2021-08-25 17:24 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Patch (49.47 KB, patch) 2021-08-25 18:06 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Patch (50.90 KB, patch) 2021-08-25 19:18 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Patch (50.89 KB, patch) 2021-08-25 19:58 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Patch (51.51 KB, patch) 2021-08-25 22:05 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Patch (52.20 KB, patch) 2021-08-26 00:38 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Patch (123.52 KB, patch) 2021-08-26 09:23 PDT, Joone Hur	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (126.25 KB, patch) 2021-08-26 12:20 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Updated ChangeLog (126.29 KB, patch) 2021-08-26 12:58 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Patch (126.29 KB, patch) 2021-08-26 21:05 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Show Obsolete (17) View All Add attachment proposed patch, testcase, etc.

Joone Hur

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

Hi Ryosoke, I made this patch so I will prepare the patch for WebKit. Thanks, Joone

Joone Hur

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

(In reply to comment #1) > Hi Ryosoke, Ryosuke, I'm sorry that I misspelled your name. :-)

Joone Hur

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

Created attachment 205460 [details] Patch

Build Bot

Comment 4 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

Build Bot

Comment 5 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

Build Bot

Comment 6 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

Build Bot

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

Build Bot

Comment 8 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

Build Bot

Comment 9 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

Ryosuke Niwa

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

Joone Hur

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

Joone Hur

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

Created attachment 205678 [details] Patch

Build Bot

Comment 13 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

Build Bot

Comment 14 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

Build Bot

Comment 15 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

Build Bot

Comment 16 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

Build Bot

Comment 17 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

Build Bot

Comment 18 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

Build Bot

Comment 19 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

Build Bot

Comment 20 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

Joone Hur

Comment 21 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

WebKit Commit Bot

Comment 22 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>

WebKit Commit Bot

Comment 23 2013-07-13 23:03:43 PDT

All reviewed patches have been landed. Closing bug.

Benjamin Poulain

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

Don't forget to rebaseline.

Joone Hur

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

(In reply to comment #24) > Don't forget to rebaseline. http://trac.webkit.org/changeset/152987 Thanks!

Joone Hur

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

Reopening to attach new patch.

Joone Hur

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

Created attachment 421168 [details] Patch

Joone Hur

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

Created attachment 421169 [details] caret color test

Joone Hur

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

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

Joone Hur

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

Created attachment 421173 [details] Patch

Ryosuke Niwa

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

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

Joone Hur

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

Sure, I will fix them. Thanks!

Joone Hur

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

Created attachment 422670 [details] Patch

Joone Hur

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

Created attachment 422719 [details] Patch

Joone Hur

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

Created attachment 436267 [details] Patch

Ryosuke Niwa

Comment 36 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

Joone Hur

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

Created attachment 436348 [details] Patch

Ryosuke Niwa

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

Joone Hur

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

Created attachment 436446 [details] Patch

Joone Hur

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

Created attachment 436451 [details] Patch

Joone Hur

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

Darin Adler

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

Joone Hur

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

Created attachment 436456 [details] Patch

Joone Hur

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

Joone Hur

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

Created attachment 436460 [details] Patch

Joone Hur

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

Created attachment 436471 [details] Patch

Joone Hur

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

Created attachment 436478 [details] Patch

Joone Hur

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

Created attachment 436523 [details] Patch

Joone Hur

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

Created attachment 436544 [details] Patch

Darin Adler

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

Joone Hur

Comment 51 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!

Joone Hur

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

Created attachment 436551 [details] Updated ChangeLog

Joone Hur

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

Created attachment 436609 [details] Patch

EWS

Comment 54 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].

Radar WebKit Bug Importer

Comment 55 2021-08-26 22:00:26 PDT

<rdar://problem/82422299>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component HTML Editing

Assignee

Joone Hur

Reported

2013-06-10 23:01 PDT

Modified

2021-08-28 15:12 PDT History

CC List

11 users Show

URL

Keywords BlinkMergeCandidate, InRadar

Depends on

Blocks

44862

Dependencies

tree graph