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 #19434

Conversation

shvaikalesh
Copy link
Member

@shvaikalesh shvaikalesh commented Oct 23, 2023

2b9f4fa

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

Reviewed by NOBODY (OOPS!).

Initial form-associated custom elements implementation attempted to avoid adding a String field by
relying on m_customValidationMessage for both regular [1] and custom [2] validation messages,
which turned out to be incompatible with the implementation of FormListedElement::customError().

Rather than deepening the hack / micro-optimization, this change adds a field and precisely implements
all the spec steps for setValidity() [3].

[1] https://html.spec.whatwg.org/multipage/custom-elements.html#face-validation-message
[2] https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#custom-validity-error-message
[3] https://html.spec.whatwg.org/multipage/custom-elements.html#dom-elementinternals-setvalidity

* LayoutTests/imported/w3c/web-platform-tests/custom-elements/form-associated/ElementInternals-validation.html:
* Source/WebCore/html/FormAssociatedCustomElement.cpp:
(WebCore::FormAssociatedCustomElement::setValidity):
(WebCore::FormAssociatedCustomElement::validationMessage const):
* Source/WebCore/html/FormAssociatedCustomElement.h:

2b9f4fa

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

@shvaikalesh shvaikalesh self-assigned this Oct 23, 2023
@shvaikalesh shvaikalesh added the Forms For bugs specific to form elements (checkboxes, buttons, text fields, etc.) label Oct 23, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 23, 2023
@shvaikalesh shvaikalesh removed the merging-blocked Applied to prevent a change from being merged label Oct 24, 2023
@shvaikalesh shvaikalesh force-pushed the eng/Form-associated-ElementInternals-always-reports-customError-when-using-setValidity branch from dfa8426 to de90c54 Compare October 24, 2023 08:50
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 24, 2023
@shvaikalesh shvaikalesh removed the merging-blocked Applied to prevent a change from being merged label Oct 24, 2023
@shvaikalesh shvaikalesh force-pushed the eng/Form-associated-ElementInternals-always-reports-customError-when-using-setValidity branch from de90c54 to 36008ea Compare October 24, 2023 19:33
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 24, 2023
@shvaikalesh shvaikalesh removed the merging-blocked Applied to prevent a change from being merged label Oct 25, 2023
@shvaikalesh shvaikalesh force-pushed the eng/Form-associated-ElementInternals-always-reports-customError-when-using-setValidity branch from 36008ea to 32904d3 Compare October 25, 2023 03:01
@annevk
Copy link
Contributor

annevk commented Dec 29, 2023

I restarted gtk-wk2. @shvaikalesh I suggest we land this if that passes. Seems like a shame to leave this unfixed.

Edit: it seems like this might need to be rebased for the bots to run properly.

@shvaikalesh shvaikalesh force-pushed the eng/Form-associated-ElementInternals-always-reports-customError-when-using-setValidity branch from 32904d3 to 0b5cb42 Compare January 2, 2024 22:52
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 3, 2024
@lukewarlow
Copy link
Member

Any chance of getting this rebased? Is a shame to have this (interop) bug in this API.

@annevk
Copy link
Contributor

annevk commented Apr 13, 2024

The problem as I understand it is that the gtk-wk2 failure seems real and thus needs investigation by someone(tm).

…g setValidity

https://bugs.webkit.org/show_bug.cgi?id=261432
<rdar://problem/115681066>

Reviewed by NOBODY (OOPS!).

Initial form-associated custom elements implementation attempted to avoid adding a String field by
relying on m_customValidationMessage for both regular [1] and custom [2] validation messages,
which turned out to be incompatible with the implementation of FormListedElement::customError().

Rather than deepening the hack / micro-optimization, this change adds a field and precisely implements
all the spec steps for setValidity() [3].

[1] https://html.spec.whatwg.org/multipage/custom-elements.html#face-validation-message
[2] https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#custom-validity-error-message
[3] https://html.spec.whatwg.org/multipage/custom-elements.html#dom-elementinternals-setvalidity

* LayoutTests/imported/w3c/web-platform-tests/custom-elements/form-associated/ElementInternals-validation.html:
* Source/WebCore/html/FormAssociatedCustomElement.cpp:
(WebCore::FormAssociatedCustomElement::setValidity):
(WebCore::FormAssociatedCustomElement::validationMessage const):
* Source/WebCore/html/FormAssociatedCustomElement.h:
@shvaikalesh shvaikalesh removed the merging-blocked Applied to prevent a change from being merged label Oct 10, 2024
@shvaikalesh shvaikalesh force-pushed the eng/Form-associated-ElementInternals-always-reports-customError-when-using-setValidity branch from 0b5cb42 to 2b9f4fa Compare October 10, 2024 19:43
@kyubisation
Copy link
Contributor

Hello everyone
This bug has been fixed in #42134

@annevk
Copy link
Contributor

annevk commented Mar 10, 2025

That's great, thanks!

@annevk annevk closed this Mar 10, 2025
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.) merging-blocked Applied to prevent a change from being merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants