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);
Hi Ryosoke, I made this patch so I will prepare the patch for WebKit. Thanks, Joone
(In reply to comment #1) > Hi Ryosoke, Ryosuke, I'm sorry that I misspelled your name. :-)
Created attachment 205460 [details] Patch
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
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 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
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 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
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 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.
(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.
Created attachment 205678 [details] Patch
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
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 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
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 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
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 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
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
Created attachment 206369 [details] Patch This patch allows to skip the caret color test case on the Mac port
Comment on attachment 206369 [details] Patch Clearing flags on attachment: 206369 Committed r152612: <http://trac.webkit.org/changeset/152612>
All reviewed patches have been landed. Closing bug.
Don't forget to rebaseline.
(In reply to comment #24) > Don't forget to rebaseline. http://trac.webkit.org/changeset/152987 Thanks!
Reopening to attach new patch.
Created attachment 421168 [details] Patch
Created attachment 421169 [details] caret color test
The caret is not visible in the first line in the test file attached.
Created attachment 421173 [details] Patch
Looks like the tests are failing. We need to fix that.
Sure, I will fix them. Thanks!
Created attachment 422670 [details] Patch
Created attachment 422719 [details] Patch
Created attachment 436267 [details] Patch
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
Created attachment 436348 [details] Patch
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.
Created attachment 436446 [details] Patch
Created attachment 436451 [details] Patch
LayoutTests/editing/caret/caret-color.html is the original test case I added for this bug so I just updated it.
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.
Created attachment 436456 [details] Patch
(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.
Created attachment 436460 [details] Patch
Created attachment 436471 [details] Patch
Created attachment 436478 [details] Patch
Created attachment 436523 [details] Patch
Created attachment 436544 [details] Patch
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.
(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!
Created attachment 436551 [details] Updated ChangeLog
Created attachment 436609 [details] Patch
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].
<rdar://problem/82422299>