Bug 100904 – Text selection leaves visual artifacts behind when selected text has a different color

• Status: ASSIGNED
• Component: Text

[Pierre Rossi]

Text selection leaves visual artifacts behind when selected text has a different color

[Pierre Rossi]

Created attachment 171747 [details]
Patch

[Pierre Rossi]

Created attachment 171818 [details]
Patch

rebased to get EWS data

[WebKit Review Bot]

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.

[Pierre Rossi]

Created attachment 171820 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Pierre Rossi]

Comment on attachment 171820 [details]
Patch

fast/text/selection-painted-separately.html seems to indicate a legitimate regression. This needs revisiting.

[Pierre Rossi]

Created attachment 172321 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Pierre Rossi]

Created attachment 173260 [details]
Patch

[Pierre Rossi]

Any opinions on this ? :(

[Julien Chaffraix]

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?)

[Julien Chaffraix]

leviw / rniwa would know this code, I don't unfortunately.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Comment on attachment 173260 [details]
Patch

I don't think this patch is laudable as is.

[Pierre Rossi]

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.

[Pierre Rossi]

Created attachment 190268 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Pierre Rossi]

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.

[Pierre Rossi]

Created attachment 190732 [details]
Patch

[WebKit Review Bot]

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.

[Allan Sandfeld Jensen]

Comment on attachment 190732 [details]
Patch

Could the expected result be made in HTML instead as a reference test?

[Pierre Rossi]

(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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Pierre Rossi]

Created attachment 196482 [details]
Patch

[Pierre Rossi]

Comment on attachment 196482 [details]
Patch

Just trying to get the Chromium EWS to spit out new pixel expectations for now.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Pierre Rossi]

Created attachment 196653 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Myles C. Maxfield]

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?

[Pierre Rossi]

(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).

[Myles C. Maxfield]

This code has since moved to Source/WebCore/rendering/TextDecorationPainter.cpp. You may want to update your patch.

[Myles C. Maxfield]

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?

[Brady Eidson]

Comment on attachment 196653 [details]
Patch

r- to clear stale patches from the review queue