Bug 19681 – borderColor on table elements should be ignored

• Component: Layout and Rendering
• Status: RESOLVED DUPLICATE

[Ojan Vafai]

IE, Firefox 3 and Opera all ignore it. Doesn't seem like a big deal, but I noticed it and figured the bug should be on file. :)

[Ojan Vafai]

Created attachment 21850 [details]
page with borderColor on a table.

[Ojan Vafai]

To clarify, they all ignore borderColor on tables in both standards and quirks.

[Vamshi Krishna N]

Created attachment 121789 [details]
patch proposed for "border color on table should be ignored"

Tables border colors should only be drawn when ever the border of the table element is specified,
lonely specified border color must be ignored as this is cannot be with out the border,
this is the proposed patch along with the test case provided.

[Andreas Kling]

Comment on attachment 121789 [details]
patch proposed for "border color on table should be ignored"

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

Does this new behavior match what other browsers are doing? The bug description seems to say that we should ignore bordercolorAttr altogether.

> Source/WebCore/html/HTMLTableElement.cpp:340
> -        m_borderColorAttr = attr->decl();
> -        if (!attr->decl() && !attr->isEmpty()) {
> -            addCSSColor(attr, CSSPropertyBorderColor, attr->value());
> -            m_borderColorAttr = true;
> -        }
> +       if (m_borderAttr) {        
> +            m_borderColorAttr = attr->decl();
> +            if (!attr->decl() && !attr->isEmpty()) {
> +                addCSSColor(attr, CSSPropertyBorderColor, attr->value());
> +                m_borderColorAttr = true;
> +            }
> +       }

parseMappedAttribute() shouldn't depend on the presence/absensce of attributes other than the one being parsed. This will break if borderAttr is parsed before bordercolorAttr.
HTMLTableElement::additionalAttributeStyleDecls() exists for this purpose; to apply additional style that depends on multiple attributes.

> LayoutTests/fast/table/border-table-ignore.html:12
> +<table class="brdrcls" cellpadding="10">

This table doesn't have a bordercolor attribute, which means that this test will not hit the code path you're changing.
Furthermore, it should be possible to do this as a dumpAsText() test.

[Robert Hogan]

Comment on attachment 121789 [details]
patch proposed for "border color on table should be ignored"

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

> Source/WebCore/html/HTMLTableElement.cpp:NaN
>  void HTMLTableElement::parseMappedAttrib

I think you should be fixing this in HTMLTableElement::additionalAttributeStyleDecls() instead. If there's a border attribute we want to add a border, otherwise we do not. Currently we add a border if either are present. Discarding the bordercolor attribute altogether would be wrong, since the border attribute could be added dynamically later I think.

> LayoutTests/fast/table/border-table-ignore.html:8
> +}

You need a few tests here, each with a a different combination of the 'border' and 'bordercolor' attributes so that the corrected behaviour is clear. FF and Opera only give the table a border if the border attribute is present.

[Kishore Bolisetty]

