Bug 88066 – Make RenderTableSection - nodeAtPoint and paintObject reuse spanning logic

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Allan Sandfeld Jensen]

The recent patch for bug #86605 introduced new functions that handles writing-mode, text direction and calculates rows and columns intersected by a rectangle. This is somewhat similar to what the functions dirtiedRows() and dirtiedColumns() already does, and the code should be combined.

This will also solve a problem of dirtiedColumns() not handling right-to-left text.

[Allan Sandfeld Jensen]

Created attachment 145248 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 145969 [details]
Patch

[Julien Chaffraix]

Comment on attachment 145969 [details]
Patch

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

> Source/WebCore/rendering/RenderTableSection.cpp:1053
>  CellSpan RenderTableSection::dirtiedRows(const LayoutRect& damageRect) const

I would really have loved for dirtiedRows / dirtiedColumns to just die but it seems that we need to patch the CellSpan returned from spannedRows. Having different behaviors for hit-testing and painting is unfortunate and should make us wonder if we don't prefer a unified code path (less bug but also any improvement would improve both code path).

> Source/WebCore/rendering/RenderTableSection.cpp:1058
> +    CellSpan coveredRows = spannedRows(damageRect);

I don't think this is right: spannedRows takes a rect that is flipped to account for the section's direction. However the damage rect is in device coordinates which doesn't account for direction.

> Source/WebCore/rendering/RenderTableSection.cpp:1062
> +    // To repaint the border we might need to repaint first or last row even if they are not spanned themselves.
> +    if (coveredRows.start() >= m_rowPos.size() - 1 && m_rowPos[m_rowPos.size() - 1] + table()->outerBorderAfter() >= damageRect.y())
> +        --coveredRows.start();

Is your change working with vertical writing modes as m_rowPos is actually in the x direction? (which was why |before| needed to take the writing mode into account)

> Source/WebCore/rendering/RenderTableSection.cpp:1064
> +    if (!coveredRows.end() && m_rowPos[0] - table()->outerBorderBefore() <= damageRect.maxY())

This is not mixed direction aware AFAICT (e.g if table and table-section have opposite directions - see bug 87900). Could you add a FIXME about that?

[Julien Chaffraix]

Comment on attachment 145969 [details]
Patch

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

>> Source/WebCore/rendering/RenderTableSection.cpp:1064
>> +    if (!coveredRows.end() && m_rowPos[0] - table()->outerBorderBefore() <= damageRect.maxY())
> 
> This is not mixed direction aware AFAICT (e.g if table and table-section have opposite directions - see bug 87900). Could you add a FIXME about that?

Scratch that actually, I misread the direction (inline base direction is start / end) so no FIXME needed.

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (From update of attachment 145969 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145969&action=review
> 
> > Source/WebCore/rendering/RenderTableSection.cpp:1058
> > +    CellSpan coveredRows = spannedRows(damageRect);
> 
> I don't think this is right: spannedRows takes a rect that is flipped to account for the section's direction. However the damage rect is in device coordinates which doesn't account for direction.
> 
The variable name should probably have been updated, because the damage-rect is now pre-flipped.

> > Source/WebCore/rendering/RenderTableSection.cpp:1062
> > +    // To repaint the border we might need to repaint first or last row even if they are not spanned themselves.
> > +    if (coveredRows.start() >= m_rowPos.size() - 1 && m_rowPos[m_rowPos.size() - 1] + table()->outerBorderAfter() >= damageRect.y())
> > +        --coveredRows.start();
> 
> Is your change working with vertical writing modes as m_rowPos is actually in the x direction? (which was why |before| needed to take the writing mode into account)
> 
As far as I can tell this is the correct use. Look at outBorderLeft for instance, which selects the correct border-value based on writing-mode and direction.

[Allan Sandfeld Jensen]

Created attachment 147270 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 147270 [details]
Patch

Remove dirtiedRows and dirtiedColumns functions. Otherwise unchanged logic.

[WebKit Review Bot]

Comment on attachment 147270 [details]
Patch

Attachment 147270 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12943964

New failing tests:
css1/box_properties/padding_top_inline.html
css1/formatting_model/inline_elements.html
css2.1/20110323/replaced-intrinsic-ratio-001.htm
fast/table/table-display-types-strict.html
fast/loader/loadInProgress.html
fast/loader/unload-form-post-about-blank.html
fast/repaint/table-section-overflow.html
fast/css-generated-content/015.html
fullscreen/full-screen-zIndex.html
fast/table/border-collapsing/border-collapsing-head-foot-vertical.html
fast/css/box-shadow-and-border-radius.html
fast/repaint/float-overflow.html
http/tests/security/script-crossorigin-loads-correctly.html
fast/repaint/table-cell-vertical-overflow.html
fast/repaint/float-overflow-right.html
fast/css/min-width-with-spanned-cell-fixed.html
fast/repaint/overflow-flipped-writing-mode-table.html
css1/box_properties/border_style_inline.html
css1/box_properties/border_inline.html
fullscreen/full-screen-iframe-zIndex.html
fast/table/009.html
fast/borders/bidi-009a.html
fast/block/basic/quirk-percent-height-table-cell.html
css1/box_properties/border_top_inline.html
fast/text/whitespace/025.html

[WebKit Review Bot]

Created attachment 147366 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Allan Sandfeld Jensen]

