Bug 101060

Summary: Fix the collapsing border code to handle mixed directionality at the row level
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: TablesAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, ojan, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 79272    
Attachments:
Description Flags
Proposed change: fixed the collapsing border code.
none
Updated change: fixed per Ojan's comment. none

Julien Chaffraix
Reported 2012-11-02 08:24:27 PDT
Our current border collapsing code wrongly ignores any direction queues below the table-row-group (section in WebKit speak). Other browsers (and the specification) don't have this limitation. This is another step towards fixing bug 79272 and properly supporting mixed direction at the cell level.
Attachments
Proposed change: fixed the collapsing border code. (20.09 KB, patch)
2012-11-02 09:02 PDT, Julien Chaffraix 		no flags
DetailsFormatted DiffDiff
Updated change: fixed per Ojan's comment. (20.24 KB, patch)
2012-11-02 11:02 PDT, Julien Chaffraix 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Julien Chaffraix
Comment 1 2012-11-02 09:02:02 PDT
Created attachment 172074 [details] Proposed change: fixed the collapsing border code.
Ojan Vafai
Comment 2 2012-11-02 09:32:39 PDT
Comment on attachment 172074 [details] Proposed change: fixed the collapsing border code. View in context: https://bugs.webkit.org/attachment.cgi?id=172074&action=review r- for the test. > Source/WebCore/ChangeLog:43 > + Updated these functios to work with mixed directionality. typo: functios > Source/WebCore/ChangeLog:56 > + This is a bug in our implementation: we used to return a non-zero width for inexistant borders (per CSS 2.1, > + 'border-style: off | hidden' should have a 0 width). I don't see a test for this fix. > Source/WebCore/rendering/RenderTableSection.cpp:1276 > + ASSERT_UNUSED(cell, cell->isFirstOrLastCellInRow()); this should just be an ASSERT now. > LayoutTests/fast/table/border-collapsing/table-ltr-rows-mixed-direction-expected.html:1 > +<!DOCTYPE> This doctype is quirksmode I believe. You should be using <!DOCTYPE html>.
Julien Chaffraix
Comment 3 2012-11-02 10:32:53 PDT
Comment on attachment 172074 [details] Proposed change: fixed the collapsing border code. View in context: https://bugs.webkit.org/attachment.cgi?id=172074&action=review >> Source/WebCore/ChangeLog:56 >> + 'border-style: off | hidden' should have a 0 width). > > I don't see a test for this fix. This is covered by LayoutTests/fast/table/border-collapsing/last-cell-left-border-hidden-table-ltr-section-rtl.html which was showing a diff due to having a 3px hidden border returning a width of 3px instead of 0px. >> LayoutTests/fast/table/border-collapsing/table-ltr-rows-mixed-direction-expected.html:1 >> +<!DOCTYPE> > > This doctype is quirksmode I believe. You should be using <!DOCTYPE html>. Good catch, dumb error. This should put the document in quirksmode per HTML5 parsing.
Ojan Vafai
Comment 4 2012-11-02 10:37:39 PDT
Comment on attachment 172074 [details] Proposed change: fixed the collapsing border code. View in context: https://bugs.webkit.org/attachment.cgi?id=172074&action=review >>> Source/WebCore/ChangeLog:56 >>> + 'border-style: off | hidden' should have a 0 width). >> >> I don't see a test for this fix. > > This is covered by LayoutTests/fast/table/border-collapsing/last-cell-left-border-hidden-table-ltr-section-rtl.html which was showing a diff due to having a 3px hidden border returning a width of 3px instead of 0px. I don't see a diff for this in this patch. Maybe you forgot to include it?
Julien Chaffraix
Comment 5 2012-11-02 10:55:16 PDT
Comment on attachment 172074 [details] Proposed change: fixed the collapsing border code. View in context: https://bugs.webkit.org/attachment.cgi?id=172074&action=review >>>> Source/WebCore/ChangeLog:56 >>>> + 'border-style: off | hidden' should have a 0 width). >>> >>> I don't see a test for this fix. >> >> This is covered by LayoutTests/fast/table/border-collapsing/last-cell-left-border-hidden-table-ltr-section-rtl.html which was showing a diff due to having a 3px hidden border returning a width of 3px instead of 0px. > > I don't see a diff for this in this patch. Maybe you forgot to include it? The test was showing a local diff before I touched CollapsedBorderValue::width. The diff was wrong and was what caused me to find this bug (ie that we could have a 3px hidden green border that would add 3px to our cell's logical width). The root cause of this change is that we are changing which borders are considered during resolution. FWIW our collapsing border resolution code is not very air-tight due to its complexity and this should make it more robust.
Julien Chaffraix
Comment 6 2012-11-02 11:02:35 PDT
Created attachment 172089 [details] Updated change: fixed per Ojan's comment.
WebKit Review Bot
Comment 7 2012-11-04 23:55:53 PST
Comment on attachment 172089 [details] Updated change: fixed per Ojan's comment. Clearing flags on attachment: 172089 Committed r133439: <http://trac.webkit.org/changeset/133439>
WebKit Review Bot
Comment 8 2012-11-04 23:55:57 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.