Fix readonly elements incorrectly barred from constraint validation [chromium/src : main]
YeongHan Kim (Gerrit)
YeongHan Kim added 1 comment![]( "Open in Gerrit")
Related details
- Joey Arhar
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 3 comments![]( "Open in Gerrit")
constraint validation. The current implementation does not bar
constraint validation by default, but bars it when the element is
readonly. However, for button [2] and select [3], the barred conditionWhere is the code that is already doing this? Should it be removed since this patch adds new code seemingly for add ListedElements?
bool ReadOnlyBarsFromConstraintValidation() const final { return true; }Is this tested? It seems like the test changes are only for the select and button elements, but not custom elements.
virtual bool ReadOnlyBarsFromConstraintValidation() const { return false; }I think "Prevents" would be easier to read than "BarsFrom"
Related details
- Mason Freed
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 3 comments![]( "Open in Gerrit")
virtual bool ReadOnlyBarsFromConstraintValidation() const { return false; }I think "Prevents" would be easier to read than "BarsFrom"
+1. Also, could you please add a link above this to the spec:
!(is_readonly_ && ReadOnlyBarsFromConstraintValidation());Two things:
1. I think this would be a bit easier to read:
`(!is_readonly_ || !ReadOnlyPreventsConstraintValidation())`
and 2. I think this needs a runtime enabled feature flag guard. It's a small change, but you never know what it might break.
bool ReadOnlyBarsFromConstraintValidation() const final { return true; }Add link to spec please
Related details
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
YeongHan Kim added 6 comments![]( "Open in Gerrit")
constraint validation. The current implementation does not bar
constraint validation by default, but bars it when the element is
readonly. However, for button [2] and select [3], the barred conditionWhere is the code that is already doing this? Should it be removed since this patch adds new code seemingly for add ListedElements?
This is [the part](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/listed_element.cc;l=373?q=third_party%2Fblink%2Frenderer%2Fcore%2Fhtml%2Fforms%2Flisted_element.cc&ss=chromium). I think I wrote the commit message in a confusing way. I was trying to say that the current implementation bars all elements when they are readonly.
So this code should not be removed. Instead, it should find elements that are readonly and whose constraint validation is prevented by readonly.
If I understand your point correctly, I'll update the commit message.
bool ReadOnlyBarsFromConstraintValidation() const final { return true; }Is this tested? It seems like the test changes are only for the select and button elements, but not custom elements.
I think it's being verified in [this test](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/custom-elements/form-associated/ElementInternals-validation.html;l=54-58?q=ElementInternals-validation&ss=chromium). But would it be better to add a new test under form to make it clearer?
virtual bool ReadOnlyBarsFromConstraintValidation() const { return false; }Mason FreedI think "Prevents" would be easier to read than "BarsFrom"
+1. Also, could you please add a link above this to the spec:
Done!
!(is_readonly_ && ReadOnlyBarsFromConstraintValidation());Two things:
1. I think this would be a bit easier to read:
`(!is_readonly_ || !ReadOnlyPreventsConstraintValidation())`
and 2. I think this needs a runtime enabled feature flag guard. It's a small change, but you never know what it might break.
Agreed. I've added a feature flag!
bool ReadOnlyBarsFromConstraintValidation() const final { return true; }Add link to spec please
- Joey Arhar
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 2 comments![]( "Open in Gerrit")
constraint validation. The current implementation does not bar
constraint validation by default, but bars it when the element is
readonly. However, for button [2] and select [3], the barred conditionYeongHan KimWhere is the code that is already doing this? Should it be removed since this patch adds new code seemingly for add ListedElements?
This is [the part](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/listed_element.cc;l=373?q=third_party%2Fblink%2Frenderer%2Fcore%2Fhtml%2Fforms%2Flisted_element.cc&ss=chromium). I think I wrote the commit message in a confusing way. I was trying to say that the current implementation bars all elements when they are readonly.
So this code should not be removed. Instead, it should find elements that are readonly and whose constraint validation is prevented by readonly.If I understand your point correctly, I'll update the commit message.
Acknowledged
bool ReadOnlyBarsFromConstraintValidation() const final { return true; }YeongHan KimIs this tested? It seems like the test changes are only for the select and button elements, but not custom elements.
I think it's being verified in [this test](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/custom-elements/form-associated/ElementInternals-validation.html;l=54-58?q=ElementInternals-validation&ss=chromium). But would it be better to add a new test under form to make it clearer?
So if you removed this added method, then that test would fail?
Related details
- Mason Freed
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
3 comments
LGTM, but please wait for Joey's LGTM also.
// Whether the readonly attribute prevents from constraint
// validation depends on the submittable element.nit:
```
When enabled, the readonly attribute will prevent constraint validation
only in some cases, depending on the element.
```
// This was added in M146 and can be removed after M148.It looks like this will land in M147, and can be removed after M149.
Related details
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
YeongHan Kim added 4 comments![]( "Open in Gerrit")
bool ReadOnlyBarsFromConstraintValidation() const final { return true; }YeongHan KimIs this tested? It seems like the test changes are only for the select and button elements, but not custom elements.
Joey ArharI think it's being verified in [this test](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/custom-elements/form-associated/ElementInternals-validation.html;l=54-58?q=ElementInternals-validation&ss=chromium). But would it be better to add a new test under form to make it clearer?
So if you removed this added method, then that test would fail?
No, it won't fail.
Here's a summary of the changes:
- AS-IS: Prevents constraint validation for all elements when readonly
- TO-BE: Only prevents constraint validation for elements whose condition for preventing validation is readonly
For [custom elements](https://html.spec.whatwg.org/multipage/custom-elements.html#custom-elements-core-concepts), since readonly is a condition that should prevent validation, the result doesn't change.
The elements affected by this change are button and select.
// Whether the readonly attribute prevents from constraint
// validation depends on the submittable element.nit:
```
When enabled, the readonly attribute will prevent constraint validation
only in some cases, depending on the element.
```
Done!
// This was added in M146 and can be removed after M148.It looks like this will land in M147, and can be removed after M149.
- Joey Arhar
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
1 comment
bool ReadOnlyBarsFromConstraintValidation() const final { return true; }YeongHan KimIs this tested? It seems like the test changes are only for the select and button elements, but not custom elements.
Joey ArharI think it's being verified in [this test](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/custom-elements/form-associated/ElementInternals-validation.html;l=54-58?q=ElementInternals-validation&ss=chromium). But would it be better to add a new test under form to make it clearer?
YeongHan KimSo if you removed this added method, then that test would fail?
No, it won't fail.
Here's a summary of the changes:
- AS-IS: Prevents constraint validation for all elements when readonly
- TO-BE: Only prevents constraint validation for elements whose condition for preventing validation is readonly
For [custom elements](https://html.spec.whatwg.org/multipage/custom-elements.html#custom-elements-core-concepts), since readonly is a condition that should prevent validation, the result doesn't change.
The elements affected by this change are button and select.
- Mason Freed
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted Code-Review+1![]( "Open in Gerrit")
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
YeongHan Kim voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fix readonly elements incorrectly barred from constraint validation
According to [1], a submittable element is a candidate for constraint
validation except when a condition has barred the element from
constraint validation. The current implementation does not bar
constraint validation by default, but bars it when the element is
readonly. However, for button [2] and select [3], the barred condition
is not readonly, so they are incorrectly barred from constraint
validation.
So, this patch changes the behavior so that elements are not barred from
constraint validation even if they are readonly, and only elements whose
barred condition includes readonly are barred. The elements whose barred
condition includes readonly are input, textarea, and custom elements.
[1] https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#barred-from-constraint-validation
[2] https://html.spec.whatwg.org/multipage/form-elements.html#the-button-element:barred-from-constraint-validation
[3] https://html.spec.whatwg.org/multipage/form-elements.html#the-select-element:barred-from-constraint-validation
- M third_party/blink/renderer/core/html/custom/element_internals.h
- M third_party/blink/renderer/core/html/forms/listed_element.cc
- M third_party/blink/renderer/core/html/forms/listed_element.h
- M third_party/blink/renderer/core/html/forms/text_control_element.h
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- D third_party/blink/web_tests/external/wpt/html/semantics/forms/the-button-element/button-willvalidate-readonly-attribute-expected.txt
- D third_party/blink/web_tests/external/wpt/html/semantics/forms/the-select-element/select-willvalidate-readonly-attribute-expected.txt
Code-Review: +1 by Joey Arhar, +1 by Mason Freed
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |