Bug 101677 – REGRESSION(r126683): Table cell are getting borders when the style doesn't mention any

• Component: Tables
• Status: RESOLVED FIXED

[Julien Chaffraix]

Created attachment 173164 [details]
Reduction

Following r126683, it seems that it's possible for a table cell with rule="none" to get some 3px borders as we don't set the 'border-width' to 0px.

The test case just sets bordercolor which gets translated to border-style: solid and border-color. A 3px border-width get applied (which I haven't traced back yet) which results in visible borders. This doesn't match our old behavior nor other browsers.

[Robert Hogan]

Created attachment 173667 [details]
Patch

[Julien Chaffraix]

Comment on attachment 173667 [details]
Patch

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

> Source/WebCore/html/HTMLTableElement.h:71
> +    bool borderAttribute() const { return m_borderAttr; }

m_borderAttr is misnamed as it only represents if we should consider that we have a border attribute. As such the function should be named hasBorderAttribute.

Also I would like the following cases to be covered (if they are not already):
* border is the empty string.
* border is null (setAttribute("border", null)).

These will lead to m_borderAttr = true which should show some borders in the new code.

> Source/WebCore/html/HTMLTablePartElement.cpp:58
> +        HTMLTableElement* table = findParentTable();
> +        if (!attribute.value().isEmpty() && (!table || table->borderAttribute())) {

Having a test for the no-table-case would help, you will need to do it in XHTML AFAICT as the HTML5 parsing algorithm makes it hard (impossible?) not to have a containing table.

> LayoutTests/ChangeLog:9
> +        * fast/css/table-cell-border-in-table-with-no-borderattr-expected.html: Added.
> +        * fast/css/table-cell-border-in-table-with-no-borderattr.html: Added.

The test doesn't have to be a ref-test. Here you can just query the computed style which would be better & faster.

[Robert Hogan]

Created attachment 173935 [details]
Patch

[Robert Hogan]

(In reply to comment #2)
> * border is null (setAttribute("border", null)).

Turns out we don't handle setAttribute("border", null); here - the value comes through as the string "null" so we don't interpret it properly. I think this is a separate bug: see bug 102112

[Robert Hogan]

(In reply to comment #3)
> Created an attachment (id=173935) [details]
> Patch

I've changed my approach here and removed support for bordercolor from cells, rows, cols etc. This is consistent with all other browsers except IE.

http://roberthogan.net/css/td-bordercolor-attribute.html is a good test to use to compare the difference between IE and everyone else.

[Julien Chaffraix]

Comment on attachment 173935 [details]
Patch

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

This change is actually dropping our support for borderColor on any table part so you should be following http://trac.webkit.org/wiki/DeprecatingFeatures. This is also a very visible change so the impact should be evaluated.

> Source/WebCore/ChangeLog:12
> +        the parent has no border attribute. WebKit's old behaviour (before r126683) was to display the
> +        bordercolor of the row but not the cells (I don't think this behaviour was particularly deliberate). No
> +        browser displays the bordercolor for a col or tbody element.

I traced the borderColor support and this code is super old and was merged from KHTML in r3351.

We never came back to investigate if our behavior made sense AFAICT. It's really a pita that this is not standardized anywhere and we rely on ad-hoc behavior. It would be nice if you could send something to the HTML5 spec about that with how most browsers are working.

> Source/WebCore/html/HTMLTablePartElement.cpp:-56
> -    } else if (attribute.name() == bordercolorAttr) {

This is only half of the fix, you should also remove the attribute from isPresentationAttribute.

> LayoutTests/fast/table/td-bordercolor-attribute.html:12
> +    The bordercolor attribute on a cell or row should have no effect. <br>

That's understated. You are also removing support for column, column-group (not tested btw) and row-group too.

> LayoutTests/fast/table/td-bordercolor-attribute.html:15
> +    <table id="table" border="0" cellpadding="5" cellspacing="0" width="100%" height="100%">

I don't think we need all those attributes, e.g. cellpadding, cellspacing and probably height are useless.

> LayoutTests/fast/table/td-bordercolor-attribute.html:50
> +        <col id="col6">

Unneeded column.

> LayoutTests/fast/table/td-bordercolor-attribute.html:52
> +        <tr id="row6">
> +            <td id="cell6" height="26"></td>

Unneeded ids, I haven't repeated those comments but any unneeded attribute should be removed.

[Robert Hogan]

(In reply to comment #6)
> (From update of attachment 173935 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173935&action=review
> 
> This change is actually dropping our support for borderColor on any table part so you should be following http://trac.webkit.org/wiki/DeprecatingFeatures. This is also a very visible change so the impact should be evaluated.

I think that might be going a bit far :) It's really not that visible at all - pre-r126683 the bordercolor only shows up when it is set on a row. Otherwise it is never seen. The behaviour of bordercolor is so inconsistent in this regard it can hardly be called a feature at all and it seems hard to imagine how anyone could depend on it for anything.

> 
> > Source/WebCore/ChangeLog:12
> > +        the parent has no border attribute. WebKit's old behaviour (before r126683) was to display the
> > +        bordercolor of the row but not the cells (I don't think this behaviour was particularly deliberate). No
> > +        browser displays the bordercolor for a col or tbody element.
> 
> I traced the borderColor support and this code is super old and was merged from KHTML in r3351.
> 
> We never came back to investigate if our behavior made sense AFAICT. It's really a pita that this is not standardized anywhere and we rely on ad-hoc behavior. It would be nice if you could send something to the HTML5 spec about that with how most browsers are working.

I think it's standardized (by omission) in http://dev.w3.org/html5/spec/single-page.html#tables to be honest. |bordercolor| is only mentioned in relation to frame,frameset and table there.

> 
> > Source/WebCore/html/HTMLTablePartElement.cpp:-56
> > -    } else if (attribute.name() == bordercolorAttr) {
> 
> This is only half of the fix, you should also remove the attribute from isPresentationAttribute.

Whoops.

[Julien Chaffraix]

Comment on attachment 173935 [details]
Patch

>> This change is actually dropping our support for borderColor on any table part so you should be following http://trac.webkit.org/wiki/DeprecatingFeatures. This is also a very visible change so the impact should be evaluated.

> I think that might be going a bit far :) It's really not that visible at all - pre-r126683 the bordercolor only shows up when it is set on a row. Otherwise it is never seen. The behaviour of bordercolor is so inconsistent in this regard it can hardly be called a feature at all and it seems hard to imagine how anyone could depend on it for anything.

After talking with Robert on IRC, we don't match other browsers here and it seems that it was never the case. Removing our half-working support aligns us with Firefox, Opera, what is explicitly defined in HTML5 but not IE. It really looks like the compatibility risk is small (I couldn't find explicit mention of bordercolor on anything other than <table>) so you convinced me that the change is OK.

[Robert Hogan]

Created attachment 177327 [details]
Patch

[Julien Chaffraix]

Comment on attachment 177327 [details]
Patch

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

r=me!

> LayoutTests/fast/table/td-bordercolor-attribute.html:70
> +    shouldBeEqualToString('document.defaultView.getComputedStyle(cell, null).getPropertyValue("border-top-color")', "rgb(0, 0, 0)");

For the next time: you can do window.getComputedStyle (shorter is better!) and Firefox don't need the null parameter anymore per https://developer.mozilla.org/en-US/docs/DOM/window.getComputedStyle

[WebKit Review Bot]

Comment on attachment 177327 [details]
Patch

Clearing flags on attachment: 177327

Committed r136712: <http://trac.webkit.org/changeset/136712>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.