Bug 135739 – Selection painting is incorrect when text has non-default word-spacing value.

• Component: Layout and Rendering
• Status: RESOLVED CONFIGURATION CHANGED

[alan]

Created attachment 236257 [details]
Test reduction

see attached test reduction.

[alan]

Created attachment 236258 [details]
Work in progress patch for performance evaluation.

[alan]

Comment on attachment 236258 [details]
Work in progress patch for performance evaluation.

ignore this. there must be a better way to do this as we already compute word width spacing included.

[alan]

Created attachment 236313 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 236313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236313&action=review

> LayoutTests/fast/inline/text-selection-with-wordspacing-expected.html:18
> +  <div></div>

What? Where are all the "f o ba r"s?

[alan]

(In reply to comment #4)
> (From update of attachment 236313 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236313&action=review
> 
> > LayoutTests/fast/inline/text-selection-with-wordspacing-expected.html:18
> > +  <div></div>
> 
> What? Where are all the "f o ba r"s?

color: transparent;

[Myles C. Maxfield]

r=me

[Ryosuke Niwa]

Comment on attachment 236313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236313&action=review

Your change log description and the code change doesn't really match up.

> Source/WebCore/ChangeLog:8
> +        This patch applies word-spacing value on the last cluster of the measured text regardless

What are you referring to by "cluster"?  Some grapheme clusters?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:481
> +    if (!lineBox->fitsToGlyphs() && renderer->canUseSimpleFontCodePath()) {

Presumably we were using the value for a wrong renderer?
Could you elaborate as to when that happens?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:497
> -                if (i > 0 && wordLength == 1 && (c == ' ' || c == '\t'))
> +                if (i > 0 && (c == ' ' || c == '\t'))

Could you explain why you're removing wordLength == 1 here?
By "regardless of cluster's width" in the change log, did you mean "regardless of cluster's word length"?

[alan]

(In reply to comment #7)
> (From update of attachment 236313 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236313&action=review
> 
> Your change log description and the code change doesn't really match up.
> 
> > Source/WebCore/ChangeLog:8
> > +        This patch applies word-spacing value on the last cluster of the measured text regardless
> 
> What are you referring to by "cluster"?  Some grapheme clusters?
No, these are just simple text segments separated by spaces/tabs. When Enrica was explaining the logic to me here, she was using the term 'text clusters' and I assumed it is a known (used) phrase in this part of the code. I'll change it something more descriptive (like text segment, see more explanation below.)

> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:481
> > +    if (!lineBox->fitsToGlyphs() && renderer->canUseSimpleFontCodePath()) {
> 
> Presumably we were using the value for a wrong renderer?
> Could you elaborate as to when that happens?
This part of the changeset is a simple cleanup. I just moved the expression around.

> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:497
> > -                if (i > 0 && wordLength == 1 && (c == ' ' || c == '\t'))
> > +                if (i > 0 && (c == ' ' || c == '\t'))
> 
> Could you explain why you're removing wordLength == 1 here?
With the wordLength == 1 check, we only apply the word spacing when there's nothing in the text segment but either a space or a tab character. -> <span>a </span> 
segment 1 -> 'a'
segment 2 -> ' '
However when the text segment has more than just a space, we fail to apply word spacing and the inline text box's size gets invalid.
<span>a foo</span>
segment 1 -> 'a'
segment 2 -> ' foo'

> By "regardless of cluster's width" in the change log, did you mean "regardless of cluster's word length"?
Word length is not the right term here is the text segment includes not only the word, but also the leading space/tab.

[alan]

Created attachment 236423 [details]
Patch

[alan]

With forcing kerning on, LayoutTests progresses on the following 3 test cases (with the fix):
fast/css/word-spacing-characters.html
fast/text/basic/004.html
fast/text/whitespace/span-in-word-space-causes-overflow.html

(they all fail when you load them in Safari with trunk WebKit. However since kerning is disabled for LayoutTests, they all pass.)

Comment on attachment 236423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236423&action=review

> Source/WebCore/ChangeLog:16
> +        Test: fast/inline/text-selection-with-wordspacing.html

Since the bug is only in a kerningIsEnabled branch of the code, and the test doesn’t turn on kerning with CSS, and the test harnesses disable kerning by default, how does the test work?

[alan]

(In reply to comment #11)
> (From update of attachment 236423 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236423&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +        Test: fast/inline/text-selection-with-wordspacing.html
> 
> Since the bug is only in a kerningIsEnabled branch of the code, and the test doesn’t turn on kerning with CSS, and the test harnesses disable kerning by default, how does the test work?
Good question. It definitely failed when I tested on my local build - but let me check it again. Maybe it failed because of some other, unrelated issue.

[alan]

Created attachment 236426 [details]
Patch

[alan]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 236423 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=236423&action=review
> > 
> > > Source/WebCore/ChangeLog:16
> > > +        Test: fast/inline/text-selection-with-wordspacing.html
> > 
> > Since the bug is only in a kerningIsEnabled branch of the code, and the test doesn’t turn on kerning with CSS, and the test harnesses disable kerning by default, how does the test work?
> Good question. It definitely failed when I tested on my local build - but let me check it again. Maybe it failed because of some other, unrelated issue.
It must have been something unrelated. -ofc.

[Ryosuke Niwa]

Comment on attachment 236423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236423&action=review

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:-500
> -                if (i > 0 && wordLength == 1 && (c == ' ' || c == '\t'))

Do we know the original patch that added this wordLength == 1?
This check looks quite intentional.

Also, did you check to see if we don't support this configuration in the fast text path Antti added last year?
It would be great to note that in the change log if you've checked it already.

[Darin Adler]

Comment on attachment 236426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236426&action=review

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:497
> +                if (i > 0 && (c == ' ' || c == '\t'))

Why only space and tab? What about newline? I think a correct implementation of this needs to call RenderStyle::preserveNewline. Should add a test case that uses a newline to break words. Also might want test cases for other whitespace modes.

Comment on attachment 236426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236426&action=review

> LayoutTests/fast/inline/text-selection-with-wordspacing.html:17
> +    text-rendering: geometricPrecision;

It might be better to use “-webkit-font-kerning: normal” here, because the meaning of “text-rendering: geometricPrecision” is more fuzzy.

[alan]

(In reply to comment #15)
> (From update of attachment 236423 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236423&action=review
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:-500
> > -                if (i > 0 && wordLength == 1 && (c == ' ' || c == '\t'))
> 
> Do we know the original patch that added this wordLength == 1?
> This check looks quite intentional.
As explained in the changelog, It was a fix for <spa>foo </span> case where the last segment has only one whitespace character.  

> 
> Also, did you check to see if we don't support this configuration in the fast text path Antti added last year?
> It would be great to note that in the change log if you've checked it already.
Yes I did check it. Simple line layout bails out when it sees word spacing.

[Ahmad Saleem]

It seems that this r+ patch didn't landed by checking bug ID on Webkit GitHub.

This patch is modifying this:

Remove -> https://github.com/WebKit/WebKit/blob/89d57a6f366a8abe5d012e38f160f81388b46429/Source/WebCore/rendering/LegacyLineLayout.cpp#L486

Changing canUseSimpleFontCodePath to canUseSimpleFontCodePath() - https://github.com/WebKit/WebKit/blob/89d57a6f366a8abe5d012e38f160f81388b46429/Source/WebCore/rendering/LegacyLineLayout.cpp#L490

This is changed now:

https://github.com/WebKit/WebKit/blob/89d57a6f366a8abe5d012e38f160f81388b46429/Source/WebCore/rendering/LegacyLineLayout.cpp#L508

I am not sure whether it is fixed or not but just wanted to check and update. Thanks!

[Ahmad Saleem]

The function `setLogicalWidthForTextRun` is gone with:

https://github.com/WebKit/WebKit/commit/ff763d9dead40a6ce11cc71d1aed464dbc0045f9

@Alan / @Antti - is this still applicable?

[alan]

Not it is not.