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.

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

Form associated ElementInternals always reports customError when using setValidity #42134

Conversation

kyubisation
Copy link
Contributor

@kyubisation kyubisation commented Mar 8, 2025

072d9ca

Form associated ElementInternals always reports customError when using setValidity
https://bugs.webkit.org/show_bug.cgi?id=261432

Reviewed by Ryosuke Niwa.

The current implementation toggles customError when any validationMessage is
set, which works for native form elements, but breaks with ElementInternals.
This PR fixes this by overriding the customError check in FormAssociatedCustomElement.
(Test was added in web-platform-tests/wpt#51039

* LayoutTests/imported/w3c/web-platform-tests/custom-elements/form-associated/ElementInternals-validation-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/custom-elements/form-associated/ElementInternals-validation.html:
* LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/custom-elements/form-associated/ElementInternals-validation-expected.txt
* Source/WebCore/html/FormAssociatedCustomElement.h:
* Source/WebCore/html/FormListedElement.h:

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

a97679c

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

@kyubisation kyubisation requested review from cdumez and rniwa as code owners March 8, 2025 15:51
@kyubisation
Copy link
Contributor Author

There is a previous attempt to fix this bug: #19434
As that PR seems to be stuck, this PR uses a different approach in the hope of achieving a fully green build.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 8, 2025
@kyubisation kyubisation force-pushed the eng/Form-associated-ElementInternals-always-reports-customError-when-using-setValidity branch from 24002d3 to 1fcfb12 Compare March 8, 2025 20:07
@kyubisation kyubisation marked this pull request as draft March 8, 2025 21:13
@kyubisation kyubisation force-pushed the eng/Form-associated-ElementInternals-always-reports-customError-when-using-setValidity branch from 1fcfb12 to 7bb50ca Compare March 8, 2025 21:21
@kyubisation kyubisation force-pushed the eng/Form-associated-ElementInternals-always-reports-customError-when-using-setValidity branch from 7bb50ca to 1ec62db Compare March 8, 2025 23:44
@kyubisation kyubisation marked this pull request as ready for review March 9, 2025 10:41
@kyubisation
Copy link
Contributor Author

The api-mac and api-wpe tests seem to be flakey and fail for unrelated reasons to this PR, as they also fail in other PRs for the same reason at the moment.
The failure of mac-wk1 seems weird to me, as the expected file seems out of date?

@rniwa rniwa removed the merging-blocked Applied to prevent a change from being merged label Mar 9, 2025
@rniwa
Copy link
Member

rniwa commented Mar 9, 2025

The failure in mac-wk1 looks real. You need to run the following command to reset the expected result for WebKit1

./Tools/Scripts/run-webkit-tests --no-build --no-show-results imported/w3c/web-platform-tests/custom-elements/form-associated/ElementInternals-validation.html -1 --reset

@kyubisation kyubisation force-pushed the eng/Form-associated-ElementInternals-always-reports-customError-when-using-setValidity branch from 1ec62db to 5684798 Compare March 9, 2025 19:06
@kyubisation
Copy link
Contributor Author

@rniwa Thank you for the advice!
Sorry, I'm not yet familiar with this code base so I appreciate any help. 😃

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 9, 2025
@kyubisation kyubisation force-pushed the eng/Form-associated-ElementInternals-always-reports-customError-when-using-setValidity branch from 5684798 to a97679c Compare March 9, 2025 22:00
@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label Mar 9, 2025
@Ahmad-S792 Ahmad-S792 added the Forms For bugs specific to form elements (checkboxes, buttons, text fields, etc.) label Mar 9, 2025
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 9, 2025
@kyubisation
Copy link
Contributor Author

@rniwa At this point it seems to be a matter of luck to get all of the test suites to pass. The problems seem unrelated to this PR. Should I keep trying by rebasing or is this state good enough to merge?

@rniwa rniwa added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Mar 10, 2025
@rniwa
Copy link
Member

rniwa commented Mar 10, 2025

Indeed, EWS failures seem unrelated to this PR so I'm adding it to the merge queue. Thanks for the bug fix :)

@kyubisation
Copy link
Contributor Author

No problem. Thank you again for the help. 🙇

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…g setValidity

https://bugs.webkit.org/show_bug.cgi?id=261432

Reviewed by Ryosuke Niwa.

The current implementation toggles customError when any validationMessage is
set, which works for native form elements, but breaks with ElementInternals.
This PR fixes this by overriding the customError check in FormAssociatedCustomElement.
(Test was added in web-platform-tests/wpt#51039)

* LayoutTests/imported/w3c/web-platform-tests/custom-elements/form-associated/ElementInternals-validation-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/custom-elements/form-associated/ElementInternals-validation.html:
* LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/custom-elements/form-associated/ElementInternals-validation-expected.txt
* Source/WebCore/html/FormAssociatedCustomElement.h:
* Source/WebCore/html/FormListedElement.h:

Canonical link: https://commits.webkit.org/291878@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Form-associated-ElementInternals-always-reports-customError-when-using-setValidity branch from a97679c to 072d9ca Compare March 10, 2025 07:04
@webkit-commit-queue
Copy link
Collaborator

Committed 291878@main (072d9ca): https://commits.webkit.org/291878@main

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

@webkit-commit-queue webkit-commit-queue merged commit 072d9ca 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
@kyubisation kyubisation deleted the eng/Form-associated-ElementInternals-always-reports-customError-when-using-setValidity branch March 10, 2025 07:05
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