| Summary: | border of <td> not displayed when border-color and border-style are used in CSS | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | jasneet <jasneet> | ||||||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||
| Status: | RESOLVED CONFIGURATION CHANGED | ||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | ahmad.saleem792, ap, dglazkov, jasneet, kbolisetty, kling, menard, mrahaman, rniwa, webkit.review.bot | ||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | HasReduction | ||||||||||||||||||||||||||||
| Version: | 525.x (Safari 3.2) | ||||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||||
| OS: | Windows XP | ||||||||||||||||||||||||||||||
| URL: | http://www.tizag.com/cssT/border.php | ||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
|
Description
jasneet
2009-03-05 17:10:20 PST
Created attachment 28340 [details]
reduced testcase
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?
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... 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. (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 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? (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. 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.
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.
Created attachment 134045 [details]
Tentative Patch v2
Fixed the style issue. This is NOT the final version, so need not be reviewed.
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..
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 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
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?
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 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
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.
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.
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 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
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.
Created attachment 134551 [details]
Final Patch v4
Fixed some more layout tests based on cr-linux bot result.
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 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
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? (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 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!
|