[Webkit-unassigned] [Bug 19681] borderColor on table elements should be ignored

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 6 21:27:11 PST 2012


https://bugs.webkit.org/show_bug.cgi?id=19681


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #125623|review?                     |review-
               Flag|                            |




--- Comment #9 from Julien Chaffraix <jchaffraix at webkit.org>  2012-02-06 21:27:11 PST ---
(From update of attachment 125623)
View in context: https://bugs.webkit.org/attachment.cgi?id=125623&action=review

I would have a question: do we know of any site that are failing now due to this bug? Is borderColor that important in the wild that we should support it? (bugzilla reports 2 bugs that are pretty old on 'borderColor')

I don't know what you used to generate the patch but it is confusing our review tool (we can't see the image you are adding) and our EWS (they can't apply the patch). Please generate your patch with webkit-patch as it guarantees that our tools will be able to process it. r- mostly because no automated test were run.

> Source/WebCore/ChangeLog:8
> +        borderColor is not a standard table attribute,

I don't know what is a "standard table attribute".

> Source/WebCore/ChangeLog:9
> +        hence borderColor should be ignored atleast when border is not specified.

You should put in your ChangeLog the comparison you did with FF and IE and say that it matches them. It's also an IE-only attribute that is not standardized anywhere so this makes sense.

> LayoutTests/fast/table/table-ignore-bordercolor-when-noborder.html:3
> +    <title>Test to ignore borderColor attribute when border is not present. 19681</title>

The bug number is kind of meaningless here. It would be more readable to state "bug 19681" without abbreviating. Usually you want your bug number to be visible in your test output (if you decide to include some text of course) which won't be the case here.

> LayoutTests/fast/table/table-ignore-bordercolor-when-noborder.html:6
> +        if (window.layoutTestController)
> +            layoutTestController.waitUntilDone();

Not calling layoutTestController.notifyDone() make your test fail! (see the first line of your expected.txt file)

You don't need waitUntilDone as by default DRT waits for the end of the 'load' event handler.

> LayoutTests/fast/table/table-ignore-bordercolor-when-noborder.html:17
> +                <td >Some Text</td> 

This text doesn't add anything and worse: it makes your output platform-dependent. See http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#Pixeltesttips

Here you could be just setting height: 100px; width: 100px to all table cells without this bad effect.

> LayoutTests/fast/table/table-ignore-bordercolor-when-noborder.html:21
> +        <table borderColor="red"> 

My take on using colors is that red should be for failures. Other reviewers have other options on that though.

> LayoutTests/fast/table/table-ignore-bordercolor-when-noborder.html:39
> +</html>

This test is a good example where:
* you don't really need a DRT dumps and the associated image as you just want to know if the style was applied so using getComputedStyle() should be enough.
* if I am mistaking, this should be a ref-test as pointed out by Robert.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list