Make setting defaultValue from a trusted input event validate [chromium/src : main]
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/45776.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted Auto-Submit+1![]( "Open in Gerrit")
Code-Coverage
Code-Review
| 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")
It seems a bit weird to me that the behavior for setDefaultValue should be different from the behavior for changing the child content. That doesn't appear to be the case in either Gecko or WebKit:
https://searchfox.org/mozilla-central/source/dom/html/HTMLTextAreaElement.cpp#322
https://searchfox.org/wubkat/source/Source/WebCore/html/HTMLTextAreaElement.cpp#408
It also seems like the WebKit source matches our current code rather than your modifications, so I'm curious what the source of the behavior difference is. It also seems like the Gecko code (which should go through `ContentChanged` and then `Reset`) matches our current code.
Related details
- Mason Freed
Code-Coverage
Code-Review
| 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")
It seems a bit weird to me that the behavior for setDefaultValue should be different from the behavior for changing the child content. That doesn't appear to be the case in either Gecko or WebKit:
https://searchfox.org/mozilla-central/source/dom/html/HTMLTextAreaElement.cpp#322
https://searchfox.org/wubkat/source/Source/WebCore/html/HTMLTextAreaElement.cpp#408It also seems like the WebKit source matches our current code rather than your modifications, so I'm curious what the source of the behavior difference is. It also seems like the Gecko code (which should go through `ContentChanged` and then `Reset`) matches our current code.
Oops, I misread the Gecko code. It resets `mUserInteracted` but it *doesn't* reset `mLastValueChangeWasInteractive` which is the one that's relevant here. (But that holds for content changes too.)
But the webkit code looks like it resets both `m_lastChangeWasNotUserEdit` and `m_wasModifiedByUser`.
Related details
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
David BaronIt seems a bit weird to me that the behavior for setDefaultValue should be different from the behavior for changing the child content. That doesn't appear to be the case in either Gecko or WebKit:
https://searchfox.org/mozilla-central/source/dom/html/HTMLTextAreaElement.cpp#322
https://searchfox.org/wubkat/source/Source/WebCore/html/HTMLTextAreaElement.cpp#408It also seems like the WebKit source matches our current code rather than your modifications, so I'm curious what the source of the behavior difference is. It also seems like the Gecko code (which should go through `ContentChanged` and then `Reset`) matches our current code.
Oops, I misread the Gecko code. It resets `mUserInteracted` but it *doesn't* reset `mLastValueChangeWasInteractive` which is the one that's relevant here. (But that holds for content changes too.)
But the webkit code looks like it resets both `m_lastChangeWasNotUserEdit` and `m_wasModifiedByUser`.
Hmm, I admit I'm not great at navigating Gecko code, but I can't seem to find how setting defaultValue avoids setting `mLastValueChangeWasInteractive`. I'm happy to follow another pattern, if I can find it. This way (just adding a local var to keep track) seemed the most straightforward. But suggestions appreciated!
Related details
- David Baron
Code-Coverage
Code-Review
No-Unresolved-Comments
| 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")
David BaronIt seems a bit weird to me that the behavior for setDefaultValue should be different from the behavior for changing the child content. That doesn't appear to be the case in either Gecko or WebKit:
https://searchfox.org/mozilla-central/source/dom/html/HTMLTextAreaElement.cpp#322
https://searchfox.org/wubkat/source/Source/WebCore/html/HTMLTextAreaElement.cpp#408It also seems like the WebKit source matches our current code rather than your modifications, so I'm curious what the source of the behavior difference is. It also seems like the Gecko code (which should go through `ContentChanged` and then `Reset`) matches our current code.
Mason FreedOops, I misread the Gecko code. It resets `mUserInteracted` but it *doesn't* reset `mLastValueChangeWasInteractive` which is the one that's relevant here. (But that holds for content changes too.)
But the webkit code looks like it resets both `m_lastChangeWasNotUserEdit` and `m_wasModifiedByUser`.
Hmm, I admit I'm not great at navigating Gecko code, but I can't seem to find how setting defaultValue avoids setting `mLastValueChangeWasInteractive`. I'm happy to follow another pattern, if I can find it. This way (just adding a local var to keep track) seemed the most straightforward. But suggestions appreciated!
At a minimum, I'd suggest making the change unconditionally in `ChildrenChanged()` rather than conditioning it on the boolean; I don't see the reason to make the `defaultValue` setter different from changing the child content. I think it's clear that it doesn't match WebKit but I think it's plausible (although I haven't really verified -- this is complicated) that it makes the behavior closer to Gecko's. (It's possible that the condition call to `SetLastChangeWasNotUserEdit` in `HTMLTextAreaElement::SetValueCommon` could be equivalent to what Gecko does -- but I didn't really check -- see below.)
Some previous Chrome changes here were https://chromium.googlesource.com/chromium/src/+/21d9f6214b83322e0a741b77977556c735d0b189 and https://chromium.googlesource.com/chromium/src/+/e91b6459cf18e90f2766a68a2487701610213be4 and https://chromium.googlesource.com/chromium/src/+/90b048661a3949cd38ef690052d6930c230b4100 , but I didn't dig in to the details enough to really understand what's going on and whether something there made the behavior diverge from WebKit.
The Gecko logic is that `HTMLTextAreaElement::ContentChanged` (similar to WebKit and Chromium `ChildrenChanged`) calls `HTMLTextAreaElement::Reset` which calls `HTMLTextAreaElement::SetValueInternal(..., ValueSetterOption::ByInternalAPI)` which calls `TextControlState::SetValue` which uses that option to set `changeKind` to `ValueChangeKind::Internal` which calls `HTMLTextAreaElement::OnValueChanged`, which, as a result of the `aChangeKind`, does not touch `mLastValueChangeWasInteractive`.
Related details
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| 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")
Also, whatever you do, you probably should have a feature flag for this.
Related details
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Auto-Submit | +1 |
Also, whatever you do, you probably should have a feature flag for this.
Done
David BaronIt seems a bit weird to me that the behavior for setDefaultValue should be different from the behavior for changing the child content. That doesn't appear to be the case in either Gecko or WebKit:
https://searchfox.org/mozilla-central/source/dom/html/HTMLTextAreaElement.cpp#322
https://searchfox.org/wubkat/source/Source/WebCore/html/HTMLTextAreaElement.cpp#408It also seems like the WebKit source matches our current code rather than your modifications, so I'm curious what the source of the behavior difference is. It also seems like the Gecko code (which should go through `ContentChanged` and then `Reset`) matches our current code.
Mason FreedOops, I misread the Gecko code. It resets `mUserInteracted` but it *doesn't* reset `mLastValueChangeWasInteractive` which is the one that's relevant here. (But that holds for content changes too.)
But the webkit code looks like it resets both `m_lastChangeWasNotUserEdit` and `m_wasModifiedByUser`.
David BaronHmm, I admit I'm not great at navigating Gecko code, but I can't seem to find how setting defaultValue avoids setting `mLastValueChangeWasInteractive`. I'm happy to follow another pattern, if I can find it. This way (just adding a local var to keep track) seemed the most straightforward. But suggestions appreciated!
At a minimum, I'd suggest making the change unconditionally in `ChildrenChanged()` rather than conditioning it on the boolean; I don't see the reason to make the `defaultValue` setter different from changing the child content. I think it's clear that it doesn't match WebKit but I think it's plausible (although I haven't really verified -- this is complicated) that it makes the behavior closer to Gecko's. (It's possible that the condition call to `SetLastChangeWasNotUserEdit` in `HTMLTextAreaElement::SetValueCommon` could be equivalent to what Gecko does -- but I didn't really check -- see below.)
Some previous Chrome changes here were https://chromium.googlesource.com/chromium/src/+/21d9f6214b83322e0a741b77977556c735d0b189 and https://chromium.googlesource.com/chromium/src/+/e91b6459cf18e90f2766a68a2487701610213be4 and https://chromium.googlesource.com/chromium/src/+/90b048661a3949cd38ef690052d6930c230b4100 , but I didn't dig in to the details enough to really understand what's going on and whether something there made the behavior diverge from WebKit.
The Gecko logic is that `HTMLTextAreaElement::ContentChanged` (similar to WebKit and Chromium `ChildrenChanged`) calls `HTMLTextAreaElement::Reset` which calls `HTMLTextAreaElement::SetValueInternal(..., ValueSetterOption::ByInternalAPI)` which calls `TextControlState::SetValue` which uses that option to set `changeKind` to `ValueChangeKind::Internal` which calls `HTMLTextAreaElement::OnValueChanged`, which, as a result of the `aChangeKind`, does not touch `mLastValueChangeWasInteractive`.
Thanks for the suggestion. It turns out just unconditionally (other than the new feature flag) *not* setting `SetLastChangeWasNotUserEdit` in `ChildrenChanged()` does the trick and is quite a bit simpler.
Related details
- David Baron
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
LGTM. I'm still a little bit worried that the underlying mechanism here may differ from other browsers, but at this point I've gotten myself confused enough reading the code across three engines, and this does align us with other engines on the tests you've added (based on https://wpt.fyi/results/html/semantics/forms/constraints/form-validation-validity-textarea-defaultValue.html?label=pr_head&max-count=1&pr=45776 ).
Related details
- Mason Freed
Code-Coverage
Code-Review
| 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
Make setting defaultValue from a trusted input event validate
This adds a large-ish test of the validation behavior of the
textarea element, which as far as I can tell is under-specified,
or at least I could not find this behavior in the spec. I looked
mainly here:
https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#validity-states
In particular, there are some interesting differences in the
validity of initially-empty vs initially-non-empty textareas.
This CL, though, also aligns the behavior of Chromium with both
WebKit and Gecko in the specific case where the `input` event
handler sets textarea.defaultValue. In that case, the textarea
will be validated correctly. Previous to this CL, Chromium would
not validate the textarea as a result of the change.
- M third_party/blink/renderer/core/html/forms/html_text_area_element.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- A third_party/blink/web_tests/external/wpt/html/semantics/forms/constraints/form-validation-validity-textarea-defaultValue.html
Code-Review: +1 by David Baron
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/45776
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |