Bug 87900 – Add support for direction on table row group with collapsing borders

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 145402 [details]
Properly support direction on row group, naming nits / forgotten tests welcome. Admire the new hotness\!

[Ojan Vafai]

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.

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 145882 [details]
Fixed naming, added descriptions.

[Ojan Vafai]

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.

[WebKit Review Bot]

Comment on attachment 145882 [details]
Fixed naming, added descriptions.

Clearing flags on attachment: 145882

Committed r119594: <http://trac.webkit.org/changeset/119594>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.