Bug 138348 – Some words are placed on top of each other in complex text layout

• Status: REOPENED
• Component: New Bugs

[Myles C. Maxfield]

[OSX] Some words are placed on top of each other in complex text layout

[Myles C. Maxfield]

Created attachment 240909 [details]
Patch

[Build Bot]

Created attachment 240911 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Created attachment 240912 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Myles C. Maxfield]

<rdar://problem/18797255>

[Myles C. Maxfield]

Created attachment 240934 [details]
Updated for older OS X

[Myles C. Maxfield]

Created attachment 240935 [details]
Updated for older OS X

Comment on attachment 240935 [details]
Updated for older OS X

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

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:620
> +            CGSize advance = advances[i];

Last time we tried something like this was in http://trac.webkit.org/r129176, and we had to revert it in http://trac.webkit.org/r129389. Is this different?

[Myles C. Maxfield]

I'm not going to submit this until I make sure that line breaking and max width works correctly (or fix them)

[Myles C. Maxfield]

1. I found some problems with width calculations, but these problems are present both with and without this patch. Fixing these seems to be a part of https://bugs.webkit.org/show_bug.cgi?id=138213

2. I looked at the code which calculates max-width and line breaking, and couldn't find anything wrong from inspection.

I think the combination of these two things is enough to land this, keeping in mind that we can easily roll it out if we discover a reason to. Also, if this occurs, I'll create a test showing the incorrect behavior.

[Myles C. Maxfield]

http://trac.webkit.org/changeset/175813

[Myles C. Maxfield]

<rdar://problem/18928367>

(In reply to comment #9)
> 1. I found some problems with width calculations, but these problems are
> present both with and without this patch. Fixing these seems to be a part of
> https://bugs.webkit.org/show_bug.cgi?id=138213
> 
> 2. I looked at the code which calculates max-width and line breaking, and
> couldn't find anything wrong from inspection.
> 
> I think the combination of these two things is enough to land this, keeping
> in mind that we can easily roll it out if we discover a reason to. Also, if
> this occurs, I'll create a test showing the incorrect behavior.

There are actually two problems with the patch you landed, and this test case demonstrates both:

<style>
    div { font-size: 36px; font-family: times; border-right: solid black; float: left; clear: both; }
</style>
<div>
    Woe&#x0300; Woe Woe Woe Woe Woe Woe Woe Woe Woe Woe Woe Woe Woe
</div>
<div>
    Wo&#xe8; Woe Woe Woe Woe Woe Woe Woe Woe Woe Woe Woe Woe Woe
</div>

1. The patch introduced a discrepancy between the complex code path and the simple code path. We need to maintain the invariant that whenever something that can be rendered with the simple path is rendered with the complex path, it remains the same.
2. The patch introduced a mismatch in the complex code path between actual width and computed max-width.

I think you should revert your change for now.

[Simon Fraser (smfr)]

(In reply to comment #12)
> 1. The patch introduced a discrepancy between the complex code path and the
> simple code path. We need to maintain the invariant that whenever something
> that can be rendered with the simple path is rendered with the complex path,
> it remains the same.
> 2. The patch introduced a mismatch in the complex code path between actual
> width and computed max-width.

Are there no tests that detect these issues?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 138573

[Myles C. Maxfield]

This is because RenderText::computePreferredLogicalWidths() is completely broken. When this function tries to calculate the width of “word1 word2“ it first calculates the width of a space by itself, and then will add up the following measurements:
“word1 “ minus the width of the space alone
“ “
“word2“

Similarly, stripTrailingSpace() subtracts a space width from the total width, which is not correct (since the space width is measured in isolation)

[Myles C. Maxfield]

https://bugs.webkit.org/show_bug.cgi?id=138581

[Myles C. Maxfield]

Created attachment 252430 [details]
WIP

[Myles C. Maxfield]

Created attachment 252431 [details]
Reduction

[Myles C. Maxfield]

In addition to RenderText::computePreferredLogicalWidths(), TextFragmentIterator in SimpleLineLayout must be updated.

[Myles C. Maxfield]

TextFragmentIterator:

spaceWidth(font.width(TextRun(&space, 1)))

if (startPosition < nextPosition)
        width = m_style.spaceWidth;

[Myles C. Maxfield]

BreakingContext::handleText() also needs to be updated.

[Myles C. Maxfield]

Man, the code inside RenderText::computePreferredLogicalWidths() and BreakingContext::handleText() really have to be unified some day.

[Myles C. Maxfield]

BreakingContext::handleText() actually already includes 1 character of prior whitespace

[Myles C. Maxfield]

Created attachment 252570 [details]
WIP

[Myles C. Maxfield]

(In reply to comment #23)
> BreakingContext::handleText() actually already includes 1 character of prior
> whitespace

... except if the previous run of spaces is longer than 1 character.

We try to include a single prior space in every word. However, if there are two spaces in a row, we will add the leftmost space alone, and ignore spaces from here on until we hit a non-space. The next word we hit, we will not include a prior space (but we'll pick up doing this afterwards)

[Myles C. Maxfield]

RenderText::computePreferredLogicalWidths() measures each character individually for CJK text. :(

[Myles C. Maxfield]

Created attachment 252578 [details]
WIP

[Myles C. Maxfield]

Created attachment 252673 [details]
Patch

[Myles C. Maxfield]

Created attachment 252674 [details]
Patch

[Myles C. Maxfield]

stripTrailingSpace() in RenderBlockFlow also needs to be updated

[Myles C. Maxfield]

Also RenderText::trimmedPrefWidths()

[Myles C. Maxfield]

Callers of SVGInlineTextBox::constructTextRun() as well

[Myles C. Maxfield]

.... along with SVGTextMetrics::constructTextRun

[Myles C. Maxfield]

Created attachment 252838 [details]
Patch

[Myles C. Maxfield]

And Font::widthForGlyph()

[Csaba Osztrogonác]

Comment on attachment 240909 [details]
Patch

Cleared Simon Fraser's review+ from obsolete attachment 240909 [details] so that this bug does not appear in http://webkit.org/pending-commit.