border of <td> not displayed when border-color and border-style are used in CSS

[jasneet]

I Steps:
1. Go to http://www.tizag.com/cssT/border.php
2. create a HTML table
3. specify a css style to the border of a <td>

II Issue: 
The border doesn't appear

III Conclusion:
When the styles defined for the <td> are defined as
    {border-color: red; border-style: ridge;}
Webkit doesn't display the borders. (see the columns in 1st row of the table in 
testcase)

But, when the styles are defined as
    {border: ridge blue;}
Webkit displays the borders.  (see the columns in 2nd row of the table in testcase)

IV Other Browsers:
FF3: OK
IE7: OK 

V Nightly tested: 41443

Bug in Chromium : http://code.google.com/p/chromium/issues/detail?id=6837

[jasneet]

Created attachment 28340 [details]
reduced testcase

[Mustafizur Rahaman (rahaman)]

I have done preliminary investigation of the issue.

Primarily I have found that the border width for the style element corresponding to 1st table cell element [(((*((((*((m_style).m_ptr)).surround).m_data).m_ptr)).border).m_left).m_width] is zero, where as the same value for the second table cell element is 3(I guess that is default value) & therefore, the 1st table cell border is not being drawn.

Upon further investigation I found that, while parsing value in CSSParser for the 1st Cell element,CSSParser::parseValue() is called with propId = 1038 (CSSPropertyBorderColor) & 1049 (CSSPropertyBorderStyle)

The same call flow for second cell element is called with propId=1030 (CSSPropertyBorder) & it internally includes BorderWidth property as follows

    case CSSPropertyBorder:
        // [ 'border-width' || 'border-style' || <color> ] | inherit
    {
        const int properties[3] = { CSSPropertyBorderWidth, CSSPropertyBorderStyle,
                                    CSSPropertyBorderColor };
        return parseShorthand(propId, properties, 3, important);
    }
    
i.e. the CSSMutableStyleDeclaration created for 1st cell element does not have BorderWidth where as the second CSSMutableStyleDeclaration does have the value. Therefore, in CSSStyleSelector::styleForElement(), while calling applyDeclarations with different rule does NOT add the BorderWidth to the StyleElement for the 1st cell, but the borderwidth for the style element for the second cell gets added
=========================================================

Then the question comes why it is the problem with Table cell ement only, because same style syntax for a paragraph element works perfectly fine.
td.borderCHex //Does not Work    
{
border-color:red;
border-style:solid;
}

p.borderCHex //Perfectly works
{
border-color:red;
border-style:solid;
}

Upon going into further details, I found that while calling 
applyDeclarations<false>(false, lastUARule + 1, m_matchedDecls.size() - 1); in CSSStyleSelector::styleForElement(), 

For table cell element, while running through all the matched declarations, first all the width value are set to zero & then it is again set to default value of 3px if CSSPropertyBorder was used as discussed above.

For paragraph element, it does not have any matching declaration except one & therefore no one sets the value to zero.Then I found that those extra matching declaration for table cell element are populated in the code with the following comments in
CSSStyleSelector::styleForElement()
// Now we check additional mapped declarations.
// Tables and table cells share an additional mapped rule that must be applied
// after all attributes, since their mapped style depends on the values of multiple attributes.

I kind of gave up here, Can any one help me with some more details to proceed further?

[Mustafizur Rahaman( :rahaman)]

I tried to look at this issue again as it is still reproducible. The problem is if we want to specify border properties for a <td> inside <table>, mentioning border-color & border-style is not sufficient. We have to explicitly specify border-width, too.

The reason being, in CSSStyleSelector::matchAllRules(), we try to add additional attribute style for table & table cell, and if no border-width is mentioned, the border-widths are all set to zero (in HTMLTableElement::createSharedCellStyle())

When we mention style property as "border: blue dashed", the border-width is assigned default value(while handling CSSPropertyBorder in CSSParser::parseValue()). So, even if the table cell sets the border-width as zero, the border-width from the CSSParser gets assigned to the Style element & therefore, the border is drawn properly. I have provided a summary of my analysis here, but I can discuss in details, if required.

Therefore, my question is:
#1> Do i need to explicitly mention border-width for <td> inside <table> i.e. is the given test case an invalid one?
#2> What is the need for the code we have in HTMLTableElement::createSharedCellStyle()? If we don't call that logic, the test is passing fine.
#3> It is working fine for FF.

Any help is greatly appreciated so that I can take this to logical closure...

[Mustafizur Rahaman( :rahaman)]

I had a discussion on this with kling in #webkit. Even though we agree that the code reference mentioned above is causing the problem, but not sure why those logic being present in HTMLTableElement::createSharedCellStyle(). Decided to take this to whatwg for further clarification.

