Bug 110063 – Ensure margins in table captions contributes to minimum table width

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Robert Hogan]

..

[Robert Hogan]

Created attachment 189144 [details]
Patch

[WebKit Review Bot]

Attachment 189144 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css2.1/20110323/margin-right-applies-to-015-expected.html', u'LayoutTests/css2.1/20110323/margin-right-applies-to-015.htm', u'LayoutTests/platform/chromium-linux/tables/mozilla_expected_failures/core/captions1-expected.png', u'LayoutTests/platform/chromium-linux/tables/mozilla_expected_failures/core/captions2-expected.png', u'LayoutTests/platform/chromium-win/tables/mozilla_expected_failures/core/captions1-expected.txt', u'LayoutTests/platform/chromium-win/tables/mozilla_expected_failures/core/captions2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderTable.cpp']" exit_code: 1
LayoutTests/platform/chromium-linux/tables/mozilla_expected_failures/core/captions2-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 189144 [details]
Patch

Attachment 189144 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16621932

New failing tests:
tables/mozilla_expected_failures/core/captions2.html
tables/mozilla_expected_failures/core/captions1.html
css2.1/20110323/margin-right-applies-to-015.htm

[WebKit Review Bot]

Comment on attachment 189144 [details]
Patch

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

New failing tests:
css2.1/20110323/margin-right-applies-to-015.htm

[Robert Hogan]

Created attachment 189254 [details]
Patch

[WebKit Review Bot]

Attachment 189254 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css2.1/20110323/margin-right-applies-to-015-expected.html', u'LayoutTests/css2.1/20110323/margin-right-applies-to-015.htm', u'LayoutTests/platform/chromium-linux/tables/mozilla_expected_failures/core/captions1-expected.png', u'LayoutTests/platform/chromium-linux/tables/mozilla_expected_failures/core/captions2-expected.png', u'LayoutTests/platform/chromium-win/tables/mozilla_expected_failures/core/captions1-expected.txt', u'LayoutTests/platform/chromium-win/tables/mozilla_expected_failures/core/captions2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderTable.cpp']" exit_code: 1
LayoutTests/platform/chromium-linux/tables/mozilla_expected_failures/core/captions2-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 189254 [details]
Patch

Attachment 189254 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16657127

New failing tests:
tables/mozilla_expected_failures/core/captions2.html
tables/mozilla_expected_failures/core/captions1.html

[Build Bot]

Comment on attachment 189254 [details]
Patch

Attachment 189254 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16646942

New failing tests:
tables/mozilla_expected_failures/core/captions2.html
tables/mozilla_expected_failures/core/captions1.html

[Radar WebKit Bug Importer]

<rdar://problem/120990942>

[Ahmad Saleem]

It compiles and progress WPT test case but it makes below legit failures:

tables/mozilla_expected_failures/core/captions2.html
tables/mozilla_expected_failures/core/captions1.html

Changes to make it compiles:

Webkit Source: https://github.com/WebKit/WebKit/blob/b2323ec6eaf613835bbaf8964d6e6d53b5310653/Source/WebCore/rendering/RenderTable.cpp#L947

and make `containingBlockLogicalWidthForContent` public - https://github.com/WebKit/WebKit/blob/b2323ec6eaf613835bbaf8964d6e6d53b5310653/Source/WebCore/rendering/RenderTableCaption.h#L40

It might be something about this FIXME: https://github.com/WebKit/WebKit/blob/b2323ec6eaf613835bbaf8964d6e6d53b5310653/Source/WebCore/rendering/RenderTable.cpp#L426 (// FIXME: Collapse caption margin.)

Change:


for (unsigned i = 0; i < m_captions.size(); i++)
        m_minPreferredLogicalWidth = std::max(m_minPreferredLogicalWidth, m_captions[i]->minPreferredLogicalWidth());
    m_maxPreferredLogicalWidth = std::max(m_maxPreferredLogicalWidth, m_minPreferredLogicalWidth);

To:

for (unsigned i = 0; i < m_captions.size(); i++) {
    LayoutUnit availableLogicalWidth = m_captions[i]->containingBlockLogicalWidthForContent();
    LayoutUnit marginStart = minimumValueForLength(m_captions[i]->style().marginStart(), availableLogicalWidth);
    LayoutUnit marginEnd = minimumValueForLength(m_captions[i]->style().marginEnd(), availableLogicalWidth);
    m_minPreferredLogicalWidth = std::max(m_minPreferredLogicalWidth, m_captions[i]->minPreferredLogicalWidth());
    m_maxPreferredLogicalWidth = std::max(m_maxPreferredLogicalWidth, m_minPreferredLogicalWidth);
}

[Ahmad Saleem]

```
for (unsigned i = 0; i < m_captions.size(); i++) {
        LayoutUnit captionMinWidth = m_captions[i]->minPreferredLogicalWidth();

        // Only add fixed margins during preferred width calculation
        auto& captionStyle = m_captions[i]->style();
        if (auto fixedMarginStart = captionStyle.marginStart().tryFixed())
            captionMinWidth += fixedMarginStart->resolveZoom(captionStyle.usedZoomForLength());
        if (auto fixedMarginEnd = captionStyle.marginEnd().tryFixed())
            captionMinWidth += fixedMarginEnd->resolveZoom(captionStyle.usedZoomForLength());

        m_minPreferredLogicalWidth = std::max(m_minPreferredLogicalWidth, captionMinWidth);
    }
    m_maxPreferredLogicalWidth = std::max(m_maxPreferredLogicalWidth, m_minPreferredLogicalWidth);
```

This fixes and compile and don't have to do `public` -> `private` and also noted that we need to rebase line these two:

tables/mozilla_expected_failures/core/captions2.html
tables/mozilla_expected_failures/core/captions1.html

I noticed images from post-change vs Blink current and noticed that our text was not getting contained and with above one, it does and match Blink, so it is just rebase line issue.

We need to import relevant WPT test though, which progresses.

[Ahmad Saleem]

Pull request: https://github.com/WebKit/WebKit/pull/54541

[EWS]

Committed 303642@main (0d389e7c6ac1): <https://commits.webkit.org/303642@main>

Reviewed commits have been landed. Closing PR #54541 and removing active labels.