Bug 89826 – CSS 2.1 failure: Word-spacing affects each space and non-breaking space

• Component: CSS
• Status: RESOLVED FIXED

[Robert Hogan]

Add word-spacing for each eligible space.

[Robert Hogan]

Created attachment 149197 [details]
Patch

[WebKit Review Bot]

Comment on attachment 149197 [details]
Patch

Attachment 149197 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13039890

New failing tests:
fast/css/word-space-extra.html

[WebKit Review Bot]

Created attachment 149199 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Robert Hogan]

Created attachment 150459 [details]
Patch

Comment on attachment 150459 [details]
Patch

Shouldn’t the complex text code path be patched as well? r- for now.

[Robert Hogan]

(In reply to comment #5)
> (From update of attachment 150459 [details])
> Shouldn’t the complex text code path be patched as well? r- for now.

The complex text path is unique for each platform/port. It's not really possible for me to go there.

[Robert Hogan]

Created attachment 151297 [details]
Patch

Comment on attachment 151297 [details]
Patch

Seems like you also need to patch RenderText::widthFromCache(), and possibly also RenderText::trimmedPrefWidths() and RenderText::computePreferredLogicalWidths(). I don’t understand why none of the code in line layout (specifically LineBreaker::nextLineBreak()) isn’t affected by this change.

[Robert Hogan]

(In reply to comment #8)
> (From update of attachment 151297 [details])
> Seems like you also need to patch RenderText::widthFromCache(), and possibly also RenderText::trimmedPrefWidths() and RenderText::computePreferredLogicalWidths(). 

Yes, but there's clearly a lack of test coverage here. 

>I don’t understand why none of the code in line layout (specifically LineBreaker::nextLineBreak()) isn’t affected by this change.

It is, but I guess there isn't any existing layout test in which word-spacing is decisive in creating a new line or causing a float to push down.

[Robert Hogan]

Created attachment 152042 [details]
Patch

[Robert Hogan]

Comment on attachment 152042 [details]
Patch

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

> Source/WebCore/ChangeLog:33
> +        (WebCore::RenderText::widthFromCache): This code-path is not covered by any existing layout tests!
> +          I've tried to come up with a test to hit it but have failed.

Rather less dramatically - the clause altered here is a no-op for Chromium Linux as Font::isFixedPitch() is not implemented. This blocks me from testing it locally.

[Robert Hogan]

Hi Dan,

Could you take a look at this one?

Thanks,
Robert

[Eric Seidel (no email)]

Comment on attachment 152042 [details]
Patch

Seems reasonable.

[Robert Hogan]

Committed r125430: <http://trac.webkit.org/changeset/125430>

[WebKit Review Bot]

Re-opened since this is blocked by 93881

[Dean Jackson]

This caused some failures on Apple Mountain Lion.
https://bugs.webkit.org/show_bug.cgi?id=93882

[Robert Hogan]

I suspect css2.1/t1604-c541-word-sp-00-b-a.html may be invalid now but need to understand why it regressed on Mac before replacing it.

http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r125430%20(2289)/results.html

http://build.webkit.org/results/Chromium%20Mac%20Release%20(Tests)/r125430%20(21773)/results.html

[Robert Hogan]

Hi Eric,

Rolled out because it broke css2.1/t1604-c541-word-sp-00-b-a.htm on Mac builds, including Chromium Mac. 

I plan to replace css2.1/t1604-c541-word-sp-00-b-a.htm with a more up-to-date version but would still expect it to fail unless I remove the speculative code changes I made to platform/graphics/mac/ComplexTextController.cpp and WebCore::RenderText::widthFromCache.

Would you like to re-review before I attempt to land a second time tomorrow? 

Thanks,
Robert

[Robert Hogan]

Created attachment 158085 [details]
Patch

[Robert Hogan]

Committed r125578: <http://trac.webkit.org/changeset/125578>

(In reply to comment #20)
> Committed r125578: <http://trac.webkit.org/changeset/125578>

Quite remarkably, this change did not include any changes to ComplexTextController (such as the ones that were in <http://trac.webkit.org/r125430>). As a result, there are now new discrepancies between the fast path and the complex path. It seems like comment #5 was just ignored.

<http://trac.webkit.org/r125578> also neglected to patch RenderText, which introduced a serious discrepancy in this case:

<span style="font-size: 36px; word-spacing: 50px; border: solid red;">a &nbsp;b</span>

(In reply to comment #22)
> <http://trac.webkit.org/r125578> also neglected to patch RenderText, which introduced a serious discrepancy in this case:
> 
> <span style="font-size: 36px; word-spacing: 50px; border: solid red;">a &nbsp;b</span>

Filed bug 96856.

[Eric Seidel (no email)]

Sorry, my bad.  I make a bad habit of clicking on the review links directly.

I can roll this out, but Robert is really the man to talk to here.  Again, my apologies for inadvertently overriding your r- here.

(In reply to comment #21)
> (In reply to comment #20)
> > Committed r125578: <http://trac.webkit.org/changeset/125578>
> 
> Quite remarkably, this change did not include any changes to ComplexTextController (such as the ones that were in <http://trac.webkit.org/r125430>). As a result, there are now new discrepancies between the fast path and the complex path. It seems like comment #5 was just ignored.

Filed bug 96857.

(In reply to comment #20)
> Committed r125578: <http://trac.webkit.org/changeset/125578>

This caused bug 96865.

(In reply to comment #20)
> Committed r125578: <http://trac.webkit.org/changeset/125578>

This also caused bug 96869.

[Robert Hogan]

(In reply to comment #24)
> Sorry, my bad.  I make a bad habit of clicking on the review links directly.
> 
> I can roll this out, but Robert is really the man to talk to here.  Again, my apologies for inadvertently overriding your r- here.

The patch Eric r+'d did address Mitz's comments re the complex path. However I had to roll it out as it broke a couple of tests. I fixed those tests and re-landed it per comments 17 and 18.

I wish I hadn't re-landed now obviously - the platform-specific code paths for the complext text case and the fact that Chromium doesn't use the monospace code path mean that I stand very little chance of getting this right given my knowledge of this code. Sorry.