(In reply to comment #5)
> (From update of attachment 121789 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121789&action=review
> 
> > Source/WebCore/html/HTMLTableElement.cpp:NaN
> >  void HTMLTableElement::parseMappedAttrib
> 
> I think you should be fixing this in HTMLTableElement::additionalAttributeStyleDecls() instead. If there's a border attribute we want to add a border, otherwise we do not. Currently we add a border if either are present. Discarding the bordercolor attribute altogether would be wrong, since the border attribute could be added dynamically later I think.
> 
> > LayoutTests/fast/table/border-table-ignore.html:8
> > +}
> 
> You need a few tests here, each with a a different combination of the 'border' and 'bordercolor' attributes so that the corrected behaviour is clear. FF and Opera only give the table a border if the border attribute is present.

Thanks for the suggestions. I will resubmit the patch with necessary changes.

[Kishore Bolisetty]

Created attachment 125623 [details]
Fix to ignore borderColor attribute of table when its border attribute is not specified.

This patch ignores borderColor attribute of table when its border attribute is not specified.
Compared with firefox, and the patch inclines to firefox behaviour.
I tried to incorporate the suggestions provided by Andreas Kling and Robert Hogan as a part of their review comments.

[Robert Hogan]

Comment on attachment 125623 [details]
Fix to ignore borderColor attribute of table when its border attribute is not specified.

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

Change looks ok - but the tests need updating - you should use reference tests as I describe in my inline comment.

> LayoutTests/platform/win/fast/table/table-ignore-bordercolor-when-noborder-expected.txt:1
> +FAIL: Timed out waiting for notifyDone to be called

You can just drop the LayoutTestController stuff from the test, it's not necessary.

Create a LayoutTests/fast/table/table-ignore-bordercolor-when-noborder-expected.html as your expected result. run-webkit-tests will run table-ignore-bordercolor-when-noborder.html and table-ignore-bordercolor-when-noborder-expected.html and make sure they render the same. Naturally table-ignore-bordercolor-when-noborder-expected.html should not use the borderColor attribute to achieve the same result!

[Julien Chaffraix]

Comment on attachment 125623 [details]
Fix to ignore borderColor attribute of table when its border attribute is not specified.

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.

[Kishore Bolisetty]

Created attachment 126947 [details]
Patch

[WebKit Review Bot]

Attachment 126947 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168766 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robert Hogan]

Comment on attachment 126947 [details]
Patch

You need to add LayoutTestController.dumpAsText() to your test. Grep for it, there are many examples to follow. You may need to use waitUntilDone() and notifyDone() too. Also there's something wrong with the svn props on the new files - I use git, so not sure what to advise you here. You will need to fix both of these before approval.

[Kishore Bolisetty]

Created attachment 127351 [details]
Fix to ignore borderColor attribute of table when its border attribute is not specified.

Fix to ignore borderColor attribute of table when its border attribute is not specified.

The contains the following updates.
1.	Incorporated review comments from Robert Hogan and Julien chaffraix.
2.	Modified test case to avoid pixel testing using getComputedStyle() and detailed change log that includes comparison with FF
3.	Implemented test case without  using layoutTestController.waitUntillDone/notifyDone() , because the test doesn’t depends on any other event expect load. And by default DRT waits till load event.

[Kishore Bolisetty]

