Text selection leaves visual artifacts behind when selected text has a different color
Created attachment 171747 [details] Patch
Created attachment 171818 [details] Patch rebased to get EWS data
Attachment 171818 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 LayoutTests/platform/chromium/TestExpectations:4147: Path does not exist. [test/expectations] [5] LayoutTests/platform/efl/TestExpectations:1685: Path does not exist. [test/expectations] [5] LayoutTests/platform/mac/TestExpectations:1369: Path does not exist. [test/expectations] [5] Total errors found: 3 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 171820 [details] Patch
Comment on attachment 171820 [details] Patch Attachment 171820 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14561601 New failing tests: fast/text/selection-painted-separately.html fast/css/shadow-multiple.html fast/text/justified-selection-at-edge.html editing/selection/3690703-2.html editing/selection/3690703.html editing/selection/extend-by-word-002.html editing/selection/3690719.html
Comment on attachment 171820 [details] Patch Attachment 171820 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14693529 New failing tests: fast/text/selection-painted-separately.html fast/css/shadow-multiple.html fast/text/justified-selection-at-edge.html editing/selection/3690703-2.html editing/selection/3690703.html editing/selection/extend-by-word-002.html editing/selection/3690719.html
Comment on attachment 171820 [details] Patch fast/text/selection-painted-separately.html seems to indicate a legitimate regression. This needs revisiting.
Created attachment 172321 [details] Patch
Attachment 172321 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 LayoutTests/platform/mac/TestExpectations:804: Path does not exist. [test/expectations] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 172321 [details] Patch Attachment 172321 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14680232 New failing tests: fast/css/shadow-multiple.html fast/text/justified-selection-at-edge.html editing/selection/3690703-2.html editing/selection/3690703.html editing/selection/extend-by-word-002.html editing/selection/3690719.html
Created attachment 173260 [details] Patch
Any opinions on this ? :(
Comment on attachment 173260 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173260&action=review > LayoutTests/editing/selection/selection-slanted-font-rendering.html:43 > +::selection { > +background: #bb2020; > +color: white; > +} We usually indent here as you would do for a WebCore function. > LayoutTests/editing/selection/selection-slanted-font-rendering.html:76 > +<p> Text within the selection rect should be painted in white, but not outside</p> This sentence is weird (especially the 2nd part). Also you don't really state the condition for passing explicitly: For this test to pass, the text without the selection should be white. It should be black outside without any trail. > LayoutTests/editing/selection/selection-slanted-font-rendering.html:81 > +<p>More manual testing below:</p> > +<a href="#" onclick="manualTest1()">Test 1</a> <a href="#" onclick="manualTest2()">Test 2</a> > +<h2 id="test-kern">TrueType AVAILABLE</h2> > +<h2 id="test-spacing">Grumpy Wizards make toxic brew for the Evil Queen and Jack</h2> Usually manual tests should go into ManualTests. I would advise you to split the test in 2 following that (better would be to make them a non-manual test, maybe using some SVG text property?)
leviw / rniwa would know this code, I don't unfortunately.
Comment on attachment 173260 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173260&action=review > Source/WebCore/rendering/InlineTextBox.cpp:-721 > - // FIXME: Truncate right-to-left text correctly. Are we doing right-to-left text clipping correctly now? I don't see any tests for it being added? > Source/WebCore/rendering/InlineTextBox.cpp:-736 > - if (!paintSelectedTextSeparately || ePos <= sPos) { > - // FIXME: Truncate right-to-left text correctly. Again, why are you removing this work-around for RTL text? > LayoutTests/editing/selection/selection-slanted-font-rendering.html:49 > + var s = window.getSelection(); Please don't use one-lettr variable like s. Spell out the word. > LayoutTests/editing/selection/selection-slanted-font-rendering.html:52 > + var r = document.createRange(); Ditto. Also wrong indentation. > LayoutTests/editing/selection/selection-slanted-font-rendering.html:56 > + r.setStart(target.firstChild, start); > + r.setEnd(target.firstChild, end); > + s.removeAllRanges(); > + s.addRange(r); You can replace all of this code by getSelection().setBaseAndExtent(target.firstChild, start, target.firstChild, end); > LayoutTests/editing/selection/selection-slanted-font-rendering.html:60 > + var length = target.innerHTML.length; It doesn't make sense to use innerHTML here because your offset isn't going to be an offset in the markup. Use testContent instead.
Comment on attachment 173260 [details] Patch I don't think this patch is laudable as is.
Comment on attachment 173260 [details] Patch Alright, I tried cleaning up that messy layout test. View in context: https://bugs.webkit.org/attachment.cgi?id=173260&action=review >> Source/WebCore/rendering/InlineTextBox.cpp:-721 >> - // FIXME: Truncate right-to-left text correctly. > > Are we doing right-to-left text clipping correctly now? I don't see any tests for it being added? I'm not sure what was incorrect about it before. My understanding was that this FIXME meant "we should get truncation working properly for RTL". I strongly believe this truncation trick only renders correctly in particular cases, and that in general, performing selection with a different text color can only be correct by clipping (and yes, it'll be slow). Now if I overlooked a reason for that FIXME to stay, I'll happily revive it. >> Source/WebCore/rendering/InlineTextBox.cpp:-736 >> - // FIXME: Truncate right-to-left text correctly. > > Again, why are you removing this work-around for RTL text? I'm not removing the workaround per se, rather making it the norm, as I don't see how we could get away with drawing only the selected glyphs and get the right result. As I said above, I don't mind leaving the FIXMEs, but right now I think they would be really confusing, as they seem to refer to an "optimization" that would go away with this patch.
Created attachment 190268 [details] Patch
Attachment 190268 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/editing/selection/selection-slanted-font-rendering.html', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/qt/editing/selection/selection-slanted-font-rendering-expected.png', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/InlineTextBox.cpp']" exit_code: 1 LayoutTests/platform/qt/editing/selection/selection-slanted-font-rendering-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 190268 [details] Patch Attachment 190268 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16775244 New failing tests: fast/ruby/select-ruby.html
Created attachment 190494 [details] Patch Add the fairly recent select-ruby to the list of tests needing rebaseline. The style bot could probably use http://wkb.ug/107724.
Created attachment 190732 [details] Patch
Attachment 190732 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/editing/selection/selection-slanted-font-rendering.html', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/qt/editing/selection/selection-slanted-font-rendering-expected.png', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/InlineTextBox.cpp']" exit_code: 1 LayoutTests/platform/qt/editing/selection/selection-slanted-font-rendering-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 190732 [details] Patch Could the expected result be made in HTML instead as a reference test?
(In reply to comment #24) > (From update of attachment 190732 [details]) > Could the expected result be made in HTML instead as a reference test? I had tried the reftest approach back then and it gave me the worst headaches. IIRC I gave up because it essentially meant testing that codepath against itself, which seemed pretty pointless.
Comment on attachment 190732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190732&action=review On my second look, the code change looks right but I'd like to see more rationals for each one of your changes since this code is rarely tested. Also, could you add a test for RTL case? Maybe some Hebrew text? > Source/WebCore/ChangeLog:10 > + since there is no guarantee that a given glyph will fit entirely in the selection rect, the text might then > + look 'broken' in places after it has been incrementally deselected. In that case, it seems clipping is in order. This sentence will read more naturally if the clause starting with "since" had been placed at the end. > Source/WebCore/ChangeLog:16 > + (WebCore::InlineTextBox::paint): clip to the selection rect to paint selected text, then to its complement > + to repaint the deselected part. I would like to see more explanation on the code change. Namely, we should explain why we're now replacing sPos and ePos with 0 and length. > Source/WebCore/rendering/InlineTextBox.cpp:-738 > - // FIXME: Truncate right-to-left text correctly. We should explain why we're removing FIXME. > Source/WebCore/rendering/InlineTextBox.cpp:831 > + ePos = max(min(endPos - m_start, (int)m_len), 0); While we're at, can we use static_cast? > LayoutTests/platform/efl/TestExpectations:677 > +# New test, plus rebaselines needed after bug 100904 > +webkit.org/b/100904 editing/selection/selection-slanted-font-rendering.html [ Missing ImageOnlyFailure ] > +webkit.org/b/100904 editing/selection/3690703.html [ ImageOnlyFailure ] > +webkit.org/b/100904 editing/selection/3690703-2.html [ ImageOnlyFailure ] > +webkit.org/b/100904 editing/selection/3690719.html [ ImageOnlyFailure ] > +webkit.org/b/100904 editing/selection/extend-by-word-002.html [ ImageOnlyFailure ] > +webkit.org/b/100904 fast/css/shadow-multiple.html [ ImageOnlyFailure ] > +webkit.org/b/100904 fast/text/justified-selection-at-edge.html [ ImageOnlyFailure ] > +webkit.org/b/100904 fast/ruby/select-ruby.html [ ImageOnlyFailure ] > + Non-chromium ports don't run pixel tests. Please don't add these entries. Also, please be sure to rebaseline tests or ask chromium sheriffs to do so once the patch is landed.
By the way, please feel free to ping me on IRC or email me if you needed a review :) I have a lot of emails coming in each day (2-300 of them), and I try but often forget to follow up on some bugs.
Created attachment 196482 [details] Patch
Comment on attachment 196482 [details] Patch Just trying to get the Chromium EWS to spit out new pixel expectations for now.
Comment on attachment 196482 [details] Patch Attachment 196482 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17446021 New failing tests: editing/selection/selection-slanted-font-rendering.html editing/selection/rtl-selection-slanted-font-rendering.html
Created attachment 196488 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Comment on attachment 196482 [details] Patch Attachment 196482 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17441108 New failing tests: editing/selection/selection-slanted-font-rendering.html editing/selection/rtl-selection-slanted-font-rendering.html
Created attachment 196494 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Comment on attachment 196482 [details] Patch Attachment 196482 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17513046 New failing tests: editing/selection/selection-slanted-font-rendering.html fast/css/shadow-multiple.html editing/selection/rtl-selection-slanted-font-rendering.html fast/text/justified-selection-at-edge.html editing/selection/3690703-2.html editing/selection/3690703.html editing/selection/extend-by-word-002.html fast/ruby/select-ruby.html editing/selection/3690719.html
Created attachment 196496 [details] Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment on attachment 196482 [details] Patch Attachment 196482 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17440042 New failing tests: editing/selection/selection-slanted-font-rendering.html fast/css/shadow-multiple.html editing/selection/rtl-selection-slanted-font-rendering.html fast/text/justified-selection-at-edge.html editing/selection/3690703-2.html editing/selection/3690703.html editing/selection/extend-by-word-002.html fast/ruby/select-ruby.html editing/selection/3690719.html
Created attachment 196502 [details] Archive of layout-test-results from gce-cr-linux-01 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Created attachment 196653 [details] Patch
Comment on attachment 196653 [details] Patch Attachment 196653 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/37020 New failing tests: editing/selection/selection-slanted-font-rendering.html css3/filters/custom/filter-fallback-to-software.html editing/selection/rtl-selection-slanted-font-rendering.html
Created attachment 197467 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Comment on attachment 196653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196653&action=review > Source/WebCore/rendering/InlineTextBox.cpp:723 > + selectionClip = font.selectionRectForText(textRun, boxOrigin, selectionHeight(), sPos, ePos); Adding this selectionRect call whenever there is a selection could potentially be expensive. Do we know if there is a performance regression here?
(In reply to comment #41) > (From update of attachment 196653 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=196653&action=review > > > Source/WebCore/rendering/InlineTextBox.cpp:723 > > + selectionClip = font.selectionRectForText(textRun, boxOrigin, selectionHeight(), sPos, ePos); > > Adding this selectionRect call whenever there is a selection could potentially be expensive. Do we know if there is a performance regression here? Thanks for reminding me to re-visit this to get proper pixel results. I wanted to do that a while back and then somehow forgot about it. I don't think you can call it a performance regression if the rendering before the fix is arguably not correct. Besides, this only applies to selections with reversed color, so it should be fairly limited (on Mac at least, where the standard palette options don't usually require this).
This code has since moved to Source/WebCore/rendering/TextDecorationPainter.cpp. You may want to update your patch.
Comment on attachment 196653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196653&action=review > Source/WebCore/rendering/InlineTextBox.cpp:733 > + context->clipOut(clipOut); I don't understand how this works. Currently, our graphics context doesn't have the ability to "un-clip" without pushing/popping state, but I don't see a place where we push/pop state. How does this work?
Comment on attachment 196653 [details] Patch r- to clear stale patches from the review queue