LibWeb: Propagate vertical-align to anonymous table wrapper boxes - #11064
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe table layout code now resets Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Libraries/LibWeb/Layout/Node.cpp (1)
1101-1115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the transfer-contract comment accurate.
Line 1104 says that all other non-inheritable values remain on the table box, but Line 1115 now transfers
vertical-alignto the wrapper. Update the comment to listvertical-alignas an exception.Suggested comment update
- // all other values of non-inheritable properties are used on the table box and not the table wrapper box. + // Other non-inheritable values are used on the table box and not the table wrapper box. + // `vertical-align` is also transferred to the wrapper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibWeb/Layout/Node.cpp` around lines 1101 - 1115, Update the transfer-contract comment in NodeWithStyle::transfer_table_box_computed_values_to_wrapper_computed_values to include vertical-align among the properties transferred to the table wrapper box, keeping the documented behavior consistent with the existing set_vertical_align call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Libraries/LibWeb/Layout/Node.cpp`:
- Around line 1101-1115: Update the transfer-contract comment in
NodeWithStyle::transfer_table_box_computed_values_to_wrapper_computed_values to
include vertical-align among the properties transferred to the table wrapper
box, keeping the documented behavior consistent with the existing
set_vertical_align call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c648f0d3-8edb-42a5-a645-49354dd603e0
📒 Files selected for processing (5)
Libraries/LibWeb/Layout/Node.cppTests/LibWeb/Ref/input/wpt-import/css/CSS2/borders/border-bottom-applies-to-014.xhtTests/LibWeb/Ref/input/wpt-import/css/CSS2/borders/border-bottom-color-applies-to-014.xhtTests/LibWeb/Ref/input/wpt-import/css/CSS2/borders/border-top-applies-to-014.xhtTests/LibWeb/Ref/input/wpt-import/css/CSS2/borders/border-top-color-applies-to-014.xht
Previously, a table's computed `vertical-align` value remained on the table box, where it had no effect. Inline tables were therefore always baseline aligned. The value is now moved to the anonymous table wrapper box, which participates in inline layout in place of the table box.
74fcad4 to
5676e18
Compare
2 participants
Previously, a table's computed
vertical-alignvalue remained on the table box, where it had no effect. Inline tables were therefore always baseline aligned. The value is now moved to the anonymous table wrapper box, which participates in inline layout in place of the table box.