Skip to content

Conversation

annevk
Copy link
Contributor

@annevk annevk commented Jan 31, 2025

c670903

Improve <input type=color alpha> performance on macOS
https://bugs.webkit.org/show_bug.cgi?id=286803
rdar://143955048

Reviewed by Aditya Keerthi.

Using data: URLs here resulted in annoying flickering when moving the
alpha slider quickly.

Using SVG directly in the shadow tree ran into
https://bugs.webkit.org/show_bug.cgi?id=287472 so therefore we use a
div element with a polygon clip-path. This results in additional render
layers, but websites will likely not have a lot of these controls so
that seems okay.

* LayoutTests/fast/forms/color/color-input-swatch-expected.txt:
* LayoutTests/fast/forms/color/color-input-swatch.html:
* LayoutTests/platform/mac/fast/forms/color/color-input-swatch-expected.txt:
* LayoutTests/platform/mac/fast/forms/color/input-appearance-color-expected.txt:
* Source/WebCore/html/ColorInputType.cpp:
(WebCore::ColorInputType::createShadowSubtree):
(WebCore::ColorInputType::updateColorSwatch):
* Source/WebCore/rendering/RenderTheme.h:
(WebCore::RenderTheme::createColorWellSwatchSubtree):
* Source/WebCore/rendering/mac/RenderThemeMac.h:
* Source/WebCore/rendering/mac/RenderThemeMac.mm:
(WebCore::RenderThemeMac::createColorWellSwatchSubtree):
(WebCore::RenderThemeMac::setColorWellSwatchBackground):

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

cebe08f

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
🛠 🧪 unsafe-merge ✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@annevk annevk requested review from cdumez and rniwa as code owners January 31, 2025 14:02
@annevk annevk self-assigned this Jan 31, 2025
@annevk annevk added the Forms For bugs specific to form elements (checkboxes, buttons, text fields, etc.) label Jan 31, 2025
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 31, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look quite right. Maybe you want a shouldBe-like function here to clearly state that empty string is the expectation? Other logged values may benefit from shouldBe, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really want to state the expectations as they can differ per platform, as they do here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even this one, empty innerHTML?

That said, color components differing per platform (and I'm sure also per configuration) is only more of a reason to have expectations. Possibly, saying that the numbers should be within a certain range. Currently, you are asking everyone who sees the test fail to investigate for themselves whether the different number means failing or passing, and that's rather hard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They only differ between macOS and everywhere else at this point. And only macOS has a deeper subtree. Anyone touching the shadow tree for this component would know why this test regressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hear your explanations, but I still really dislike tests that just dump results, as existing ones that do so are a constant nuisance.

@annevk
Copy link
Contributor Author

annevk commented Jan 31, 2025

The fast/shadow-dom/color-input-element-shadow-manipulation.html failure on macOS seems like a real problem. Hit testing (at least through document.caretRangeFromPoint()) no longer returns the input element because it now contains SVG in its shadow root. Apparently whenever you'd use SVG in your shadow root you'd be susceptible to this.

@annevk annevk removed the merging-blocked Applied to prevent a change from being merged label Feb 11, 2025
@annevk annevk force-pushed the eng/Improve-input-type-color-alpha-performance-on-macOS branch from af481d7 to cebe08f Compare February 11, 2025 09:46
@annevk annevk added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks and removed unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing labels Feb 11, 2025
@webkit-ews-buildbot webkit-ews-buildbot added 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 Feb 11, 2025
@webkit-ews-buildbot
Copy link
Collaborator

Safe-Merge-Queue: Build #48499.

https://bugs.webkit.org/show_bug.cgi?id=286803
rdar://143955048

Reviewed by Aditya Keerthi.

Using data: URLs here resulted in annoying flickering when moving the
alpha slider quickly.

Using SVG directly in the shadow tree ran into
https://bugs.webkit.org/show_bug.cgi?id=287472 so therefore we use a
div element with a polygon clip-path. This results in additional render
layers, but websites will likely not have a lot of these controls so
that seems okay.

* LayoutTests/fast/forms/color/color-input-swatch-expected.txt:
* LayoutTests/fast/forms/color/color-input-swatch.html:
* LayoutTests/platform/mac/fast/forms/color/color-input-swatch-expected.txt:
* LayoutTests/platform/mac/fast/forms/color/input-appearance-color-expected.txt:
* Source/WebCore/html/ColorInputType.cpp:
(WebCore::ColorInputType::createShadowSubtree):
(WebCore::ColorInputType::updateColorSwatch):
* Source/WebCore/rendering/RenderTheme.h:
(WebCore::RenderTheme::createColorWellSwatchSubtree):
* Source/WebCore/rendering/mac/RenderThemeMac.h:
* Source/WebCore/rendering/mac/RenderThemeMac.mm:
(WebCore::RenderThemeMac::createColorWellSwatchSubtree):
(WebCore::RenderThemeMac::setColorWellSwatchBackground):

Canonical link: https://commits.webkit.org/290228@main
@webkit-commit-queue
Copy link
Collaborator

Committed 290228@main (c670903): https://commits.webkit.org/290228@main

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

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 11, 2025
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Improve-input-type-color-alpha-performance-on-macOS branch from cebe08f to c670903 Compare February 11, 2025 19:27
@webkit-commit-queue webkit-commit-queue merged commit c670903 into WebKit:main Feb 11, 2025
@annevk annevk deleted the eng/Improve-input-type-color-alpha-performance-on-macOS branch February 12, 2025 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Forms For bugs specific to form elements (checkboxes, buttons, text fields, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants