Bug 89826

Summary: CSS 2.1 failure: Word-spacing affects each space and non-breaking space
Product: WebKit Reporter: Robert Hogan <robert>
Component: CSSAssignee: Robert Hogan <robert>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dino, eric, gyuyoung.kim, jshin, mitz, rakuco, robert, thorton, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 93881    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-04
none
Patch
none
Patch
none
Patch
none
Patch none

Description Robert Hogan 2012-06-24 04:42:30 PDT
Add word-spacing for each eligible space.
Comment 1 Robert Hogan 2012-06-24 11:47:27 PDT
Created attachment 149197 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-24 12:15:12 PDT
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
Comment 3 WebKit Review Bot 2012-06-24 12:15:16 PDT
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
Comment 4 Robert Hogan 2012-07-02 11:52:40 PDT
Created attachment 150459 [details]
Patch
Comment 5 mitz 2012-07-02 12:10:19 PDT
Comment on attachment 150459 [details]
Patch

Shouldn’t the complex text code path be patched as well? r- for now.
Comment 6 Robert Hogan 2012-07-02 12:35:05 PDT
(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.
Comment 7 Robert Hogan 2012-07-09 12:29:18 PDT
Created attachment 151297 [details]
Patch
Comment 8 mitz 2012-07-09 13:21:41 PDT
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.
Comment 9 Robert Hogan 2012-07-10 13:37:44 PDT
(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.
Comment 10 Robert Hogan 2012-07-12 13:19:21 PDT
Created attachment 152042 [details]
Patch
Comment 11 Robert Hogan 2012-07-13 11:48:11 PDT
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.
Comment 12 Robert Hogan 2012-07-16 14:31:21 PDT
Hi Dan,

Could you take a look at this one?

Thanks,
Robert
Comment 13 Eric Seidel (no email) 2012-08-10 12:09:04 PDT
Comment on attachment 152042 [details]
Patch

Seems reasonable.
Comment 14 Robert Hogan 2012-08-13 11:55:54 PDT
Committed r125430: <http://trac.webkit.org/changeset/125430>
Comment 15 WebKit Review Bot 2012-08-13 12:39:41 PDT
Re-opened since this is blocked by 93881
Comment 16 Dean Jackson 2012-08-13 12:46:27 PDT
This caused some failures on Apple Mountain Lion.
https://bugs.webkit.org/show_bug.cgi?id=93882
Comment 17 Robert Hogan 2012-08-13 12:46:41 PDT
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
Comment 18 Robert Hogan 2012-08-13 12:56:55 PDT
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
Comment 19 Robert Hogan 2012-08-13 13:15:50 PDT
Created attachment 158085 [details]
Patch
Comment 20 Robert Hogan 2012-08-14 10:37:35 PDT
Committed r125578: <http://trac.webkit.org/changeset/125578>
Comment 21 mitz 2012-09-14 20:58:03 PDT
(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.
Comment 22 mitz 2012-09-14 22:22:06 PDT
<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>
Comment 23 mitz 2012-09-14 22:34:31 PDT
(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.
Comment 24 Eric Seidel (no email) 2012-09-14 22:36:30 PDT
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.
Comment 25 mitz 2012-09-14 23:44:35 PDT
(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.
Comment 26 mitz 2012-09-15 11:03:42 PDT
(In reply to comment #20)
> Committed r125578: <http://trac.webkit.org/changeset/125578>

This caused bug 96865.
Comment 27 mitz 2012-09-15 14:58:42 PDT
(In reply to comment #20)
> Committed r125578: <http://trac.webkit.org/changeset/125578>

This also caused bug 96869.
Comment 28 Robert Hogan 2012-09-16 01:36:54 PDT
(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.