Bug 71705 - CSS 2.1 failure: border-collapse-offset-002.htm fails
Summary: CSS 2.1 failure: border-collapse-offset-002.htm fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robert Hogan
URL: http://test.csswg.org/suites/css2.1/n...
Keywords:
Depends on: 71244 72534
Blocks: 47141 74888
  Show dependency treegraph
 
Reported: 2011-11-07 10:34 PST by Robert Hogan
Modified: 2012-01-02 06:29 PST (History)
6 users (show)

See Also:


Attachments
Patch (120.16 KB, patch)
2011-11-08 12:05 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch for WebCore.xcodeproj (4.26 KB, text/plain)
2011-11-16 17:26 PST, Kenneth Russell
no flags Details
Patch (113.97 KB, patch)
2011-11-17 10:22 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (114.06 KB, patch)
2011-11-17 10:35 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (133.52 KB, patch)
2011-11-22 11:50 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (88.17 KB, patch)
2011-12-20 04:02 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (89.60 KB, patch)
2011-12-28 08:52 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Rebaselined results on Chromium Linux (deleted)
2012-01-01 06:29 PST, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2011-11-07 10:34:14 PST
the caption has the wrong width
Comment 1 Robert Hogan 2011-11-08 12:05:21 PST
Created attachment 114130 [details]
Patch
Comment 2 Antti Koivisto 2011-11-08 12:14:38 PST
Comment on attachment 114130 [details]
Patch

r=me, but you should put RenderTableCaption to a file of its own.
Comment 3 Robert Hogan 2011-11-08 12:21:10 PST
This won't build on the bots as it depends on 71244 which I haven't landed yet, sorry for the upcoming bug spam.
Comment 4 Robert Hogan 2011-11-16 11:13:16 PST
Committed r100473: <http://trac.webkit.org/changeset/100473>
Comment 5 Kenneth Russell 2011-11-16 17:26:23 PST
Created attachment 115490 [details]
Patch for WebCore.xcodeproj

Here's the necessary patch to WebCore.xcodeproj/project.pbxproj to allow the new file to build on Mac. Tested by building WebKit on Snow Leopard.

I strongly recommend assembling a new, complete patch and uploading it to this bug. It was confusing that the patch landed differed significantly from this one -- and the EWS would have caught the build failure.
Comment 6 Kenneth Russell 2011-11-16 17:27:12 PST
This was rolled out in https://bugs.webkit.org/show_bug.cgi?id=72534 / http://trac.webkit.org/changeset/100473 .
Comment 7 Robert Hogan 2011-11-17 10:22:28 PST
Created attachment 115615 [details]
Patch
Comment 8 Robert Hogan 2011-11-17 10:35:58 PST
Created attachment 115617 [details]
Patch
Comment 9 Julien Chaffraix 2011-11-17 12:08:59 PST
Comment on attachment 115617 [details]
Patch

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

> Source/WebCore/rendering/RenderTable.h:256
> +    mutable Vector<RenderTableCaption*> m_captions;

Do we need the mutable here? (asking as you are modifying some code that would love such neat changes)

> Source/WebCore/rendering/RenderTableCaption.cpp:23
> +using namespace std;

This is unneeded.

> Source/WebCore/rendering/RenderTableCaption.cpp:38
> +    RenderBlock* cb = containingBlock();

I don't think it is guaranteed for your containing block to be the table (using some absolute or fixed positioning for example). In this case, you are changing the behavior of the table caption without having some tests AFAICT.

> Source/WebCore/rendering/RenderTableCaption.h:33
> +    virtual bool isTableCaption() const { return true; }

Should be private to catch unneeded checks.

> Source/WebCore/rendering/RenderTableCaption.h:40
> +}

Please implement the other cast methods, especially the one that catches unneeded casts.
Comment 10 WebKit Review Bot 2011-11-18 00:06:41 PST
Comment on attachment 115617 [details]
Patch

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

