Bug 98876 – [Qt] REGRESSION (r130851): fast/text/word-space-with-kerning.html fails

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Zoltan Arvai]

fast/text/word-space-with-kerning.html looks differently from reference html after r130851:
http://build.webkit.org/results/Qt%20Linux%20Release/r130865%20%2853124%29/results.html

[Zoltan Arvai]

An other bot that stores results for longer time:
http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r130851%20%2843632%29/results.html

[Zoltan Arvai]

Skipped on Qt in https://trac.webkit.org/changeset/130873.
Please unskip it with the proper fix.

[Allan Sandfeld Jensen]

It seems the problem is that our version of Font::floatWidthForComplexText, when used on a string such as " b ", applies word-spacing both before and after the word, while the layout assumes we only apply word-spacing once.

[Allan Sandfeld Jensen]

Created attachment 170104 [details]
Patch

[Allan Sandfeld Jensen]

Btw, quick note. The problem is only related to kerning because kerning enables complex font-path. The problem happens in many more similar cases if we disable simple font-path completely and always choose complex. 

This means more tests are likely to have been influence by this bug if they caused complex font-path due to some other mechanism.

[Simon Hausmann]

Comment on attachment 170104 [details]
Patch

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

> Source/WebCore/platform/graphics/qt/FontQt.cpp:216
> +    // RenderBlockLineLayout expects us to only add word-spacing for trailing spaces, not for leading spaces.
> +    if (treatAsSpace(run[0]))
> +        return width + run.expansion() - m_wordSpacing;

Where do you find that logic in the simple text code path?

[Allan Sandfeld Jensen]

(In reply to comment #6)
> (From update of attachment 170104 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170104&action=review
> 
> > Source/WebCore/platform/graphics/qt/FontQt.cpp:216
> > +    // RenderBlockLineLayout expects us to only add word-spacing for trailing spaces, not for leading spaces.
> > +    if (treatAsSpace(run[0]))
> > +        return width + run.expansion() - m_wordSpacing;
> 
> Where do you find that logic in the simple text code path?

I didn't find it, I just instrumented the code to call both and dumped the results.

"A" was 17px wide by both
" A" was 83px wide by complex, and 23px by simple
" A " was 149px wide by complex, and 89px by simple.

[Simon Hausmann]

Comment on attachment 170104 [details]
Patch

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

>>> Source/WebCore/platform/graphics/qt/FontQt.cpp:216
>>> +        return width + run.expansion() - m_wordSpacing;
>> 
>> Where do you find that logic in the simple text code path?
> 
> I didn't find it, I just instrumented the code to call both and dumped the results.
> 
> "A" was 17px wide by both
> " A" was 83px wide by complex, and 23px by simple
> " A " was 149px wide by complex, and 89px by simple.

Indeed, that logic existed before r113968 and in that revision was moved into FontQt4.cpp, but it's missing here.

I personally would prefer if it was written like before also:

    if (treatAsSpace(run[0]))
        width -= m_wordSpacing

and then share the return:

    return width + run.expansion();

But the change seems correct.

[Allan Sandfeld Jensen]

Committed r132205: <http://trac.webkit.org/changeset/132205>