Summary: | Spinbuttons on [type="number"] etc. with [step="any"] do not work | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | alexander farkas <info> | ||||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Enhancement | CC: | ap, info, joepeck, tkent, webkit.review.bot | ||||||||
Priority: | P3 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
alexander farkas
2011-04-03 01:29:47 PDT
> I would suggest, if step is "any" the button "spins" the value with the default step (like it would if no step is configured).
That sounds good.
We could probably then fix up RangeInputType::handleKeydownEvent which has the following comment: // FIXME: We can't use stepUp() for the step value "any". So, we increase // or decrease the value by 1/100 of the value range. Is it reasonable? Created attachment 99054 [details]
Patch
Comment on attachment 99054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99054&action=review > Source/WebCore/ChangeLog:17 > + * html/HTMLInputElement.cpp: > + (WebCore::HTMLInputElement::applyStep): > + (WebCore::HTMLInputElement::getAllowedValueStepWithDecimalPlacesWithAllowingStepAny): > + (WebCore::HTMLInputElement::stepUp): > + (WebCore::HTMLInputElement::stepDown): > + (WebCore::HTMLInputElement::stepUpFromRenderer): > + * html/HTMLInputElement.h: You should write summaries of changes in each of files/functions. > Source/WebCore/html/HTMLInputElement.cpp:380 > +void HTMLInputElement::applyStep(double count, bool allowsStepAny, ExceptionCode& ec) Do not introduce bool parameter. We prefer enum to bool strongly. e.g. enum AnyStepHandling { RejectAny, AnyIsDefaultStep }; > Source/WebCore/html/HTMLInputElement.cpp:443 > + ASSERT(step); > + double defaultStep = m_inputType->defaultStep(); > + double stepScaleFactor = m_inputType->stepScaleFactor(); > + if (!isfinite(defaultStep) || !isfinite(stepScaleFactor)) > + return false; This part is duplicate with getAllowedValueStepWithDecimalPlaces(). It's not good. The difference between this function and getAllowedValueStepWithDecimalPlaces() is only "any" handling. I think it's ok to add the enum parameter to getAllowedValueStepWithDecimalPlaces(). Created attachment 99260 [details]
Patch
Comment on attachment 99260 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99260&action=review > Source/WebCore/html/HTMLInputElement.h:67 > + bool getAllowedValueStepWithDecimalPlaces(AnyStepHandling, double*, unsigned*) const; Can you make getAllowedValeuStepWithDecimalPlaces() and AnyStepHandling private? Created attachment 99670 [details]
Patch
Comment on attachment 99670 [details]
Patch
Great patch. Thank you!
Comment on attachment 99670 [details] Patch Clearing flags on attachment: 99670 Committed r90385: <http://trac.webkit.org/changeset/90385> All reviewed patches have been landed. Closing bug. (In reply to comment #2) > We could probably then fix up RangeInputType::handleKeydownEvent > which has the following comment: > > // FIXME: We can't use stepUp() for the step value "any". So, we increase > // or decrease the value by 1/100 of the value range. Is it reasonable? I'm not sure if assuming "any" 1.0 for type=range is better. We have another issue about keyboard operation with the default step, Bug 63574. |