|
Description
Eric O'Connell
2005-07-12 10:47:36 PDT
I see the bug on the page listed here, so confirming. Seems like a pattern offset bug. It's possible mitz might know this area of the code. Created attachment 5338 [details]
Colspan above causes border to extend to other cells
This demonstrates what I think is the core issue here: the cell with colspan 2
on the first row adopts as its bottom border the top border of the first cell
on the second row.
Added HasReduction. I think this bug is related to bug 3820, in the very least because both seem to be pointing out errors in code relating to colspan. Another minimal testcase:
data:text/html;charset=utf-8,<!DOCTYPE html><html lang%3D"en"><head><meta charset%3D"UTF-8" /><style>table{ border-collapse: collapse; }td{ border-bottom: 1px dotted red; }</style></head><body><table><tr><td>Col 1</td><td>Col 2</td></tr><tr><td colspan%3D"2">Cell spanning 2 columns.</td></tr></table></body></html>
Getting rid of border-collapse:collapse fixes it.
Broken in Chrome 8 and Safari 5. Working in Firefox 4b9
Created attachment 98738 [details]
Minimal test case, border-collapse, two rows, no colspan
Any EVEN number in the padding will make the 2px dotted border solid.
Tested in Google Chrome 12.0.742.100 on WinXP.
A workaround for HTML authors is to try changing padding by one pixel. Created attachment 188751 [details] HTML, minimal test case, collapsed and thin dotted borders My specific encounter: In a table element with collapsed borders and thin dotted cell borders, the border between a spanning cell and every other one of its neighbor cells becomes solid. Reproduced on Win7 x64 with Webkit r131444 (16 Oct 2012), version 537.17. Did not occur for me using non-thin dotted cell borders. Also fails to reproduce at a few intermediate zoom levels. Attaching another minimal test case. The first two testcases seem similar to the issue specified in #5515 (https://bugs.webkit.org/show_bug.cgi?id=5515). Created attachment 194905 [details]
WIP - 1
(In reply to comment #10) > Created an attachment (id=194905) [details] > WIP - 1 > The patch does not work properly for cells with colspan. Also the cell corners where the borders overlap is not proper. Will upload another addressing these issues. Attachment 194905 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderTableCell.cpp']" exit_code: 1
Source/WebCore/rendering/RenderTableCell.cpp:1132: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 194905 [details] WIP - 1 Attachment 194905 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17336108 New failing tests: css2.1/20110323/border-conflict-element-001d.htm http/tests/cache/subresource-failover-to-network.html css2.1/t170602-bdr-conflct-w-19-d.html css2.1/t170602-bdr-conflct-w-25-d.html css2.1/t170602-bdr-conflct-w-03-d.html css2.1/20110323/border-conflict-style-079.htm css2.1/t170602-bdr-conflct-w-11-d.html css2.1/t170602-bdr-conflct-w-07-d.html css2.1/t170602-bdr-conflct-w-26-d.html css2.1/t170602-bdr-conflct-w-15-d.html css2.1/t170602-bdr-conflct-w-08-d.html css2.1/t170602-bdr-conflct-w-12-d.html css2.1/t170602-bdr-conflct-w-02-d.html css2.1/t170602-bdr-conflct-w-21-d.html css2.1/t170602-bdr-conflct-w-23-d.html css2.1/20110323/border-collapse-offset-002.htm css2.1/t170602-bdr-conflct-w-17-d.html css2.1/t170602-bdr-conflct-w-28-d.html css2.1/t170602-bdr-conflct-w-06-d.html css2.1/20110323/border-conflict-style-088.htm css2.1/t170602-bdr-conflct-w-16-d.html css2.1/t170602-bdr-conflct-w-24-d.html css2.1/t170602-bdr-conflct-w-13-d.html css2.1/t170602-bdr-conflct-w-01-d.html css2.1/t170602-bdr-conflct-w-04-d.html css2.1/t170602-bdr-conflct-w-18-d.html css2.1/t170602-bdr-conflct-w-27-d.html css2.1/t170602-bdr-conflct-w-14-d.html css2.1/t170602-bdr-conflct-w-05-d.html css2.1/t170602-bdr-conflct-w-22-d.html Created attachment 195299 [details]
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment on attachment 194905 [details] WIP - 1 Attachment 194905 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17324194 New failing tests: css2.1/20110323/border-conflict-element-001d.htm Created attachment 195433 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Comment on attachment 194905 [details] WIP - 1 Attachment 194905 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17311557 New failing tests: media/video-zoom.html css2.1/20110323/border-conflict-element-001d.htm Created attachment 195550 [details]
Archive of layout-test-results from webkit-ews-01 for mac-future
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01 Port: mac-future Platform: Mac OS X 10.8.2
Comment on attachment 194905 [details] WIP - 1 Attachment 194905 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17311581 New failing tests: css2.1/20110323/border-conflict-element-001d.htm Created attachment 195559 [details]
Archive of layout-test-results from webkit-ews-05 for mac-future
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05 Port: mac-future Platform: Mac OS X 10.8.2
Created attachment 204585 [details]
Patch
Comment on attachment 204585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204585&action=review > Source/WebCore/rendering/RenderTableCell.cpp:1113 > +bool RenderTableCell::alignTopBottomBorderPaintRect(RenderTableCell* cell, int& topYOffset, int& bottomYOffset) This should just be a method on RenderTableCell instead of passing it as the first argument. It doesn't use any local state. cell->align...(...); > Source/WebCore/rendering/RenderTableCell.cpp:1120 > + bottomYOffset = max<int>(bottomYOffset, (bottom)); extra parens. > Source/WebCore/rendering/RenderTableCell.cpp:1122 > + bShouldDrawBorder = false; return false; > Source/WebCore/rendering/RenderTableCell.cpp:1123 > + return bShouldDrawBorder; return true; > Source/WebCore/rendering/RenderTableCell.cpp:1163 > + int bottomYOffsetLeft = bottomWidth, bottomYOffsetRight = bottomYOffsetLeft; I think we usually do one variable init per line, but I don't know if the style is specific about it. > Source/WebCore/rendering/RenderTableCell.cpp:1171 > + bool shouldDrawLeftBorder = true, shouldDrawRightBorder = true; Same deal. > Source/WebCore/rendering/RenderTableCell.cpp:1174 > + if (top->colSpan() > 1 || this->colSpan() > 1) It's weird that one returns a boolean and one doesn't. > Source/WebCore/rendering/RenderTableCell.cpp:1182 > + shouldDrawLeftBorder = alignTopBottomBorderPaintRect(left, topYOffsetLeft, bottomYOffsetLeft); if (left) left->alignTopBottom...(...); Created attachment 205465 [details]
Patch
Comment on attachment 205465 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205465&action=review Looking at the layout test it's not clear if this is really better, you paint half blocks in the corners now which looks bad. I do see in Safari that if you zoom in the test case the borders will become dashes and sometimes even a solid line. Maybe you want the layout test to adjust the zoom? > Source/WebCore/rendering/RenderTableCell.cpp:1106 > + const RenderStyle* styleForTopCell = this->styleForCellFlow(); this-> is not needed anywhere in here. > Source/WebCore/rendering/RenderTableCell.cpp:1118 > + const RenderStyle* styleForBottomCell = this->styleForCellFlow(); Ditto. > Source/WebCore/rendering/RenderTableCell.cpp:1166 > + int topYOffsetLeft = topWidth; These declarations are a bit weird, why is topWidth the same as topYOffsetLeft ? > Source/WebCore/rendering/RenderTableCell.cpp:1186 > + if (bottom) I'd just merge all these declarations: if (RenderTableCell* bottom = table()->cellBelow(this)) bottom->... > LayoutTests/fast/table/border-collapsing/dotted-collapsed-border.html:1 > +<html> Add a doctype unless you need quirks mode. Created attachment 206225 [details]
Patch
Comment on attachment 206225 [details] Patch Attachment 206225 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/949125 New failing tests: fullscreen/full-screen-iframe-with-max-width-height.html Created attachment 206228 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
(In reply to comment #24) > (From update of attachment 205465 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205465&action=review > > Looking at the layout test it's not clear if this is really better, you paint half blocks in the corners now which looks bad. I do see in Safari that if you zoom in the test case the borders will become dashes and sometimes even a solid line. Maybe you want the layout test to adjust the zoom? > Let me explain a little abt the issue.. Lets take the layout testcase as the example. The collapse border(with the issue) are drawn as follows: 1) The top border of the lower cell is drawn. The start of the border is from the left extreme i.e from the left most point till which the left border extends. This translates to start X(top border)S1 = current cell start X - CellLeftWidth/2; end X (top border)E1 = current cell end X - CellRightWidth/2; 2) The bottom border of the top cell is drawn to overlap the previously drawn border. The start and end points are same as above. Lets name the start and end points as S2 and E2. The problem occurs when S1 != S2 and/or E1 != E2. The problem becomes more and more visible when zoomed. The fix(only for dotted border case) is that we try to keep S1 = S2 and E1 = E2 by considering the left and right width of both the top and bottom cells which share the collapsed border. > > Source/WebCore/rendering/RenderTableCell.cpp:1106 > > + const RenderStyle* styleForTopCell = this->styleForCellFlow(); > > this-> is not needed anywhere in here. > > > Source/WebCore/rendering/RenderTableCell.cpp:1118 > > + const RenderStyle* styleForBottomCell = this->styleForCellFlow(); > > Ditto. > done. > > Source/WebCore/rendering/RenderTableCell.cpp:1166 > > + int topYOffsetLeft = topWidth; > > These declarations are a bit weird, why is topWidth the same as topYOffsetLeft ? > Wen drawing the left border the starts from top most point where top border of the cell is drawn. > > Source/WebCore/rendering/RenderTableCell.cpp:1186 > > + if (bottom) > > I'd just merge all these declarations: > > if (RenderTableCell* bottom = table()->cellBelow(this)) > bottom->... > Done > > LayoutTests/fast/table/border-collapsing/dotted-collapsed-border.html:1 > > +<html> > > Add a doctype unless you need quirks mode. > Done Comment on attachment 206225 [details] Patch Clearing flags on attachment: 206225 Committed r153510: <http://trac.webkit.org/changeset/153510> All reviewed patches have been landed. Closing bug. |