Bug 76664 - Incorrect positioning of floating pseudo-elements in table captions
Summary: Incorrect positioning of floating pseudo-elements in table captions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords:
Depends on: 72981
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-19 13:59 PST by Robert Hogan
Modified: 2012-01-24 11:54 PST (History)
7 users (show)

See Also:


Attachments
Test Case (367 bytes, text/html)
2012-01-19 14:00 PST, Robert Hogan
no flags Details
Float next sibling patch for bug 72981 (2.31 KB, application/octet-stream)
2012-01-21 17:10 PST, Abhishek Arya
no flags Details
Patch (7.75 KB, patch)
2012-01-22 09:47 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (7.42 KB, patch)
2012-01-23 12:42 PST, Robert Hogan
jchaffraix: review+
jchaffraix: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2012-01-19 13:59:20 PST
Floating :before and :after pseudo elements are not correctly positioned in table captions.
Comment 1 Robert Hogan 2012-01-19 14:00:06 PST
Created attachment 123190 [details]
Test Case
Comment 2 Robert Hogan 2012-01-21 15:58:20 PST
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.
Comment 3 Robert Hogan 2012-01-21 17:06:50 PST
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.
Comment 4 Abhishek Arya 2012-01-21 17:10:46 PST
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 ?
Comment 5 Abhishek Arya 2012-01-21 17:26:46 PST
(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]);
Comment 6 Robert Hogan 2012-01-22 09:47:03 PST
Created attachment 123480 [details]
Patch
Comment 7 Robert Hogan 2012-01-22 09:51:21 PST
(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.
Comment 8 Abhishek Arya 2012-01-22 09:57:56 PST
(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.
Comment 9 Julien Chaffraix 2012-01-23 12:11:35 PST
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.
Comment 10 Robert Hogan 2012-01-23 12:42:55 PST
Created attachment 123599 [details]
Patch
Comment 11 Julien Chaffraix 2012-01-24 05:20:29 PST
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')
Comment 12 Abhishek Arya 2012-01-24 10:51:09 PST
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.
Comment 13 Robert Hogan 2012-01-24 11:54:51 PST
Committed r105768: <http://trac.webkit.org/changeset/105768>