(In reply to comment #9)
> Created an attachment (id=147366) [details]
> Archive of layout-test-results from ec2-cr-linux-02
> 
> The attached test failures were seen while running run-webkit-tests on the chromium-ews.
> Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

The regression is due to the change in fullTableColumnSpan. I missed the fact it calls size of table->columns() and not size of table->columnPositions(). Only columnPositions() is one larger than the number of columns.

[Allan Sandfeld Jensen]

Created attachment 148304 [details]
Patch

[Eric Seidel (no email)]

I'm excited about this patch, but I have another 600 bugs to go through from vacation-back-up.  If you don't have a review in the next 48 hours, feel encouraged to pick on me.

[Julien Chaffraix]

Comment on attachment 148304 [details]
Patch

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

Sorry for the back and forth.

> Source/WebCore/rendering/RenderTableSection.cpp:1107
> +    if (m_forceSlowPaintPathWithOverflowingCell) {

Note that this should apply also to hit-testing. We just handle it separately for the moment.

> Source/WebCore/rendering/RenderTableSection.cpp:1129
> +            --dirtiedColumns.start();

If the alternative is to inline dirtiedRows / dirtiedColumns in paintObject which is already massive, I prefer it if we don't remove the 2 functions. It was short sighted from me to ask that as I missed the patching needed to properly paint the first / last row and column.

[Allan Sandfeld Jensen]

(In reply to comment #13)
> (From update of attachment 148304 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148304&action=review
> 
> Sorry for the back and forth.
> 
> > Source/WebCore/rendering/RenderTableSection.cpp:1107
> > +    if (m_forceSlowPaintPathWithOverflowingCell) {
> 
> Note that this should apply also to hit-testing. We just handle it separately for the moment.
> 
I just checked. This should already be handled by hasOwerflowingCell() check in the top of nodeAtPoint.

> > Source/WebCore/rendering/RenderTableSection.cpp:1129
> > +            --dirtiedColumns.start();
> 
> If the alternative is to inline dirtiedRows / dirtiedColumns in paintObject which is already massive, I prefer it if we don't remove the 2 functions. It was short sighted from me to ask that as I missed the patching needed to properly paint the first / last row and column.

No problem. I think I already have a patch like that :)

[Allan Sandfeld Jensen]

Created attachment 148328 [details]
Patch

[Julien Chaffraix]

> > > Source/WebCore/rendering/RenderTableSection.cpp:1107
> > > +    if (m_forceSlowPaintPathWithOverflowingCell) {
> > 
> > Note that this should apply also to hit-testing. We just handle it separately for the moment.
> > 
> I just checked. This should already be handled by hasOwerflowingCell() check in the top of nodeAtPoint.

You are totally right, however the hit-testing code doesn't benefit from the overflowing cell optimization. This was the meaning of this comment (as in: we need to move this to the common spanning logic at some point) but it's non-trivial to fix. Sorry for being unclear.

[WebKit Review Bot]

Comment on attachment 148328 [details]
Patch

Clearing flags on attachment: 148328

Committed r120721: <http://trac.webkit.org/changeset/120721>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.