Bug 76664 – Incorrect positioning of floating pseudo-elements in table captions

• Component: CSS
• Status: RESOLVED FIXED

[Robert Hogan]

Floating :before and :after pseudo elements are not correctly positioned in table captions.

[Robert Hogan]

Created attachment 123190 [details]
Test Case

[Robert Hogan]

Like FF and Opera, table captions should expand to enclose overhanging floats in RenderBlock::expandsToEncloseOverhangingFloats(). Doing this causes multiple-captions-crash3.html to start crashing again though, so needs more investigation.

[Robert Hogan]

Allowing table captions to enclose overhanging floats regress multiple-captions-crash-3.html because the float in the <keygen> element doesn't get removed in markSiblingsWithFloatsForLayout() anymore. This happens because the logicalBottomForFloat() == the keygen element's logicalHeight(). Once markSiblingsWithFloatsForLayout() is broadened, it will be safe to allow captions to enclose overhanging floats here.

[Abhishek Arya]

Created attachment 123463 [details]
Float next sibling patch for bug 72981

Robert, does the crash still happen after this patch ? If not, we can combine your and mine patch in 72981 itself ?

[Abhishek Arya]

(In reply to comment #3)
> Allowing table captions to enclose overhanging floats regress multiple-captions-crash-3.html because the float in the <keygen> element doesn't get removed in markSiblingsWithFloatsForLayout() anymore. This happens because the logicalBottomForFloat() == the keygen element's logicalHeight(). Once markSiblingsWithFloatsForLayout() is broadened, it will be safe to allow captions to enclose overhanging floats here.

I remember hitting something similar. This should be because of the problem in this code. we layout all the captions at logicaltop 0, and then we adjust the caption height. So, floats get incorrectly added.

    for (unsigned i = 0; i < m_captions.size(); i++)
        m_captions[i]->layoutIfNeeded();

    // If any table section moved vertically, we will just repaint everything from that
    // section down (it is quite unlikely that any of the following sections
    // did not shift).
    bool sectionMoved = false;
    LayoutUnit movedSectionLogicalTop = 0;

    // FIXME: Collapse caption margin.
    if (!m_captions.isEmpty()) {
        for (unsigned i = 0; i < m_captions.size(); i++) {
            if (m_captions[i]->style()->captionSide() == CAPBOTTOM)
                continue;
            adjustLogicalHeightForCaption(m_captions[i]);

[Robert Hogan]

Created attachment 123480 [details]
Patch

[Robert Hogan]

(In reply to comment #5)
> (In reply to comment #3)
> > Allowing table captions to enclose overhanging floats regress multiple-captions-crash-3.html because the float in the <keygen> element doesn't get removed in markSiblingsWithFloatsForLayout() anymore. This happens because the logicalBottomForFloat() == the keygen element's logicalHeight(). Once markSiblingsWithFloatsForLayout() is broadened, it will be safe to allow captions to enclose overhanging floats here.
> 
> I remember hitting something similar. This should be because of the problem in this code. we layout all the captions at logicaltop 0, and then we adjust the caption height. So, floats get incorrectly added.

Yes, that's the problem. The attached fixes it - with the result that the floats no longer intrude on their siblings since they now allows fully enclose them. This should make all of these caption/float related crashes go away I think.

[Abhishek Arya]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > Allowing table captions to enclose overhanging floats regress multiple-captions-crash-3.html because the float in the <keygen> element doesn't get removed in markSiblingsWithFloatsForLayout() anymore. This happens because the logicalBottomForFloat() == the keygen element's logicalHeight(). Once markSiblingsWithFloatsForLayout() is broadened, it will be safe to allow captions to enclose overhanging floats here.
> > 
> > I remember hitting something similar. This should be because of the problem in this code. we layout all the captions at logicaltop 0, and then we adjust the caption height. So, floats get incorrectly added.
> 
> Yes, that's the problem. The attached fixes it - with the result that the floats no longer intrude on their siblings since they now allows fully enclose them. This should make all of these caption/float related crashes go away I think.

Yeah, i think my patch might not needed at all now since this was the root cause. I will check with ClusterFuzz once your patch lands.

[Julien Chaffraix]

Comment on attachment 123480 [details]
Patch

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

The logical looks good to me, I look forward to the updated patch.

> Source/WebCore/rendering/RenderBlock.cpp:1518
> +           || hasColumns() || isTableCell() || isFieldset() || isWritingModeRoot() || isRoot() || isTableCaption();

Nit: Put the isTableCaption() next to the isTableCell() for consistency.

> Source/WebCore/rendering/RenderTable.cpp:278
> +    // The margins may not be available but ensure the caption is at least located beneath any previous sibling caption
> +    // so that it does not mistakenly think any floats in the previous caption intrude into it.
>      caption->setLogicalLocation(IntPoint(caption->marginStart(), caption->marginBefore() + logicalHeight()));
> +    caption->layoutIfNeeded();
> +    // Apply the margins to the location now that they are definitely available from layout
> +    caption->setLogicalLocation(IntPoint(caption->marginStart(), caption->marginBefore() + logicalHeight()));

I think you could simplify this code further:

if (caption->needsLayout()) {
    // The margins may not be available but ensure the caption is at least located beneath any previous sibling caption...
    caption->setLogicalLocation(IntPoint(caption->marginStart(), caption->marginBefore() + logicalHeight()));
    caption->layout();
}

// Apply the margins to the location now that they are definitely available from layout
caption->setLogicalLocation(IntPoint(caption->marginStart(), caption->marginBefore() + logicalHeight()));

That would make it easier to see what is going on and avoid 2 calls to setLogicalHeight when one only is needed.

> Source/WebCore/rendering/RenderTable.cpp:344
> +    for (unsigned i = 0; i < m_captions.size(); i++) {
> +        if (m_captions[i]->style()->captionSide() == CAPBOTTOM)
> +            continue;
> +        layoutCaption(m_captions[i]);
> +    }

Why not keep those lines below? I don't think this justifies splitting the caption layout / logical height update logic.

> Source/WebCore/rendering/RenderTable.h:251
> +    void layoutCaption(RenderBlock*);

This should take a RenderTableCaption pointer now.

> LayoutTests/fast/table/caption-encloses-overhanging-float-expected.html:13
> +    <table>
> +    <div class="caption2">caption1<div class="fl">a</div></div>
> +    </table>

Could you please add the auto-generated rendering bits of your table here (row-group, row and cell) as that would show you are comparing against a RenderTableCell.

[Robert Hogan]

Created attachment 123599 [details]
Patch

[Julien Chaffraix]

Comment on attachment 123599 [details]
Patch

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

> Source/WebCore/rendering/RenderTable.cpp:277
> +        caption->layoutIfNeeded();

It should be caption->layout(); directly as you know you need layout in this branch.

> Source/WebCore/rendering/RenderTable.h:251
> +    void layoutCaptionIfNeeded(RenderTableCaption*);

layoutCaptionIfNeeded is definitely a misnomer here (look at how the *IfNeeded() functions are implemented and you deviate from the pattern).

I really prefer layoutCaption to name the function but here are some alternatives if you want to avoid the word 'layout': placeCaption(), correctCaptionPosition() or adjustCaptionPosition()

> LayoutTests/fast/table/caption-encloses-overhanging-float.html:10
> +    <table>

Nit: add a comment with this bug URL, possibly a description of the expected output (like 'you should see no red')

[Abhishek Arya]

Robert, today (7pm) is the last day for our merges to m17. Can you please commit it today with Julien's nits, we dont want to ship m17 with this regression. Thank you very much.

[Robert Hogan]

Committed r105768: <http://trac.webkit.org/changeset/105768>