[Mustafizur Rahaman( :rahaman)]

(In reply to comment #4)
> I had a discussion on this with kling in #webkit. Even though we agree that the code reference mentioned above is causing the problem, but not sure why those logic being present in HTMLTableElement::createSharedCellStyle(). Decided to take this to whatwg for further clarification.

Summary of the discussion on #whatwg with Hixie & zcorpan

(11:56:06 AM) Hixie: sounds like a bug in webkit
(11:56:25 AM) Hixie: the spec in question would be the CSS spec, though, not the HTML spec
(11:57:56 AM) zcorpan: Hixie: surely the rendering section is relevant
(11:58:20 AM) Hixie: not in this case, as far as i can tell
(11:58:27 AM) zcorpan: oh
(11:58:59 AM) Hixie: oh... wait...
(11:59:01 AM) Hixie: maybe it is...
(11:59:08 AM) Hixie: the border-width is ending up zero in webkit
(11:59:09 AM) Hixie: interesting
(11:59:15 AM) Hixie: wonder what the rendering section says about that...
(12:00:54 PM) rahaman: hixie: yeah i have debugged the relevant code in WebKit..
(12:01:25 PM) Hixie: this does seem like a webkit bug, but zcorpan is right, it's a default rendering rules bug
(12:01:25 PM) rahaman: if i dont mention border-width explicitly, the border-width is coming as zero for <td> because of some code we have written in webkit...
(12:01:55 PM) rahaman: n when I discussed about whay those part of the code is thr in #webkit, we could not find out the reason as this is old code..
(12:03:07 PM) rahaman: so me & Kling decided to discuss this here to get a clear idea of the expected behavior in such cases...
(12:04:03 PM) rahaman: hixie: I did not understand this "but zcorpan is right, it's a default rendering rules bug " :( can u pls elaborate a bit?
(12:04:19 PM) zcorpan: see http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#tables
(12:04:32 PM) zcorpan: that specifies the expected behavior
(12:04:38 PM) zcorpan: webkit does something different
(12:05:20 PM) zcorpan: i'd recommend writing tests against that, see that it passes other browsers (it it doesn't, maybe the spec is still wrong), and then implement it :-)
(12:05:45 PM) zcorpan: and share the tests :-)
(12:06:35 PM) zcorpan: (this can be checked with javascript so you could use http://w3c-test.org/resources/testharness.js )
(12:06:45 PM) Hixie: webkit seems to have some default rule for border-width even when there's no border attribute
(12:06:54 PM) Hixie: which contradicts the section zcorpan cited
(12:07:07 PM) Hixie: (sorry, would have explained earlier but ran into a bug with the status annotation script!)
(12:07:12 PM) rahaman: ok sure thanks zcorpan & Hixie for your time...
(12:07:24 PM) Hixie: (in case anyone got 500s changing annotations recently, that should be fixed now...)
(12:07:53 PM) zcorpan: np

[Mustafizur Rahaman( :rahaman)]

Considering the above discussion, I would prefer either of the following:
#1> Remove the suspicious code under consideration & see how many regressions we have
#2> Alternatively, as this is not being observed in any real websites & it is risky to touch the existing table layout code, we can live with the issue & mark the bug as WontFix.

Kling/Others, any thoughts on this please?

[Andreas Kling]

(In reply to comment #6)
> Considering the above discussion, I would prefer either of the following:
> #1> Remove the suspicious code under consideration & see how many regressions we have
> #2> Alternatively, as this is not being observed in any real websites & it is risky to touch the existing table layout code, we can live with the issue & mark the bug as WontFix.
> 
> Kling/Others, any thoughts on this please?

You should start by doing #1 and run the layout tests. It looks like a low-risk change to me, and if it doesn't break any tests, we can feel a little more confident that it doesn't affect anything relevant.

As this is a behavior change, the final patch should include a test that checks for the new behavior, i.e that border-color and border-style are actually relevant without overriding border-width explicitly.

[Mustafizur Rahaman( :rahaman)]

Created attachment 134043 [details]
Tentative patch for the issue

Adding the patch with the code changes to see if cr-linux reports any regression. While testing the patch on windows setup, observed some weird behavior, so uploading the patch. This is NOT the final version, so need not be reviewed.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


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

[Mustafizur Rahaman( :rahaman)]

Created attachment 134045 [details]
Tentative Patch v2

Fixed the style issue. This is NOT the final version, so need not be reviewed.

[Mustafizur Rahaman( :rahaman)]

Created attachment 134051 [details]
Tentative Patch v3

Fixed the style issue. This is NOT the final version, so need not be reviewed. Hopefully, i got it right this time..

[WebKit Review Bot]

Comment on attachment 134051 [details]
Tentative Patch v3

Attachment 134051 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12145585

New failing tests:
css1/cascade/important.html
css1/basic/class_as_selector.html
css1/box_properties/border_color.html
css1/box_properties/border_bottom.html
http/tests/inspector/inspect-element.html
accessibility/aria-disabled.html
css1/basic/grouping.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_bottom_width_inline.html
css1/classification/list_style_position.html
css1/basic/id_as_selector.html
css1/basic/inheritance.html
css1/box_properties/border_color_inline.html
css1/box_properties/border.html
css1/color_and_background/background_attachment.html
css1/classification/list_style_image.html
css1/cascade/cascade_order.html
css1/color_and_background/background.html
css1/classification/list_style.html
css1/conformance/forward_compatible_parsing.html
css1/box_properties/border_inline.html
css1/color_and_background/background_color.html
css1/color_and_background/background_image.html
css1/box_properties/border_bottom_inline.html
css1/basic/containment.html
css1/basic/contextual_selectors.html
css1/basic/comments.html
css1/classification/white_space.html
css1/classification/display.html
css1/classification/list_style_type.html

[WebKit Review Bot]

Created attachment 134072 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mustafizur Rahaman( :rahaman)]

Created attachment 134257 [details]
Final Patch v1

Proposed patch as per Kling's comment. I generated the expectation.txt only for gtk port & uploaded the patch. As this is style issue, the expectation file would be same for all other ports, so not sure whether I should copy the same file for other ports. Any suggestion?

[WebKit Review Bot]

Comment on attachment 134257 [details]
Final Patch v1

Attachment 134257 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12156055

New failing tests:
css1/cascade/important.html
css1/basic/class_as_selector.html
css1/box_properties/border_color.html
css1/box_properties/border_bottom.html
accessibility/aria-disabled.html
css1/basic/grouping.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_bottom_width_inline.html
css1/classification/list_style_position.html
css1/basic/id_as_selector.html
css1/basic/inheritance.html
css1/box_properties/border_color_inline.html
css1/box_properties/border.html
css1/box_properties/border_left.html
css1/color_and_background/background_attachment.html
css1/classification/list_style_image.html
css1/cascade/cascade_order.html
css1/color_and_background/background.html
css1/classification/list_style.html
css1/conformance/forward_compatible_parsing.html
css1/box_properties/border_inline.html
css1/color_and_background/background_color.html
css1/color_and_background/background_image.html
css1/box_properties/border_bottom_inline.html
css1/basic/containment.html
css1/basic/contextual_selectors.html
css1/basic/comments.html
css1/classification/white_space.html
css1/classification/display.html
css1/classification/list_style_type.html

[WebKit Review Bot]

Created attachment 134262 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Andreas Kling]