(In reply to comment #12)
> (From update of attachment 126947 [details])
> You need to add LayoutTestController.dumpAsText() to your test. Grep for it, there are many examples to follow. You may need to use waitUntilDone() and notifyDone() too. Also there's something wrong with the svn props on the new files - I use git, so not sure what to advise you here. You will need to fix both of these before approval.

Added LayoutTestController.dumpAsText()
waitUntilDone() and notifyDone() is not used, because the test doesn’t depends on any other event expect load. And by default DRT waits till load event.

[Kishore Bolisetty]

(In reply to comment #9)
> (From update of attachment 125623 [details])
> 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

I retained the text to highlight the tables with no borders, now this test doesn't use pixel testing. So this should be create any issue.
> 
> Here you could be just setting height: 100px; width: 100px to all table cells without this bad effect.
Corrected.
> 
> > 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.

Changed to green.
> 
> > 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.

now Using getComputedStyle().

[Alexis Menard (darktears)]

Comment on attachment 127351 [details]
Fix to ignore borderColor attribute of table when its border attribute is not specified.

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

I think the test should use the new pattern we use to write layout tests. For example : http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/getComputedStyle/getComputedStyle-border-radius-shorthand.html?rev=104469. I feel it more readable

> Source/WebCore/ChangeLog:8
> +        Hence borderColor should be ignored atleast when border attribute is not specified, inclining to firefox's implementation

Typo : atleast -> at least.

[Kishore Bolisetty]

Created attachment 127821 [details]
Fix to ignore borderColor attribute of table when its border attribute is not specified.

updated test case as per review comments.

[Alexis Menard (darktears)]

Comment on attachment 127821 [details]
Fix to ignore borderColor attribute of table when its border attribute is not specified.

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

> LayoutTests/fast/table/table-ignore-bordercolor-when-noborder.html:16
> +            testContainer.contentEditable = true;

I don't think it's needed. I know in some tests it was like that wrongly (I will get rid of those).

[Kishore Bolisetty]

Created attachment 127916 [details]
Fix to ignore borderColor attribute of table when its border attribute is not specified.

updated based on review comments.

[Julien Chaffraix]

Comment on attachment 127916 [details]
Fix to ignore borderColor attribute of table when its border attribute is not specified.

I really don't think we need to maintain this attribute. I haven't found any new content that mentioned it, FireFox still has a different interpretation of what bordercolor means (see http://www.htmlcodetutorial.com/tables/index_famsupp_149.html), CSS border-color is a suitable replacement and we have exactly 2 very old bugs related to that. It seems that the compatibility risk is low.

Unless you know of a web-site needing it, we should ask the Firefox community what their take on dumping the attribute would be.

[Kishore Bolisetty]

Created attachment 129672 [details]
Regarding borderColor attribute of table

Hi All,
As suggested by Julien Chaffraix, I posted a query in mozilla forums. Please find the attached response for the same.

Also copying the part of it here:

http://dev.w3.org/html5/spec/Overview.html#tables says:

 When a table element has a bordercolor attribute, its value is
 expected to be parsed using the rules for parsing a legacy color
 value, and if that does not return an error, the user agent is
 expected to treat the attribute as a presentational hint setting
 the element's 'border-top-color', 'border-right-color',
 'border-bottom-color', and 'border-right-color' properties to
 the resulting color.

[Kishore Bolisetty]

Comment on attachment 127916 [details]
Fix to ignore borderColor attribute of table when its border attribute is not specified.

Hi Julien Chaffraix,Can you please review this patch considering "Comment #21 "

[Julien Chaffraix]

(In reply to comment #21)
> Created an attachment (id=129672) [details]
> Regarding borderColor attribute of table

Thanks for following up with Mozilla people. In the future, please provide a URL instead of saving your email thread in HTML:

https://groups.google.com/group/mozilla.dev.tech.layout/browse_thread/thread/8bcd990ea33747e4#

> http://dev.w3.org/html5/spec/Overview.html#tables says:
> 
>  When a table element has a bordercolor attribute, its value is
>  expected to be parsed using the rules for parsing a legacy color
>  value, and if that does not return an error, the user agent is
>  expected to treat the attribute as a presentational hint setting
>  the element's 'border-top-color', 'border-right-color',
>  'border-bottom-color', and 'border-right-color' properties to
>  the resulting color.

I stand corrected on this one. The spec doesn't specify any relations between |border| and |borderColor| and there seem to be one (or at least you are going to define one). I tried IE9 with your patch and it doesn't seem to be recognizing |border|. I am fine with matching FF here but this should be brought to the WHATWG attention for standardization. There may have an opinion on that too.

[Kishore Bolisetty]

(In reply to comment #23)
> (In reply to comment #21)
> > Created an attachment (id=129672) [details] [details]
> > Regarding borderColor attribute of table
> 
> Thanks for following up with Mozilla people. In the future, please provide a URL instead of saving your email thread in HTML:
> 
> https://groups.google.com/group/mozilla.dev.tech.layout/browse_thread/thread/8bcd990ea33747e4#
> 
> > http://dev.w3.org/html5/spec/Overview.html#tables says:
> > 
> >  When a table element has a bordercolor attribute, its value is
> >  expected to be parsed using the rules for parsing a legacy color
> >  value, and if that does not return an error, the user agent is
> >  expected to treat the attribute as a presentational hint setting
> >  the element's 'border-top-color', 'border-right-color',
> >  'border-bottom-color', and 'border-right-color' properties to
> >  the resulting color.
> 
> I stand corrected on this one. The spec doesn't specify any relations between |border| and |borderColor| and there seem to be one (or at least you are going to define one). I tried IE9 with your patch and it doesn't seem to be recognizing |border|. I am fine with matching FF here but this should be brought to the WHATWG attention for standardization. There may have an opinion on that too.

Hi Julien,
As per your suggestion, I initiated a discussion on this topic at WHATWG. Please take a look at this URL.
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-March/035009.html

[Adele Peterson]

Comment on attachment 127916 [details]
Fix to ignore borderColor attribute of table when its border attribute is not specified.

I'm going to clear this from the review queue until there's some general consensus about whether we want this.

[Kishore Bolisetty]

(In reply to comment #25)
> (From update of attachment 127916 [details])
> I'm going to clear this from the review queue until there's some general consensus about whether we want this.

Hi All,
Could you please take a look at this thread.
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-June/036325.html

I guess we can reconsider this issue.

[Julien Chaffraix]

Comment on attachment 127916 [details]
Fix to ignore borderColor attribute of table when its border attribute is not specified.

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

The code has changed since the patch was created to account for the |rules| attribute. Kishore, could post an updated patch and I will be happy to r+? Thanks for the follow-up and good work.

> Source/WebCore/ChangeLog:10
> +        Comparision for Safari and Firefox whether to display border color or not dependening on the presence 

Typo: comparison.

[Kishore Bolisetty]

Created attachment 150076 [details]
patch to ignore bordercolor on table when no border is specified

[Kishore Bolisetty]

Comment on attachment 150076 [details]
patch to ignore bordercolor on table when no border is specified

There is a repeated typo. I will correct it and will re upload the patch.

[Kishore Bolisetty]

Created attachment 150864 [details]
patch to ignore bordercolor on table when no border is specified

[Julien Chaffraix]

Comment on attachment 150864 [details]
patch to ignore bordercolor on table when no border is specified 

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

AFAICT your change doesn't change anything in how the function behave, yet we pass your test. Is it possible that this got fixed in the meantime and that you only need to land your test case?

> Source/WebCore/html/HTMLTableElement.cpp:450
> +    if (m_borderColorAttr && m_borderColorAttr) {

This change doesn't do anything.

> Source/WebCore/html/HTMLTableElement.cpp:455
> +    if (m_borderAttr) {

Per the checks above, this should be an ASSERT as you cannot reach this point without having m_borderAttr be NULL.

> Source/WebCore/html/HTMLTableElement.cpp:460
> +
> +    return 0;

That case can't be reached as it would mean that !m_borderColorAttr && !m_borderAttr (which is checked above).

[Ahmad Saleem]

By using test case from attachment "page with borderColor on a table", these are results across browsers:

*** Safari Technical Preview 152 ***

Display "red" outer border and "grey" inner around text

*** Chrome Canary 107 ***

Same as Safari but thinner border size

*** Firefox Nightly 106 ***

No border just "some text"..

_______

Just wanted to share updated test results. From this:

https://lists.whatwg.org/pipermail/whatwg-whatwg.org/2012-June/036325.html

"Safari is incorrect".. It seems that we need to follow "Firefox" behavior here.

Appreciate if someone can have a look and decide whether it is better to ask Firefox to match others or change it to New and fix it. Thanks!

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #32)
> By using test case from attachment "page with borderColor on a table", these
> are results across browsers:
> 
> *** Safari Technical Preview 152 ***
> 
> Display "red" outer border and "grey" inner around text
> 
> *** Chrome Canary 107 ***
> 
> Same as Safari but thinner border size
> 
> *** Firefox Nightly 106 ***
> 
> No border just "some text"..
> 
> _______
> 
> Just wanted to share updated test results. From this:
> 
> https://lists.whatwg.org/pipermail/whatwg-whatwg.org/2012-June/036325.html
> 
> "Safari is incorrect".. It seems that we need to follow "Firefox" behavior
> here.
> 
> Appreciate if someone can have a look and decide whether it is better to ask
> Firefox to match others or change it to New and fix it. Thanks!

Post your change @Karl - we are now matching Firefox in the attached test case.

https://github.com/WebKit/WebKit/commit/9311f46770bac895da2a29aedd976598f44e1ca0

We can mark this as 'DUPLICATE of bug 256850'?

[Karl Dubost]

yup. Thanks!

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