Fix HTMLFormControlElement::IsReadOnly for non-supporting elements. [chromium/src : main]
Nan Lin (Gerrit)
Nan Lin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nan Lin
| Commit-Queue | +1 |
1 comment
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
I think this is mostly correct, except that I think form-associated custom elements *probably* go through this codepath as well, and they need to honor `readonly` too.
I think it is a web-exposed behavior change, and it almost certainly needs both (a) [a `RuntimeEnabledFeatures` flag that can be used as a killswitch](https://groups.google.com/a/chromium.org/g/blink-dev/c/jhJLN9drXy4/m/RXCJx0-VCAAJ) if needed and (b) tests in WPT that fail without the change and pass with it (these don't need to be exhaustive but it's probably also good if they cover a few different cases) and (c) tests that `readonly` still works for form-associated custom elements which I *think* this change breaks (although I'm not 100% sure). (I don't think the unittests are particularly needed... although I basically always think that about unittests.)
I think it also intersects with the fix in https://issues.chromium.org/486228104 which is rolling out to stable within the next few days, but which appears to me to be incomplete since it doesn't have any per-input-type specific behavior but instead honors `readonly` for all `<input>` types (since they all inherit from `TextControlElement`).
I'm also curious whether this brings us closer in line with other browser engines or further away from them.
Related details
- Nan Lin
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. |
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
I think this is mostly correct, except that I think form-associated custom elements *probably* go through this codepath as well, and they need to honor `readonly` too.
I think it is a web-exposed behavior change, and it almost certainly needs both (a) [a `RuntimeEnabledFeatures` flag that can be used as a killswitch](https://groups.google.com/a/chromium.org/g/blink-dev/c/jhJLN9drXy4/m/RXCJx0-VCAAJ) if needed and (b) tests in WPT that fail without the change and pass with it (these don't need to be exhaustive but it's probably also good if they cover a few different cases) and (c) tests that `readonly` still works for form-associated custom elements which I *think* this change breaks (although I'm not 100% sure). (I don't think the unittests are particularly needed... although I basically always think that about unittests.)
I think it also intersects with the fix in https://issues.chromium.org/486228104 which is rolling out to stable within the next few days, but which appears to me to be incomplete since it doesn't have any per-input-type specific behavior but instead honors `readonly` for all `<input>` types (since they all inherit from `TextControlElement`).
I'm also curious whether this brings us closer in line with other browser engines or further away from them.
Thanks David.
I've looked into the concerns regarding web-exposed behavior. It appears that the change in this CL doesn't actually change web-exposed behaviors, e.g. CSS pseudo-classes and validation.
CSS pseudo-classes: Elements like <button> already match :read-only because they are not editable. Blink's selector matching for these elements doesn't currently call `IsReadOnly()`, with <textarea> and <input> correctly checks the attibute as well as the supported input types. See the [WPT test](third_party/blink/web_tests/external/wpt/html/semantics/selectors/pseudo-classes/readwrite-readonly.html).
Validation: I agree that the fix for crbug.com/486228104 is incomplete which doesn't handle unsupported input types. But this will not be fixed by this CL yet, as it doesn't call `IsReadOnly()`. I will fix this in a follow up CL.
The updated logic may change callers, e.g. `AIPageContentAgent` which checks the readonly state of the element for page content extraction.
I wrapped the updated logic in a runtime feature as a kill switch in case of unexpected behaviors. Does this make sense?
Related details
- David Baron
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. |
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
Nan LinI think this is mostly correct, except that I think form-associated custom elements *probably* go through this codepath as well, and they need to honor `readonly` too.
I think it is a web-exposed behavior change, and it almost certainly needs both (a) [a `RuntimeEnabledFeatures` flag that can be used as a killswitch](https://groups.google.com/a/chromium.org/g/blink-dev/c/jhJLN9drXy4/m/RXCJx0-VCAAJ) if needed and (b) tests in WPT that fail without the change and pass with it (these don't need to be exhaustive but it's probably also good if they cover a few different cases) and (c) tests that `readonly` still works for form-associated custom elements which I *think* this change breaks (although I'm not 100% sure). (I don't think the unittests are particularly needed... although I basically always think that about unittests.)
I think it also intersects with the fix in https://issues.chromium.org/486228104 which is rolling out to stable within the next few days, but which appears to me to be incomplete since it doesn't have any per-input-type specific behavior but instead honors `readonly` for all `<input>` types (since they all inherit from `TextControlElement`).
I'm also curious whether this brings us closer in line with other browser engines or further away from them.
Thanks David.
I've looked into the concerns regarding web-exposed behavior. It appears that the change in this CL doesn't actually change web-exposed behaviors, e.g. CSS pseudo-classes and validation.
CSS pseudo-classes: Elements like <button> already match :read-only because they are not editable. Blink's selector matching for these elements doesn't currently call `IsReadOnly()`, with <textarea> and <input> correctly checks the attibute as well as the supported input types. See the [WPT test](third_party/blink/web_tests/external/wpt/html/semantics/selectors/pseudo-classes/readwrite-readonly.html).
Validation: I agree that the fix for crbug.com/486228104 is incomplete which doesn't handle unsupported input types. But this will not be fixed by this CL yet, as it doesn't call `IsReadOnly()`. I will fix this in a follow up CL.
The updated logic may change callers, e.g. `AIPageContentAgent` which checks the readonly state of the element for page content extraction.
I wrapped the updated logic in a runtime feature as a kill switch in case of unexpected behaviors. Does this make sense?
Form-associated custom elements are implemented with `ElementInternals` which doesn't go through `HTMLFormControlElement` code paths IIUC.