The testcase at https://bug717854.bugzilla.mozilla.org/attachment.cgi?id=588310 should show a red border between the A and B and likewise between the D and F. The relevant part of the HTML5 spec is in http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#tables and says: For the purposes of the CSS table model, the col element is expected to be treated as if it was present as many times as its span attribute specifies.
Bugzilla returns a ton of results for "border span". Is this bug new? I suspect that no one attempted to clean up those (I certainly didn't, due to limited understanding of how these work).
I did search before filing. All the bugs I saw had to do with colspan/rowspan on cells.
Created attachment 161890 [details] Patch
Comment on attachment 161890 [details] Patch Attachment 161890 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13740120
Created attachment 161966 [details] Patch
Comment on attachment 161966 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161966&action=review > Source/WebCore/rendering/RenderTableCell.cpp:458 > + // Next, irrespective of whether it is a spanned col or not, we apply its start border. It would be nice to quote HTML5 here to know why we apply the col every time. > Source/WebCore/rendering/RenderTableCell.cpp:459 > + result = chooseBorder(result, CollapsedBorderValue(colElt->style()->borderStart(), includeColor ? colElt->style()->visitedDependentColor(startColorProperty) : Color(), BCOL)); The order in which you apply the borders here is the opposite of what CSS uses (column should win over column-group per 17.6.2.1 point 4). You should apply |colElt| first and then find its enclosingColumnGroup to apply the border. This should also be tested. > Source/WebCore/rendering/RenderTableCell.cpp:475 > + result = chooseBorder(CollapsedBorderValue(colElt->style()->borderEnd(), includeColor ? colElt->style()->visitedDependentColor(endColorProperty) : Color(), BCOL), result); result should be first as we return the first parameter in case of equivalent borders. The code is not really consistent here unfortunately and we should try to avoid adding more latent bugs. It should be easily testable using equivalent borders of different color. > Source/WebCore/rendering/RenderTableCell.cpp:573 > + if (RenderTableCol* enclosingColumnGroup = colElt->enclosingColumnGroupIfAdjacentBefore()) { Same remark about the ordering here too. > LayoutTests/ChangeLog:11 > + Modified the result of the existing test case which consists of a table with > + col and colgroup specified. I have a hard time understand what changed. None of the borders have changed, yet your element is bigger. Could you explain what the change is and why it occurs? > LayoutTests/ChangeLog:15 > + * fast/table/border-collapsing/collapsed-border-with-col-colgroup-span-expected.png: Added. > + * fast/table/border-collapsing/collapsed-border-with-col-colgroup-span-expected.txt: Added. > + * fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html: Added. Could it be possible to change this into a ref-test? > LayoutTests/fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html:26 > +<col span=3 style="border: 1px solid red;"></col> red should be reserved for failure.
Created attachment 165260 [details] Patch
Comment on attachment 165260 [details] Patch Attachment 165260 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13986044 New failing tests: fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html
Comment on attachment 165260 [details] Patch Attachment 165260 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13985153 New failing tests: fast/table/border-collapsing/cached-change-colgroup-border-color.html WebFilterOperationsTest.saveAndRestore fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html fast/table/border-collapsing/cached-change-colgroup-border-width.html
Created attachment 165350 [details] Patch
(In reply to comment #6) > (From update of attachment 161966 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161966&action=review Hi Julien, thank-you for the review. Have made amends to handle most of the review comments except for the following two: > > > Source/WebCore/rendering/RenderTableCell.cpp:475 > > + result = chooseBorder(CollapsedBorderValue(colElt->style()->borderEnd(), includeColor ? colElt->style()->visitedDependentColor(endColorProperty) : Color(), BCOL), result); > > result should be first as we return the first parameter in case of equivalent borders. The code is not really consistent here unfortunately and we should try to avoid adding more latent bugs. It should be easily testable using equivalent borders of different color. 'result' should be the second param here as when considering the preceeding column's end border, as per the specification, the left most should be given higher priority. Also, have added a new test (last table in the added testcase) which highlights this issue. If result were to be given higher priority, the borders for the last table come as red, light brown, green and black instead of transparent/invisible, light green, dark green and black. > > > LayoutTests/ChangeLog:15 > > + * fast/table/border-collapsing/collapsed-border-with-col-colgroup-span-expected.png: Added. > > + * fast/table/border-collapsing/collapsed-border-with-col-colgroup-span-expected.txt: Added. > > + * fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html: Added. > > Could it be possible to change this into a ref-test? I tried, but found it difficult to achieve the same results w/o using collapsed borders.
Comment on attachment 165350 [details] Patch Attachment 165350 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13992662 New failing tests: fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html
Created attachment 166250 [details] Patch
Added another patch, making the layout testcase image only and platform specific.
Comment on attachment 166250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166250&action=review > Source/WebCore/ChangeLog:8 > + As per the specifications we should apply a col element's border as if Please say which specification as it is important: tables are defined by several specifications: HTML (4 and 5) and CSS 2.1. Ideally, you should quote it somewhere (the code is usually the best place but not if we have to duplicate it). > Source/WebCore/ChangeLog:19 > + Borders from col and it's enclosing colgroup element should be handled typo: its enclosing colgroup. > Source/WebCore/rendering/RenderTableCell.cpp:477 > + // The |colElt| is a column group, in which case we apply it's start border but only if it typo: its start border (this error occurs in other comments but I didn't repeat it for each mistake) > Source/WebCore/rendering/RenderTableCell.cpp:501 > + if (RenderTableCol* colElt = table->colElement(col() -1, &startColEdge, &endColEdge)) { Let's fix the style when we move code (there should be a space after the '-'). > Source/WebCore/rendering/RenderTableCell.cpp:603 > + // If the current element is a colgroup then we apply it's start border only if it is the first > + // colgroup (considering spanned colgroups). I really wonder how we could make those comments more digest as they are pretty repetitive (yet a bit different different for each function) and could be condensed & add more value. My take below. > Source/WebCore/rendering/RenderTableCell.cpp:605 > + result = chooseBorder(result, CollapsedBorderValue(colElt->borderAdjoiningCellBefore(this), includeColor ? colElt->style()->visitedDependentColor(startColorProperty) : Color(), BCOLGROUP)); // This case is a colgroup without any col, we only compute it if it is adjacent to the cell's edge. > Source/WebCore/rendering/RenderTableCell.cpp:608 > + } else if (!colElt->isTableColumnGroup()) { You probably want to use colElt->isTableColumn() instead of the negative (not isTableColumnGroup). > Source/WebCore/rendering/RenderTableCell.cpp:609 > + // First, apply the start border of the |colElt|, irrespective of whether it is spanned or not. // Resolve the collapsing border against the col's border ignoring any 'span' per HTML5. > Source/WebCore/rendering/RenderTableCell.cpp:614 > + // Next, if the element has a parent colgroup then it's start border should be applied > + // but only if it is the first col in that colgroup (i.e. considering spanned cols within a colgroup). // If we have a parent colgroup, resolve the border only if it is adjacent to the cell. > LayoutTests/ChangeLog:19 > + Existing tests modified. This is because previously, while computing collpased > + start border, we did not take the preceeding col's enclosing colgroup's end border > + into consideration. While computing the collapsed start border, only the preceeding > + col element's end border was considered. > + > + With this fix, for the above two tests, the last col's width now changes due to > + the border being applied to it (the preceeding col's enclosing colgroup's end border) > + which causes the table's width to change. Good catch. Worth mentioning that the cell's are growing by half the border's width, which is expected. > LayoutTests/ChangeLog:25 > + New image only test added for verifying the behavior of collapsed borders with col > + and colgroup span. Bad wrapping. > LayoutTests/fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html:27 > +<!-- This table has a col with span. Individual borders for the spanned cells should be painted. --> The descriptions here are really difficult to make sense of. If you don't know the collapsing border algorithm (which isn't the case for most people), you have a hard time saying if the output is expected. > LayoutTests/fast/table/border-collapsing/collapsed-border-with-col-colgroup-span.html:40 > +<div id="space"></div> This case could probably use some red for anything that shouldn't be shown (like .bordered's border-right as it should be disabled by the next column's border, with the exception of the last col that would need an extra style).
Created attachment 167480 [details] Patch
(In reply to comment #15) > (From update of attachment 166250 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166250&action=review > Thanks for the review Julien. Have made the changes as per your comments and uploaded another patch. Apologize for the silly "it's" mistake (among others). Have nailed it in my head now :) and thank-you for your help with re-framing the comments.
Comment on attachment 167480 [details] Patch We seems to be repeating much of this code 4 times. Is there no way to share it?
Comment on attachment 167480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167480&action=review I will fix the patch for landing. > We seems to be repeating much of this code 4 times. Is there no way to share it? That would be neat but I don't see a good way to do that: each repetition is indeed similar but also slightly different from the other ones. This makes sharing difficult. The code is also confusingly complex due to equal border resolution (all other parameters being equals, the start-most border wins) > Source/WebCore/rendering/RenderTableCell.cpp:532 > + // we apply its start border. This is as per the specification which states that HTML5 instead of "the specification" here. This has an importance as CSS tables don't have the concept of spanning cell. > Source/WebCore/rendering/RenderTableCell.cpp:539 > + // Next, apply the start border of the enclosing colgroup but only if it is > + // adjacent to the cell's edge. This could be on one line. > Source/WebCore/rendering/RenderTableCell.cpp:632 > + // This is as per the specification which states that "For the purposes of the CSS table model, Same comment here. > Source/WebCore/rendering/RenderTableCell.cpp:639 > + // Next, if it has a parent colgroup then we apply its end border but only if it is > + // adjacent to the cell. Ditto. > LayoutTests/ChangeLog:12 > + Existing tests modified. This is because previously, while computing collpased Typo: collpased.
Created attachment 169288 [details] Patch for landing
Comment on attachment 169288 [details] Patch for landing Clearing flags on attachment: 169288 Committed r131671: <http://trac.webkit.org/changeset/131671>
All reviewed patches have been landed. Closing bug.
(In reply to comment #19) > (From update of attachment 167480 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167480&action=review > > I will fix the patch for landing. > Thanks for the review Julien and also for landing the patch.