Skip to content
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

Make <input type=checkbox switch> iOS-zooming compatible with other ports #22968

Merged
merged 1 commit into from
Jan 22, 2024
Merged

Make <input type=checkbox switch> iOS-zooming compatible with other ports #22968

merged 1 commit into from
Jan 22, 2024

Conversation

annevk
Copy link
Contributor

@annevk annevk commented Jan 19, 2024 •
edited by webkit-early-warning-system

7c24c6f

Make <input type=checkbox switch> iOS-zooming compatible with other ports
https://bugs.webkit.org/show_bug.cgi?id=267757
rdar://121248632

Reviewed by Aditya Keerthi.

This ensures that on iOS builds that can paint a switch, the following
tests pass as expected:

- fast/forms/switch/zoom-approximates-transform-rtl.html
- fast/forms/switch/zoom-approximates-transform-vertical-lr.html
- fast/forms/switch/zoom-approximates-transform-vertical-rl.html
- fast/forms/switch/zoom-approximates-transform.html

Without this fix there would be some amount of zooming on iOS due to it
currently aligning with the font-size, but only for very large zoom
values.

Also add a new test that demonstrates this does not impact
getComputedStyle() across platforms.

* LayoutTests/fast/forms/switch/zoom-computed-style-expected.txt: Added.
* LayoutTests/fast/forms/switch/zoom-computed-style.html: Added.
* Source/WebCore/rendering/RenderThemeIOS.mm:
(WebCore::RenderThemeIOS::adjustSwitchStyle const):

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

f5ea72d

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@annevk annevk self-assigned this Jan 19, 2024
@annevk annevk added the Forms For bugs specific to form elements (checkboxes, buttons, text fields, etc.) label Jan 19, 2024
@webkit-early-warning-system

This comment was marked as outdated.

Sign in to view
pxlcoder
pxlcoder reviewed Jan 19, 2024
View reviewed changes
Source/WebCore/rendering/RenderThemeIOS.mm
Comment on lines +728 to +729
style.setLogicalWidth({ logicalSwitchWidth * style.effectiveZoom(), LengthType::Fixed });
style.setLogicalHeight({ logicalSwitchHeight * style.effectiveZoom(), LengthType::Fixed });
Copy link
Member

Choose a reason for hiding this comment

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

This seems a little strange. Can you confirm using Web Inspector that the computed width/height are unaffected when using the zoom property?

I'm also curious why switch is special over regular checkboxes. Do they already have a similar issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed strange to me as well! I have now added a test to demonstrate that.

Checkboxes are almost equal to this, except they use min-width/min-height to set this size. It's not clear to me that is better and in fact it seems more complicated for web developers to deal with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might be able to upstream this test at some point given w3c/csswg-drafts#9398.

@annevk annevk force-pushed the eng/Make-input-typecheckbox-switch-iOS-zooming-compatible-with-other-ports branch from ae1cd93 to f5ea72d Compare January 20, 2024 09:17
@webkit-early-warning-system
Copy link
Collaborator

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

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@annevk annevk requested a review from pxlcoder January 20, 2024 09:20
@annevk annevk added the merge-queue Applied to send a pull request to merge-queue label Jan 22, 2024
@annevk
Make <input type=checkbox switch> iOS-zooming compatible with other p…
7c24c6f 
…orts

https://bugs.webkit.org/show_bug.cgi?id=267757
rdar://121248632

Reviewed by Aditya Keerthi.

This ensures that on iOS builds that can paint a switch, the following
tests pass as expected:

- fast/forms/switch/zoom-approximates-transform-rtl.html
- fast/forms/switch/zoom-approximates-transform-vertical-lr.html
- fast/forms/switch/zoom-approximates-transform-vertical-rl.html
- fast/forms/switch/zoom-approximates-transform.html

Without this fix there would be some amount of zooming on iOS due to it
currently aligning with the font-size, but only for very large zoom
values.

Also add a new test that demonstrates this does not impact
getComputedStyle() across platforms.

* LayoutTests/fast/forms/switch/zoom-computed-style-expected.txt: Added.
* LayoutTests/fast/forms/switch/zoom-computed-style.html: Added.
* Source/WebCore/rendering/RenderThemeIOS.mm:
(WebCore::RenderThemeIOS::adjustSwitchStyle const):

Canonical link: https://commits.webkit.org/273289@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Make-input-typecheckbox-switch-iOS-zooming-compatible-with-other-ports branch from f5ea72d to 7c24c6f Compare January 22, 2024 08:51
@webkit-commit-queue
Copy link
Collaborator

Committed 273289@main (7c24c6f): https://commits.webkit.org/273289@main

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

@webkit-commit-queue webkit-commit-queue merged commit 7c24c6f into WebKit:main Jan 22, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 22, 2024
@annevk annevk deleted the eng/Make-input-typecheckbox-switch-iOS-zooming-compatible-with-other-ports branch January 22, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pxlcoder pxlcoder pxlcoder approved these changes

Assignees

@annevk annevk

Labels
Forms For bugs specific to form elements (checkboxes, buttons, text fields, etc.)
Projects
None yet
Milestone
No milestone
4 participants
@annevk @webkit-early-warning-system @webkit-commit-queue @pxlcoder