Skip to content

[Button Layout] buttons should not have align-items: flex-start by default #42166

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
Merged
webkit-commit-queue merged 1 commit into from
Mar 10, 2025
Merged

[Button Layout] buttons should not have align-items: flex-start by default #42166

webkit-commit-queue merged 1 commit into from
Mar 10, 2025

Conversation

@Ahmad-S792
Copy link
Contributor

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

d12e448

[Button Layout] `buttons` should not have `align-items: flex-start` by default

https://bugs.webkit.org/show_bug.cgi?id=289441
rdar://146615626

Reviewed by Tim Nguyen.

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

Merge: https://chromium-review.googlesource.com/c/chromium/src/+/6013405

This patch removes unneeded `align-items: flex-start`, since it is leftover
from our implementation of `button` as flex containers in 228400@main.

* Source/WebCore/css/html.css:
(input:is([type="button"], [type="submit"], [type="reset"]), input[type="file"]::file-selector-button, button):
* LayoutTests/imported/w3c/web-platform-tests/html/rendering/widgets/button-layout/flex-expected.txt: Rebaselined
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/html/rendering/widgets/button-layout/flex-expected.txt: Removed.
* LayoutTests/platform/ios/imported/w3c/web-platform-tests/html/rendering/widgets/button-layout/flex-expected.txt: Removed.

Canonical link: https://commits.webkit.org/291883@main

f553e7e

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
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 self-assigned this Mar 9, 2025
@Ahmad-S792 Ahmad-S792 added the CSS Cascading Style Sheets implementation label Mar 9, 2025
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on previous version of this PR (hash 5e862fe)

Details

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 loading 🛠 jsc-armv7
✅ 🛠 tv-sim loading 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

nt1m
nt1m approved these changes Mar 9, 2025
View reviewed changes
Copy link
Member

@nt1m nt1m left a comment

Choose a reason for hiding this comment

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

Can you remove this too: https://searchfox.org/wubkat/rev/662cdccd8338a60684dac2fc6e14d26305e5db8a/Source/WebCore/css/html.css#1127 ?

The select children can't really be laid out as a flex items as it stands, so that rule does nothing.

@Ahmad-S792 Ahmad-S792 force-pushed the eng/Button-Layout-buttons-should-not-have-align-items-flex-start-by-default branch from 5e862fe to 47f05c1 Compare March 10, 2025 00:10
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on previous version of this PR (hash 47f05c1)

Details

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 force-pushed the eng/Button-Layout-buttons-should-not-have-align-items-flex-start-by-default branch from 47f05c1 to 8a510f0 Compare March 10, 2025 00:18
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on previous version of this PR (hash 8a510f0)

Details

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 Mar 10, 2025
@nt1m
Copy link
Member

nt1m commented Mar 10, 2025

Oh, the select test is still failing because: https://searchfox.org/wubkat/rev/662cdccd8338a60684dac2fc6e14d26305e5db8a/Source/WebCore/css/html.css#1119,1134

Might be worth just landing button here, and then removing both flex-start and center from select in a separate commit.

@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label Mar 10, 2025
@Ahmad-S792 Ahmad-S792 force-pushed the eng/Button-Layout-buttons-should-not-have-align-items-flex-start-by-default branch from 8a510f0 to dda65ff Compare March 10, 2025 05:24
@webkit-early-warning-system
Copy link
Collaborator

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

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

Details

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 loading 🛠 playstation
✅ 🛠 tv   🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 force-pushed the eng/Button-Layout-buttons-should-not-have-align-items-flex-start-by-default branch from dda65ff to a987525 Compare March 10, 2025 05:26
@webkit-early-warning-system
Copy link
Collaborator

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

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

Details

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

nt1m
nt1m reviewed Mar 10, 2025
View reviewed changes
@Ahmad-S792 Ahmad-S792 force-pushed the eng/Button-Layout-buttons-should-not-have-align-items-flex-start-by-default branch from a987525 to f553e7e Compare March 10, 2025 06:40
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on current version of this PR (hash f553e7e)

Details

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
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 added safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks merge-queue Applied to send a pull request to merge-queue and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Mar 10, 2025
@Ahmad-S792
[Button Layout] buttons should not have align-items: flex-start b…
d12e448 
…y default

https://bugs.webkit.org/show_bug.cgi?id=289441
rdar://146615626

Reviewed by Tim Nguyen.

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

Merge: https://chromium-review.googlesource.com/c/chromium/src/+/6013405

This patch removes unneeded `align-items: flex-start`, since it is leftover
from our implementation of `button` as flex containers in 228400@main.

* Source/WebCore/css/html.css:
(input:is([type="button"], [type="submit"], [type="reset"]), input[type="file"]::file-selector-button, button):
* LayoutTests/imported/w3c/web-platform-tests/html/rendering/widgets/button-layout/flex-expected.txt: Rebaselined
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/html/rendering/widgets/button-layout/flex-expected.txt: Removed.
* LayoutTests/platform/ios/imported/w3c/web-platform-tests/html/rendering/widgets/button-layout/flex-expected.txt: Removed.

Canonical link: https://commits.webkit.org/291883@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Button-Layout-buttons-should-not-have-align-items-flex-start-by-default branch from f553e7e to d12e448 Compare March 10, 2025 09:05
@webkit-commit-queue
Copy link
Collaborator

webkit-commit-queue commented Mar 10, 2025

Committed 291883@main (d12e448): https://commits.webkit.org/291883@main

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

@webkit-commit-queue webkit-commit-queue merged commit d12e448 into WebKit:main Mar 10, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 10, 2025
@Ahmad-S792 Ahmad-S792 deleted the eng/Button-Layout-buttons-should-not-have-align-items-flex-start-by-default branch March 10, 2025 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nt1m nt1m nt1m approved these changes

Assignees

@Ahmad-S792 Ahmad-S792

Labels

CSS Cascading Style Sheets implementation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Ahmad-S792 @webkit-early-warning-system @nt1m @webkit-commit-queue @webkit-ews-buildbot