[webkit-reviews] review denied: [Bug 101677] REGRESSION(r126683): Table cell are getting borders when the style doesn't mention any : [Attachment 173935] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 101677: REGRESSION(r126683): Table cell are getting borders when the style
doesn't mention any
https://bugs.webkit.org/show_bug.cgi?id=101677

Attachment 173935: Patch
https://bugs.webkit.org/attachment.cgi?id=173935&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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.