Bug 98876 - [Qt] REGRESSION (r130851): fast/text/word-space-with-kerning.html fails
Summary: [Qt] REGRESSION (r130851): fast/text/word-space-with-kerning.html fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks: 79666 98317
  Show dependency treegraph
 
Reported: 2012-10-10 01:02 PDT by Zoltan Arvai
Modified: 2012-10-23 05:12 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.41 KB, patch)
2012-10-23 03:04 PDT, Allan Sandfeld Jensen
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Arvai 2012-10-10 01:02:29 PDT
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
Comment 1 Zoltan Arvai 2012-10-10 01:13:02 PDT
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
Comment 2 Zoltan Arvai 2012-10-10 01:36:15 PDT
Skipped on Qt in https://trac.webkit.org/changeset/130873.
Please unskip it with the proper fix.
Comment 3 Allan Sandfeld Jensen 2012-10-23 02:39:18 PDT
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.
Comment 4 Allan Sandfeld Jensen 2012-10-23 03:04:27 PDT
Created attachment 170104 [details]
Patch
Comment 5 Allan Sandfeld Jensen 2012-10-23 03:11:38 PDT
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.
Comment 6 Simon Hausmann 2012-10-23 04:21:09 PDT
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?
Comment 7 Allan Sandfeld Jensen 2012-10-23 04:52:56 PDT
(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.
Comment 8 Simon Hausmann 2012-10-23 04:59:01 PDT
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.
Comment 9 Allan Sandfeld Jensen 2012-10-23 05:12:14 PDT
Committed r132205: <http://trac.webkit.org/changeset/132205>