| Summary: | INPUT element: Fix integer overflow in input.stepDown() | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Ahmad Saleem <ahmad.saleem792> |
| Component: | Forms | Assignee: | Nobody <webkit-unassigned> |
| Status: | NEW --- | ||
| Severity: | Normal | CC: | akeerthi, annevk, cdumez, karlcow, webkit-bug-importer, wenson_hsieh |
| Priority: | P2 | Keywords: | BrowserCompat, InRadar |
| Version: | Safari 16 | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
|
Description
Ahmad Saleem
2023-09-03 04:57:11 PDT
The specification just says to negate delta, not multiply it by -1. I also don't understand the switch from integers to floats in the Chromium patch. Perhaps this is something that needs to change, but then I'd prefer we have some web-platform-tests. Ahmad, could you create a test on WPT. So I am ranting and trying to understand it better, so ignore me, if I am totally wrong. First - read HTMLInputElement interface and see 'stepUp' and 'stepDown': --> https://html.spec.whatwg.org/multipage/input.html#htmlinputelement undefined stepUp(optional long n = 1); undefined stepDown(optional long n = 1); Which if I read WebIDL specification: --> https://webidl.spec.whatwg.org/#idl-long Where it should be (long type integer): >> The long type is a signed integer type that has values in the range [−2147483648, 2147483647]. and then in our WebKit implementation: https://searchfox.org/wubkat/rev/581e116dc6ce254811dbe2da9d1c1168762fc30c/Source/WebCore/html/HTMLInputElement.cpp#390 We have: ExceptionOr<void> HTMLInputElement::stepDown(int n) { return m_inputType->stepUp(-n); } Which as per Blink should be changed to: ExceptionOr<void> HTMLInputElement::stepDown(int n) { return m_inputType->stepUp(-1.0); } I am not sure then why we need 'int n' argument. As Anne mentioned, even I am not clear on why they change `int` to `double`, while the `step` should be integers as per web-spec. Ahmad,
I guess there is a typo in the comment not "-1.0" but "-1.0 * n"
```
void HTMLInputElement::stepDown(int n, ExceptionState& exceptionState) {
- m_inputType->stepUp(-n, exceptionState);
+ m_inputType->stepUp(-1.0 * n, exceptionState);
}
```
The algorithm process is in
https://html.spec.whatwg.org/multipage/input.html#dom-input-stepdown
stepUp is calling
https://searchfox.org/wubkat/source/Source/WebCore/html/InputType.cpp#988
Gecko is calling applyStep for both stepUp and stepDown
https://searchfox.org/mozilla-central/rev/15f758f06d97ee3c4e382b174836395442c38f71/dom/html/HTMLInputElement.h#666-668
Note that the blink Code is
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_input_element.cc;l=293;drc=3cc22c22d7637bc5604e8fef3b0882b51a762901;bpv=1;bpt=1
void HTMLInputElement::stepUp(int n, ExceptionState& exception_state) {
input_type_->StepUp(n, exception_state);
}
void HTMLInputElement::stepDown(int n, ExceptionState& exception_state) {
input_type_->StepUp(-1.0 * n, exception_state);
}
but yes it looks like a typo mistake for chromium code, but who knows? The review doesn't make any specific comment on
https://chromium.googlesource.com/chromium/src.git/+/28c02d6167e1c545716f433dc84e3656da28c816
Currently, there is an extra step to step-down. HTMLInputElement::stepUp(int n) -> InputType::stepUp(int n) -> InputType::applyStep HTMLInputElement::stepDown(int n) -> HTMLInputElement::stepUp(int n) -> InputType::stepUp(int n) -> InputType::applyStep if I wonder if it's to avoid to create InputType::stepDown(int n) |