Comment on attachment 134257 [details]
Final Patch v1

This is incorrect, you are completely removing the support for styling table cells using attributes on the table element.
The issue is that we appear always fall into the NoBorders case in createSharedCellStyle(), regardless if any relevant attributes are present.

[Mustafizur Rahaman( :rahaman)]

Created attachment 134378 [details]
Final Patch v2

Based on Kling's review comments, updated the patch. The problem is that in the existing code, the UnsetRules switch case is returning NoBorder. As NoBorder is a valid option for other cases like NoneRules, GroupsRules so could not modify the logic for UnsetRules.

Therefore, added a new case DefaultBorders for UnsetRules & added the necessary code there.

[WebKit Review Bot]

Comment on attachment 134378 [details]
Final Patch v2

Attachment 134378 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12192021

New failing tests:
fast/table/border-collapsing/cached-change-colgroup-border-color.html
fast/table/border-collapsing/rtl-border-collapsing.html
fast/table/border-collapsing/bug14274.html
fast/repaint/table-cell-collapsed-border.html
fast/table/border-collapsing/cached-change-col-border-color.html
fast/table/border-collapsing/rtl-border-collapsing-vertical.html
fast/table/border-collapsing/cached-change-tbody-border-color.html
accessibility/aria-disabled.html
fast/frames/viewsource-attribute.html
fast/table/border-collapsing/cached-change-row-border-width.html
fast/frames/lots-of-objects.html
fast/table/border-collapsing/cached-change-tbody-border-width.html
fast/loader/text-document-wrapping.html
fast/repaint/table-section-repaint.html
fast/table/border-collapsing/cached-cell-append.html
css2.1/t0805-c5521-brdr-l-01-e.html
css2.1/t0805-c5520-brdr-b-01-e.html
fast/table/border-collapsing/equal-precedence-resolution-vertical.html
fast/table/border-collapsing/cached-change-colgroup-border-width.html
fast/frames/viewsource-on-image-file.html
http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html
fast/table/border-collapsing/equal-precedence-resolution.html
fast/table/unused-percent-heights.html
fast/table/border-collapsing/cached-change-col-border-width.html
fast/table/border-collapsing/cached-cell-remove.html
css2.1/t0805-c5519-brdr-r-01-e.html
fast/repaint/table-collapsed-border.html
fast/table/border-collapsing/cached-change-row-border-color.html

