Bug 69723 - Visual glitch in webkit html table respect both firefox and IE
Summary: Visual glitch in webkit html table respect both firefox and IE
Status: RESOLVED DUPLICATE of bug 20840
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Shezan Baig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-09 05:18 PDT by Nelson Benitez
Modified: 2012-04-15 20:55 PDT (History)
5 users (show)

See Also:


Attachments
html test case (1.76 KB, text/html)
2011-10-09 05:19 PDT, Nelson Benitez
no flags Details
Image for html testcase (1.52 KB, image/png)
2011-10-09 05:20 PDT, Nelson Benitez
no flags Details
Looking good in firefox and IE (2.71 KB, image/png)
2011-10-09 05:21 PDT, Nelson Benitez
no flags Details
Looking bad in Webkit (2.73 KB, image/png)
2011-10-09 05:22 PDT, Nelson Benitez
no flags Details
Patch (13.22 KB, patch)
2012-04-13 14:17 PDT, Shezan Baig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nelson Benitez 2011-10-09 05:18:18 PDT
Steps to reproduce the problem:
1. Download the webkitbug.html and tools24.png attachment files and put on same folder.
2. Open webkitbug.html file in chrome, firefox and IE browser

Firefox and IE displays fine, you can find how it should look in attachment GOOD_FIREFOX.png

Webkit seems to made a mess with two last rows of the table (which are hidden) and shows an inappropiate extra border in the second column, you can see that in attachment BAD_CHROME.png .
Comment 1 Nelson Benitez 2011-10-09 05:19:36 PDT
Created attachment 110300 [details]
html test case
Comment 2 Nelson Benitez 2011-10-09 05:20:30 PDT
Created attachment 110301 [details]
Image for html testcase
Comment 3 Nelson Benitez 2011-10-09 05:21:54 PDT
Created attachment 110302 [details]
Looking good in firefox and IE
Comment 4 Nelson Benitez 2011-10-09 05:22:50 PDT
Created attachment 110303 [details]
Looking bad in Webkit
Comment 5 Shezan Baig 2012-04-13 14:17:21 PDT
Created attachment 137149 [details]
Patch
Comment 6 Julien Chaffraix 2012-04-13 15:14:48 PDT
Comment on attachment 137149 [details]
Patch

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

This bug is a duplicate of bug 20840 for which I posted a patch some time ago but never followed up. Please check for duplicates when fixing bugs as this is a super old bug.

> Source/WebCore/rendering/RenderTableCell.cpp:417
> +bool RenderTableCell::isHorizontallyWithin(const RenderTableCell* other) const
> +{
> +    return col() >= other->col()
> +        && col() + colSpan() <= other->col() + other->colSpan();
> +}
> +
> +bool RenderTableCell::isVerticallyWithin(const RenderTableCell* other) const
> +{
> +    return row() >= other->row()
> +        && row() + rowSpan() <= other->row() + other->rowSpan();
> +}

I don't really think that matches other browsers. Also I really think it's not a good change: it completely ignores the fact that the 2 cells can be in any configurations (above / under, left / right) and thus only checks for some of the cases.

It's also not specified in CSS 2.1 so we should keep our behavior simple and not tied to a complex algorithm. The best way would be to get this behavior specified (likely matching FF, which means resolving collapsed borders by comparing adjacent cells even if that means cutting the border for colspan or rowspan) and implement it properly.

> LayoutTests/fast/table/border-collapsing/col-row-span-collapsing-expected.txt:1
> +CONSOLE MESSAGE: line 67: TypeError: 'undefined' is not an object (evaluating 'divs[i].style.display = "none"')

We prefer tests that don't have exceptions.
Comment 7 Shezan Baig 2012-04-15 20:54:31 PDT
Thanks for taking a look, Julien :)

(In reply to comment #6)
> This bug is a duplicate of bug 20840 for which I posted a patch some time ago 
> but never followed up. Please check for duplicates when fixing bugs as this 
> is a super old bug.

Sorry about that, I'll mark this bug as a duplicate of bug 20840



> > Source/WebCore/rendering/RenderTableCell.cpp:417
> > +bool RenderTableCell::isHorizontallyWithin(const RenderTableCell* other) const
> > +{
> > +    return col() >= other->col()
> > +        && col() + colSpan() <= other->col() + other->colSpan();
> > +}
> > +
> > +bool RenderTableCell::isVerticallyWithin(const RenderTableCell* other) const
> > +{
> > +    return row() >= other->row()
> > +        && row() + rowSpan() <= other->row() + other->rowSpan();
> > +}
> 
> I don't really think that matches other browsers. Also I really think it's 
> not a good change: it completely ignores the fact that the 2 cells can be in 
> any configurations (above / under, left / right) and thus only checks for 
> some of the cases.


I'm not sure what exactly you mean here, because it seems to me like the above/under/left/right cases are handled by the changes in computeCollapsedBeforeBorder, computeCollapsedAfterBorder, computeCollapsedStartBorder, computeCollapsedEndBorder respectively.  Unless there's something else I didn't consider?


> It's also not specified in CSS 2.1 so we should keep our behavior simple and 
> not tied to a complex algorithm. The best way would be to get this behavior 
> specified (likely matching FF, which means resolving collapsed borders by 
> comparing adjacent cells even if that means cutting the border for colspan or 
> rowspan) and implement it properly.

Sounds like a good plan.


> > LayoutTests/fast/table/border-collapsing/col-row-span-collapsing-expected.txt:1
> > +CONSOLE MESSAGE: line 67: TypeError: 'undefined' is not an object (evaluating 'divs[i].style.display = "none"')
> 
> We prefer tests that don't have exceptions.


Hmm sorry about that.  I know why the exception could happen, but somehow it didn't happen when I ran the tests locally.
Comment 8 Shezan Baig 2012-04-15 20:55:02 PDT

*** This bug has been marked as a duplicate of bug 20840 ***