Bug 87900 - Add support for direction on table row group with collapsing borders
Summary: Add support for direction on table row group with collapsing borders
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on: 87902 88110
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-30 14:46 PDT by Julien Chaffraix
Modified: 2012-06-06 09:07 PDT (History)
6 users (show)

See Also:


Attachments
Test case - there should be no left table border, the other borders should be green (534 bytes, text/html)
2012-05-30 14:46 PDT, Julien Chaffraix
no flags Details
Properly support direction on row group, naming nits / forgotten tests welcome. Admire the new hotness\! (56.92 KB, patch)
2012-06-01 16:21 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Fixed naming, added descriptions. (61.75 KB, patch)
2012-06-05 15:32 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-05-30 14:46:04 PDT
Created attachment 144926 [details]
Test case - there should be no left table border, the other borders should be green

The current code doesn't handle direction on a table row group properly and we end up with bad borders (as don't pick up the right direction for resolving our collapsing borders).

See attached test case.
Comment 1 Julien Chaffraix 2012-06-01 16:21:18 PDT
Created attachment 145402 [details]
Properly support direction on row group, naming nits / forgotten tests welcome. Admire the new hotness\!
Comment 2 Ojan Vafai 2012-06-05 13:43:47 PDT
Comment on attachment 145402 [details]
Properly support direction on row group, naming nits / forgotten tests welcome. Admire the new hotness\!

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

R- just for the naming issues. Nice use of reftests!

> Source/WebCore/rendering/RenderTableSection.cpp:1426
> +void RenderTableSection::determineLogicalPositionForCell(RenderTableCell* cell, unsigned effectiveColumn) const

Maybe name this setLogicalPositionForCell? I'd expect "determine" to do some computation and then return a value.

> Source/WebCore/rendering/RenderTableSection.h:117
> +    bool hasMixedDirection() const

I don't think this method name is clear enough. It's not obvious that it compares against the table.

How about inverting the logical and calling this hasSameDirectionAsParentTable? or just hasSameDirectionAsTable?

> Source/WebCore/rendering/RenderTableSection.h:120
> +        TextDirection tableDirection = table()->style()->direction();
> +        return tableDirection != style()->direction();

Nit: I'd inline tableDirection here.

> LayoutTests/fast/table/table-rtl-section-ltr.html:21
> +    <p>Test for bug <a href="https://bugs.webkit.org/show_bug.cgi?id=87900">87900</a>: Add support for direction on table row group with collapsing borders</p>

Nit: I prefer the description to say what the test is actually testing (e.g. Test that borders are properly collapsed when a tbody has a different direction than it's parent table.).

I don't think in these cases that linking to the bug gives useful information beyond that description since it's not a tricky edge case for which you'd need to read the bug in order to understand what's going on.

Also, you can always find the bug number by looking at the commit history.

Finally, if we were ever to upstream these tests to the W3C, we'd have to strip all these.
Comment 3 Julien Chaffraix 2012-06-05 14:15:29 PDT
Comment on attachment 145402 [details]
Properly support direction on row group, naming nits / forgotten tests welcome. Admire the new hotness\!

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

>> Source/WebCore/rendering/RenderTableSection.cpp:1426
>> +void RenderTableSection::determineLogicalPositionForCell(RenderTableCell* cell, unsigned effectiveColumn) const
> 
> Maybe name this setLogicalPositionForCell? I'd expect "determine" to do some computation and then return a value.

FWIW it matches RenderBlock naming for consistency (determineLogicalLeftPositionForChild) but I am not set up on this name.

>> Source/WebCore/rendering/RenderTableSection.h:117
>> +    bool hasMixedDirection() const
> 
> I don't think this method name is clear enough. It's not obvious that it compares against the table.
> 
> How about inverting the logical and calling this hasSameDirectionAsParentTable? or just hasSameDirectionAsTable?

Totally right, I couldn't think of a nice name for this one: hasSameDirectionAsTable is fine (our parent is always a table so it would be redundant to put 'parent' in the name).

>> LayoutTests/fast/table/table-rtl-section-ltr.html:21
>> +    <p>Test for bug <a href="https://bugs.webkit.org/show_bug.cgi?id=87900">87900</a>: Add support for direction on table row group with collapsing borders</p>
> 
> Nit: I prefer the description to say what the test is actually testing (e.g. Test that borders are properly collapsed when a tbody has a different direction than it's parent table.).
> 
> I don't think in these cases that linking to the bug gives useful information beyond that description since it's not a tricky edge case for which you'd need to read the bug in order to understand what's going on.
> 
> Also, you can always find the bug number by looking at the commit history.
> 
> Finally, if we were ever to upstream these tests to the W3C, we'd have to strip all these.

I guess we have different views on that. I usually ask for a link to the bug as it's important if the test starts failing: it enables a very easy way for maintainers (who are not familiar with the code nor the test) to find the original author / reviewer (instead of blame which can be slow). Also it gives access to the discussion that happened on the bug which is sometimes more useful than the fix itself.

Let me add some descriptions.
Comment 4 Julien Chaffraix 2012-06-05 15:32:50 PDT
Created attachment 145882 [details]
Fixed naming, added descriptions.
Comment 5 Ojan Vafai 2012-06-05 15:44:18 PDT
Comment on attachment 145402 [details]
Properly support direction on row group, naming nits / forgotten tests welcome. Admire the new hotness\!

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

>>> Source/WebCore/rendering/RenderTableSection.cpp:1426
>>> +void RenderTableSection::determineLogicalPositionForCell(RenderTableCell* cell, unsigned effectiveColumn) const
>> 
>> Maybe name this setLogicalPositionForCell? I'd expect "determine" to do some computation and then return a value.
> 
> FWIW it matches RenderBlock naming for consistency (determineLogicalLeftPositionForChild) but I am not set up on this name.

Yeah, I think the RenderBlock naming is poor and we should not propagate it. :)

>>> LayoutTests/fast/table/table-rtl-section-ltr.html:21
>>> +    <p>Test for bug <a href="https://bugs.webkit.org/show_bug.cgi?id=87900">87900</a>: Add support for direction on table row group with collapsing borders</p>
>> 
>> Nit: I prefer the description to say what the test is actually testing (e.g. Test that borders are properly collapsed when a tbody has a different direction than it's parent table.).
>> 
>> I don't think in these cases that linking to the bug gives useful information beyond that description since it's not a tricky edge case for which you'd need to read the bug in order to understand what's going on.
>> 
>> Also, you can always find the bug number by looking at the commit history.
>> 
>> Finally, if we were ever to upstream these tests to the W3C, we'd have to strip all these.
> 
> I guess we have different views on that. I usually ask for a link to the bug as it's important if the test starts failing: it enables a very easy way for maintainers (who are not familiar with the code nor the test) to find the original author / reviewer (instead of blame which can be slow). Also it gives access to the discussion that happened on the bug which is sometimes more useful than the fix itself.
> 
> Let me add some descriptions.

Fair enough.
Comment 6 WebKit Review Bot 2012-06-06 09:07:34 PDT
Comment on attachment 145882 [details]
Fixed naming, added descriptions.

Clearing flags on attachment: 145882

Committed r119594: <http://trac.webkit.org/changeset/119594>
Comment 7 WebKit Review Bot 2012-06-06 09:07:40 PDT
All reviewed patches have been landed.  Closing bug.