[WebKit Review Bot]

Created attachment 134462 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mustafizur Rahaman( :rahaman)]

Created attachment 134537 [details]
Final Patch v3

Modified the implementation to fix the layout tests regression. Also modified the DRT implementation to print the border attribute.

[Mustafizur Rahaman( :rahaman)]

Created attachment 134551 [details]
Final Patch v4

Fixed some more layout tests based on cr-linux bot result.

[WebKit Review Bot]

Comment on attachment 134551 [details]
Final Patch v4

Attachment 134551 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12193299

New failing tests:
css2.1/t0805-c5521-brdr-l-02-e.html
css2.1/t0805-c5520-brdr-b-01-e.html
css2.1/t0804-c5508-ipadn-b-03-b-a.html
css1/box_properties/border_right.html
css1/box_properties/border_bottom.html
css2.1/20110323/abspos-non-replaced-width-margin-000.htm
accessibility/aria-disabled.html
css2.1/t0804-c5509-padn-l-02-f.html
css2.1/t0805-c5521-ibrdr-l-00-a.html
css2.1/t170602-bdr-conflct-w-03-d.html
css2.1/t0805-c5520-brdr-b-00-a.html
css2.1/t170602-bdr-conflct-w-02-d.html
css1/box_properties/border_left.html
css1/box_properties/border_left_inline.html
css2.1/t0805-c5519-ibrdr-r-00-a.html
css2.1/t0805-c5519-brdr-r-02-e.html
css2.1/t0805-c5521-brdr-l-01-e.html
css2.1/t0805-c5521-brdr-l-00-a.html
css2.1/t0805-c5520-ibrdr-b-00-a.html
css2.1/t0805-c5519-brdr-r-00-a.html
css2.1/20110323/absolute-replaced-height-036.htm
css2.1/t0804-c5507-padn-r-02-f.html
css1/box_properties/border_bottom_inline.html
animations/cross-fade-border-image-source.html
css2.1/t170602-bdr-conflct-w-01-d.html
css2.1/t170602-bdr-conflct-w-04-d.html
css1/units/length_units.html
css2.1/t0805-c5519-brdr-r-01-e.html
css2.1/t040302-c61-phys-len-00-b.html
css1/box_properties/border_right_inline.html

[WebKit Review Bot]

Created attachment 134564 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexis Menard (darktears)]

Comment on attachment 134551 [details]
Final Patch v4

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

Does this patch actually do something? The code is commented. And the part you are adding stuff is just used for the LayoutTests.

> Source/WebCore/html/HTMLTableElement.cpp:498
> +        // style->setProperty(CSSPropertyBorderWidth, cssValuePool->createValue(3, CSSPrimitiveValue::CSS_PX));

??? 3?

[Mustafizur Rahaman( :rahaman)]

(In reply to comment #25)
> (From update of attachment 134551 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134551&action=review
> 
> Does this patch actually do something? The code is commented. And the part you are adding stuff is just used for the LayoutTests.
> 
> > Source/WebCore/html/HTMLTableElement.cpp:498
> > +        // style->setProperty(CSSPropertyBorderWidth, cssValuePool->createValue(3, CSSPrimitiveValue::CSS_PX));
> 
> ??? 3?

Yeah, for the use case the border-width is set to 0,so I am adding another switch case where I will do nothing so that the width is not set to zero. Have talked to Kling today, will submit the final patch along with the rebaseline results soon

[Ahmad Saleem]

Created attachment 460467 [details]
Safari matches Firefox but Chrome has strange dashes

I am not able to reproduce this bug in Safari 15.5 on macOS 12.4 from attached test case, since now the expected result is shown by test case where the table border is dashed of cyan color and it matches with Firefox in rendering (decent size dashes).

In case of Chrome - as seen in the attached screenshot, it has dashes but small size and frequent.

I am not clear on desired behavior of "dash' sizes but since the expected result of border color and border style being reflect on <td> is working. Can this be marked as "RESOLVED CONFIGURATION CHANGED". Thanks!