Skip to content

Conversation

@karlcow
Copy link
Member

@karlcow karlcow commented Dec 1, 2025

2fcae46

Safari/WebKit ignores vertical padding on a display:table element with 100% height
https://bugs.webkit.org/show_bug.cgi?id=155919
rdar://122587242

Reviewed by NOBODY (OOPS!).

This is a DRAFT PR.
It progresses one WPT test for tables, but it regresses two. So probably
there is more work to do.

When a CSS table (aka an HTML element with display: table) has a height
specified in percentage, the element should not ignore the padding
information and add it to the computed height. This will align WebKit
with Gecko and Blink.

* Source/WebCore/rendering/RenderTable.cpp:
(WebCore::RenderTable::convertStyleLogicalHeightToComputedHeight):

2fcae46

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win ✅ 🛠 ios-apple
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ❌ 🧪 win-tests ✅ 🛠 mac-apple
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe loading 🛠 vision-apple
❌ 🧪 ios-wk2-wpt ❌ 🧪 api-mac-debug ⏳ 🛠 wpe-cairo
✅ 🧪 api-ios ❌ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🛠 vision ❌ 🧪 mac-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ❌ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-wk2-stress ✅ 🛠 playstation
✅ 🛠 tv ❌ 🧪 mac-intel-wk2
✅ 🛠 tv-sim ✅ 🛠 mac-safer-cpp
✅ 🛠 watch
✅ 🛠 watch-sim

…h 100% height

https://bugs.webkit.org/show_bug.cgi?id=155919
rdar://122587242

Reviewed by NOBODY (OOPS!).

This is a DRAFT PR.
It progresses one WPT test for tables, but it regresses two. So probably
there is more work to do.

When a CSS table (aka an HTML element with display: table) has a height
specified in percentage, the element should not ignore the padding
information and add it to the computed height. This will align WebKit
with Gecko and Blink.

* Source/WebCore/rendering/RenderTable.cpp:
(WebCore::RenderTable::convertStyleLogicalHeightToComputedHeight):
@karlcow karlcow self-assigned this Dec 1, 2025
@karlcow karlcow added the Layout and Rendering For bugs with layout and rendering of Web pages. label Dec 1, 2025
@karlcow karlcow requested a review from alanbaradlay December 1, 2025 15:13
@karlcow
Copy link
Member Author

karlcow commented Dec 1, 2025

@karlcow karlcow requested a review from Ahmad-S792 December 1, 2025 15:17
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 1, 2025
@karlcow
Copy link
Member Author

karlcow commented Dec 1, 2025

It should be progressing and it does on my local build. I have to double check.
http://wpt.live/css/css-tables/table-has-box-sizing-border-box-002.html

@karlcow
Copy link
Member Author

karlcow commented Dec 2, 2025

In https://wpt.live/css/css-tables/tentative/td-box-sizing-002.html

It's interesting because we get new failures but also partial fixes.

<p>box-sizing: content-box; border-collapse: separate</p>
<table class="t" style="box-sizing:content-box;border-collapse: separate" data-expected-width="180" data-expected-height="180">
  <tbody>
    <td></td>
  </tbody>
</table>

And

<p>div with display:table; box-sizing: border-box; border-collapse: separate</p>
<div class="t" style="box-sizing:border-box;border-collapse: separate" data-expected-width="100" data-expected-height="100">
    <div class="cell"></div>
</div>

From left to right: STP 233, Firefox Nightly 147, Chrome Canary 144, MiniBrowser with the patch.
Screenshot 2025-12-02 at 09 29 56

@Ahmad-S792
Copy link
Contributor

Ahmad-S792 commented Dec 2, 2025

@karlcow - I think you need to look into RenderTable::layout as well because it adds borderAndSpacing twice there as well.

setLogicalHeight(logicalHeight() + borderAndPaddingBefore);

and if you notice failure of table-offset-* test, it is in height rather than width. So something in RenderTable::layout() also need adjusting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Layout and Rendering For bugs with layout and rendering of Web pages. merging-blocked Applied to prevent a change from being merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants