[OSX] Some words are placed on top of each other in complex text layout
Created attachment 240909 [details] Patch
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
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
<rdar://problem/18797255>
Created attachment 240934 [details] Updated for older OS X
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?
I'm not going to submit this until I make sure that line breaking and max width works correctly (or fix them)
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.
http://trac.webkit.org/changeset/175813
<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> Woè Woe Woe Woe Woe Woe Woe Woe Woe Woe Woe Woe Woe Woe </div> <div> Woè 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.
(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?
Re-opened since this is blocked by bug 138573
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)
https://bugs.webkit.org/show_bug.cgi?id=138581
Created attachment 252430 [details] WIP
Created attachment 252431 [details] Reduction
In addition to RenderText::computePreferredLogicalWidths(), TextFragmentIterator in SimpleLineLayout must be updated.
TextFragmentIterator: spaceWidth(font.width(TextRun(&space, 1))) if (startPosition < nextPosition) width = m_style.spaceWidth;
BreakingContext::handleText() also needs to be updated.
Man, the code inside RenderText::computePreferredLogicalWidths() and BreakingContext::handleText() really have to be unified some day.
BreakingContext::handleText() actually already includes 1 character of prior whitespace
Created attachment 252570 [details] WIP
(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)
RenderText::computePreferredLogicalWidths() measures each character individually for CJK text. :(
Created attachment 252578 [details] WIP
Created attachment 252673 [details] Patch
Created attachment 252674 [details] Patch
stripTrailingSpace() in RenderBlockFlow also needs to be updated
Also RenderText::trimmedPrefWidths()
Callers of SVGInlineTextBox::constructTextRun() as well
.... along with SVGTextMetrics::constructTextRun
Created attachment 252838 [details] Patch
And Font::widthForGlyph()
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.