Bug 117493

Summary: Caret should respect text background color
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: 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: P2 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

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
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Patch (1.96 KB, patch)
2021-02-22 01:11 PST, Joone Hur 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Patch (49.72 KB, patch)
2021-03-09 00:06 PST, Joone Hur 		no flags
DetailsFormatted DiffDiff
Patch (50.26 KB, patch)
2021-03-09 09:45 PST, Joone Hur 		no flags
DetailsFormatted DiffDiff
Patch (50.23 KB, patch)
2021-08-24 01:54 PDT, Joone Hur 		no flags
DetailsFormatted DiffDiff
Patch (50.24 KB, patch)
2021-08-24 17:02 PDT, Joone Hur 		no flags
DetailsFormatted DiffDiff
Patch (49.60 KB, patch)
2021-08-25 17:24 PDT, Joone Hur 		no flags
DetailsFormatted DiffDiff
Patch (49.47 KB, patch)
2021-08-25 18:06 PDT, Joone Hur 		no flags
DetailsFormatted DiffDiff
Patch (50.90 KB, patch)
2021-08-25 19:18 PDT, Joone Hur 		no flags
DetailsFormatted DiffDiff
Patch (50.89 KB, patch)
2021-08-25 19:58 PDT, Joone Hur 		no flags
DetailsFormatted DiffDiff
Patch (51.51 KB, patch)
2021-08-25 22:05 PDT, Joone Hur 		no flags
DetailsFormatted DiffDiff
Patch (52.20 KB, patch)
2021-08-26 00:38 PDT, Joone Hur 		no flags
DetailsFormatted DiffDiff
Patch (123.52 KB, patch)
2021-08-26 09:23 PDT, Joone Hur 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (126.25 KB, patch)
2021-08-26 12:20 PDT, Joone Hur 		no flags
DetailsFormatted DiffDiff
Updated ChangeLog (126.29 KB, patch)
2021-08-26 12:58 PDT, Joone Hur 		no flags
DetailsFormatted DiffDiff
Patch (126.29 KB, patch)
2021-08-26 21:05 PDT, Joone Hur 		no flags
DetailsFormatted DiffDiff
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.