New failing tests:
tables/mozilla/marvin/backgr_position-table.html
tables/mozilla/core/captions.html
tables/mozilla/marvin/backgr_simple-table-column-group.html
tables/mozilla/bugs/bug1302.html
fast/replaced/width100percent-textarea.html
tables/mozilla/marvin/backgr_layers-opacity.html
tables/mozilla/bugs/bug5838.html
tables/mozilla/marvin/backgr_simple-table-cell.html
tables/mozilla/bugs/bug1163-1.html
Comment 11 Robert Hogan 2011-11-22 11:50:31 PST
Created attachment 116258 [details]
Patch
Comment 12 Robert Hogan 2011-11-22 12:01:55 PST
(In reply to comment #9)
> > Source/WebCore/rendering/RenderTableCaption.cpp:38
> > +    RenderBlock* cb = containingBlock();
> 
> I don't think it is guaranteed for your containing block to be the table (using some absolute or fixed positioning for example). 
> In this case, you are changing the behavior of the table caption without having some tests AFAICT.

Right, I've added some tests for absolutely positioned captions but I'm not sure I've captured the use-case you envisage here.
Comment 13 Julien Chaffraix 2011-11-22 22:40:34 PST
Comment on attachment 116258 [details]
Patch

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

> Right, I've added some tests for absolutely positioned captions but I'm not sure I've captured the use-case you envisage here.

The test cases makes sense and cover what I had in mind, thanks!

> Source/WebCore/ChangeLog:7
> +

This is missing the tests that you added in LayoutTests!

> LayoutTests/ChangeLog:15
> +        Affected tables/mozilla tests are rebaselined in a follow-up patch.

s/are/will be/.

I am assuming those rebaselines are small and inconsequential (like some minor pixel / dump differences). Make sure you post the updates on this bug for people to follow those.

> LayoutTests/fast/css/caption-width-absolute-position-offset-top.htm:1
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">

For custom tests, we really prefer HTML5 doctype:

<!DOCTYPE html>
Comment 14 Robert Hogan 2011-12-20 04:02:34 PST
Created attachment 120005 [details]
Patch
Comment 15 Julien Chaffraix 2011-12-21 00:48:05 PST
Comment on attachment 120005 [details]
Patch

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

Too bad your change confuses the EWS. The previous changes passed the EWS so I am inclined to trust this patch is as good.

> Source/WebCore/rendering/RenderTable.cpp:120
> +        m_captions.append(toRenderTableCaption(child));

There are several other callsites in RenderTable.cpp that are checking:

child->isRenderBlock() && child->style()->display() == TABLE_CAPTION

They should be modified too. There is also one in RenderTheme.cpp that should be patched.

> Source/WebCore/rendering/RenderTableCaption.h:34
> +    virtual LayoutUnit containingBlockLogicalWidthForContent() const;
> +    
> +private:
> +    virtual bool isTableCaption() const { return true; }

You should use the new OVERRIDE keyword for those 2 functions.

> Source/WebCore/rendering/RenderTableCaption.h:43
> +inline const RenderTableCaption* toRenderTableCaption(const RenderObject* object)

Nit: inline is not needed here as anything in a header is inlined by default. That said better to be sure sometimes.
Comment 16 Julien Chaffraix 2011-12-21 01:11:25 PST
Comment on attachment 120005 [details]
Patch

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

> Source/WebCore/rendering/RenderTableCaption.h:32
> +    

Thinking again: don't we want to have our own renderName() here? If that doesn't make you rebaseline / disable a lot more tests, I would rather have it added sooner rather than later!
Comment 17 Robert Hogan 2011-12-21 03:08:57 PST
(In reply to comment #15)
> (From update of attachment 120005 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120005&action=review
> 
> Too bad your change confuses the EWS. The previous changes passed the EWS so I am inclined to trust this patch is as good.

Yes, that's annoying. I need to be sure it compiles on Mac before I commit. So hopefully it can pass the commit queues.
Comment 18 Robert Hogan 2011-12-28 08:52:48 PST
Created attachment 120678 [details]
Patch
Comment 19 WebKit Review Bot 2011-12-28 11:49:45 PST
Comment on attachment 120678 [details]
Patch

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

New failing tests:
fast/css/bidi-override-in-anonymous-block.html
Comment 20 Robert Hogan 2012-01-01 06:29:00 PST
Created attachment 120844 [details]
Rebaselined results on Chromium Linux
Comment 21 Robert Hogan 2012-01-01 07:00:39 PST
(In reply to comment #16)
> (From update of attachment 120005 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120005&action=review
> 
> > Source/WebCore/rendering/RenderTableCaption.h:32
> > +    
> 
> Thinking again: don't we want to have our own renderName() here? If that doesn't make you rebaseline / disable a lot more tests, I would rather have it added sooner rather than later!

This will rebaseline about 60 extra tests so I'll do it separately.
Comment 22 Robert Hogan 2012-01-01 07:14:19 PST
Committed r103875: <http://trac.webkit.org/changeset/103875>