Skip to content

max-width do not work on tables #41759

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

max-width do not work on tables #41759

wants to merge 1 commit into from

Conversation

Ahmad-S792
Copy link
Contributor

@Ahmad-S792 Ahmad-S792 commented Mar 3, 2025 •
edited by webkit-early-warning-system
Loading

67060bc

max-width do not work on tables

https://bugs.webkit.org/show_bug.cgi?id=12396
rdar://96554687

Reviewed by NOBODY (OOPS!).

This aligns WebKit with Gecko / Firefox and Blink / Chromium.

Merge: https://chromium.googlesource.com/chromium/src.git/+/364480a952dc2eb710b26f82543e08bd4bac825b

The issue was that we would set the minimum logical width to
the 'width' but wouldn't apply the 'max-width' in this case.
Note that we shouldn't apply 'max-width' in all cases or else
we could end up having the content overflow its containing
table (which is not allowed per the specification).

This fix also address FIXME added by Blink by o ensure the
computed minimum preferred logical width never falls below
the intrinsic content width. Previously, the fixed table width
could override the content size, potentially violating layout
expectations and differing from other browsers.

Also we add condition that the quirk does not apply to flexbox.

* Source/WebCore/rendering/AutoTableLayout.cpp:
(WebCore::AutoTableLayout::applyPreferredLogicalWidthQuirks const):
* LayoutTests/fast/table/table-with-content-width-exceeding-max-width.html:
* LayoutTests/fast/table/table-width-exceeding-max-width.html:
* LayoutTests/fast/table/table-with-content-width-exceeding-max-width-expected.txt:
* LayoutTests/fast/table/table-width-exceeding-max-width-expected.txt:

67060bc

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

@Ahmad-S792 Ahmad-S792 self-assigned this Mar 3, 2025
@Ahmad-S792 Ahmad-S792 added the Tables For bugs specific to tables (both the DOM and rendering issues). label Mar 3, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Mar 3, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 82328a8)

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

@Ahmad-S792
Copy link
Contributor Author

image

and

image

^ Fixes both failing cases in Safari / WebKit and matches other browsers.

@Ahmad-S792 Ahmad-S792 marked this pull request as draft March 3, 2025 08:50
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 3, 2025
@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label Mar 3, 2025
@Ahmad-S792 Ahmad-S792 force-pushed the eng/max-width-do-not-work-on-tables branch from 82328a8 to 4359309 Compare March 3, 2025 09:39
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Mar 3, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 4359309)

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

@Ahmad-S792 Ahmad-S792 marked this pull request as ready for review March 3, 2025 10:16
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 3, 2025
@Ahmad-S792
Copy link
Contributor Author

TODO: Fix failing test case.

@karlcow
Copy link
Member

karlcow commented Mar 4, 2025

TODO: Fix failing test case.

https://wpt.fyi/results/css/css-flexbox/table-as-item-specified-width.html
https://wpt.live/css/css-flexbox/table-as-item-specified-width.html

<div style="display: flex;">
  <div style="
          display: table; 
          width: 500px; 
          height: 100px; 
          background: green; 
          flex: 0 0 100px;"></div>
</div>

@karlcow
Copy link
Member

karlcow commented Mar 7, 2025

TODO: Fix failing test case.

https://wpt.fyi/results/css/css-flexbox/table-as-item-specified-width.html https://wpt.live/css/css-flexbox/table-as-item-specified-width.html

The test was failing on chrome too in the past.
They fixed it here.
https://chromium-review.googlesource.com/c/chromium/src/+/2410596

@karlcow
Copy link
Member

karlcow commented Mar 7, 2025

There is a special check to change the behavior of flexbox when the child node is a table and only for row it seems. But the current code is doing something different it seems.

@Ahmad-S792
Copy link
Contributor Author

Ahmad-S792 commented Mar 7, 2025 •
edited
Loading

There is a special check to change the behavior of flexbox when the child node is a table and only for row it seems. But the current code is doing something different it seems.

Yes - I will look into over weekend. Right now, trying to sync some tests and do CSS2 fixes + one SVG Paint Server fix.

@Ahmad-S792 Ahmad-S792 marked this pull request as draft April 19, 2025 07:16
@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label Apr 19, 2025
@Ahmad-S792 Ahmad-S792 force-pushed the eng/max-width-do-not-work-on-tables branch from 4359309 to 02a28ee Compare April 19, 2025 07:18
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Apr 19, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 02a28ee)

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

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 19, 2025
@Ahmad-S792
max-width do not work on tables
67060bc 
https://bugs.webkit.org/show_bug.cgi?id=12396
rdar://96554687

Reviewed by NOBODY (OOPS!).

This aligns WebKit with Gecko / Firefox and Blink / Chromium.

Merge: https://chromium.googlesource.com/chromium/src.git/+/364480a952dc2eb710b26f82543e08bd4bac825b

The issue was that we would set the minimum logical width to
the 'width' but wouldn't apply the 'max-width' in this case.
Note that we shouldn't apply 'max-width' in all cases or else
we could end up having the content overflow its containing
table (which is not allowed per the specification).

This fix also address FIXME added by Blink by o ensure the
computed minimum preferred logical width never falls below
the intrinsic content width. Previously, the fixed table width
could override the content size, potentially violating layout
expectations and differing from other browsers.

Also we add condition that the quirk does not apply to flexbox.

* Source/WebCore/rendering/AutoTableLayout.cpp:
(WebCore::AutoTableLayout::applyPreferredLogicalWidthQuirks const):
* LayoutTests/fast/table/table-with-content-width-exceeding-max-width.html:
* LayoutTests/fast/table/table-width-exceeding-max-width.html:
* LayoutTests/fast/table/table-with-content-width-exceeding-max-width-expected.txt:
* LayoutTests/fast/table/table-width-exceeding-max-width-expected.txt:
@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label Jun 28, 2025
@Ahmad-S792 Ahmad-S792 force-pushed the eng/max-width-do-not-work-on-tables branch from 02a28ee to 67060bc Compare June 28, 2025 12:17
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 28, 2025 •
edited
Loading

EWS run on current version of this PR (hash 67060bc)

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

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 28, 2025
@Ahmad-S792
Copy link
Contributor Author

It is whack a mole - I fixed one and now another test failure showed up. Now onto next.

@karlcow
Copy link
Member

karlcow commented Jun 30, 2025

all my support.

@Ahmad-S792
Copy link
Contributor Author

@sammygill - Any idea - where should I look into Flexbox code to make it work? Any help / pointers?

alanbaradlay
alanbaradlay requested changes Jun 30, 2025
View reviewed changes
Source/WebCore/rendering/AutoTableLayout.cpp
Comment on lines +267 to +268
if (m_table->parent() && m_table->parent()->isFlexibleBoxIncludingDeprecated())
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not seem to be the right direction. This logic should be part of the flex layout; flex layout comes across a table as flex item and it tells the flex item, please do not apply your BFC quirks. (also this changes -webkit-box behavior, is that intentional?)

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

@alanbaradlay alanbaradlay alanbaradlay requested changes

@anttijk anttijk Awaiting requested review from anttijk

Assignees

@Ahmad-S792 Ahmad-S792

Labels
merging-blocked Applied to prevent a change from being merged Tables For bugs specific to tables (both the DOM and rendering issues).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ahmad-S792 @webkit-early-warning-system @karlcow @alanbaradlay @webkit-ews-buildbot