Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(406)

Issue 83413002: Derive the step base for an input element as (now) specified. (Closed)

Created:
7 years, 1 month ago by sof
Modified:
7 years ago
Reviewers:
tkent, keishi, yosin_UTC9
CC:
blink-reviews, dglazkov+blink, adamk+blink_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Derive the step base for an input element as (now) specified. Determine the step base for an input element per spec, http://www.whatwg.org/specs/web-apps/current-work/#concept-input-min-zero That is, consult the 'value' attribute if 'min' is not present, and then fallback to the default step base for the input type if that isn't present either. Previously, 'value' was not considered. R= BUG=257369 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162733

Patch Set 1 #

Total comments: 14

Patch Set 2 : Perform step base derivation via InputType::findStepBase() instead #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+99 lines, -46 lines) Patch
M LayoutTests/fast/forms/calendar-picker/calendar-picker-appearance-step.html View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step.html View 1 3 chunks +24 lines, -9 lines 0 comments Download
M LayoutTests/fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step-expected.txt View 2 chunks +4 lines, -2 lines 0 comments Download
M LayoutTests/fast/forms/calendar-picker/calendar-picker-with-step.html View 1 2 chunks +21 lines, -6 lines 0 comments Download
M LayoutTests/fast/forms/calendar-picker/calendar-picker-with-step-expected.txt View 1 chunk +4 lines, -2 lines 0 comments Download
M LayoutTests/fast/forms/calendar-picker/week-picker-appearance-step.html View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/forms/date/date-stepup-stepdown.html View 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fast/forms/date/date-stepup-stepdown-expected.txt View 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fast/forms/date/input-date-validation-message.html View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/forms/date/input-date-validation-message-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/forms/suggestion-picker/date-suggestion-picker-step-attribute.html View 4 chunks +9 lines, -5 lines 0 comments Download
M LayoutTests/fast/forms/suggestion-picker/date-suggestion-picker-step-attribute-expected.txt View 1 chunk +7 lines, -5 lines 0 comments Download
M LayoutTests/fast/forms/suggestion-picker/month-suggestion-picker-step-attribute.html View 1 2 chunks +3 lines, -3 lines 0 comments Download
M LayoutTests/fast/forms/suggestion-picker/month-suggestion-picker-step-attribute-expected.txt View 1 1 chunk +3 lines, -3 lines 0 comments Download
M LayoutTests/fast/forms/suggestion-picker/week-suggestion-picker-step-attribute.html View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/forms/DateInputType.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/forms/DateTimeLocalInputType.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/forms/InputType.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/html/forms/InputType.cpp View 1 1 chunk +8 lines, -0 lines 3 comments Download
M Source/core/html/forms/MonthInputType.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/forms/NumberInputType.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/forms/TimeInputType.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/forms/WeekInputType.cpp View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 17 (0 generated)
sof
At your own leisure, please have a look.
7 years, 1 month ago (2013-11-22 12:50:20 UTC) #1
sof
https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/InputType.h File Source/core/html/forms/InputType.h (right): https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/InputType.h#newcode205 Source/core/html/forms/InputType.h:205: virtual Decimal parseToNumber(const String&, const String&, const Decimal& defaultValue) ...
7 years, 1 month ago (2013-11-22 12:50:49 UTC) #2
tkent
Keishi, yosin, would you review this please?
7 years ago (2013-11-24 22:30:06 UTC) #3
tkent
https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/InputType.h File Source/core/html/forms/InputType.h (right): https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/InputType.h#newcode205 Source/core/html/forms/InputType.h:205: virtual Decimal parseToNumber(const String&, const String&, const Decimal& defaultValue) ...
7 years ago (2013-11-24 22:34:23 UTC) #4
yosin_UTC9
Except for RangeInputType, implementations of InputType::createStepRange() are identical. Can we share implementation of createStepRange()? https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step.html ...
7 years ago (2013-11-25 01:26:40 UTC) #5
tkent
https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/RangeInputType.cpp File Source/core/html/forms/RangeInputType.cpp (right): https://codereview.chromium.org/83413002/diff/1/Source/core/html/forms/RangeInputType.cpp#newcode132 Source/core/html/forms/RangeInputType.cpp:132: const Decimal step = equalIgnoringCase(precisionValue, "float") ? Decimal::nan() : ...
7 years ago (2013-11-25 01:36:32 UTC) #6
keishi
Tests look good. I too would prefer the fallback to be handled in createStepRange.
7 years ago (2013-11-25 02:56:49 UTC) #7
sof
Thanks very much for the great feedback. I'm travelling today, but will address as soon ...
7 years ago (2013-11-25 08:12:26 UTC) #8
sof
https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step.html File LayoutTests/fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step.html (right): https://codereview.chromium.org/83413002/diff/1/LayoutTests/fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step.html#newcode16 LayoutTests/fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step.html:16: document.getElementById('datetime-bare').value = "2012-11-20T00:00"; On 2013/11/25 01:26:41, Yoshi wrote: > ...
7 years ago (2013-11-26 17:59:05 UTC) #9
sof
On 2013/11/25 01:26:40, Yoshi wrote: > Except for RangeInputType, implementations of InputType::createStepRange() are > identical. ...
7 years ago (2013-11-26 18:01:44 UTC) #10
yosin_UTC9
On 2013/11/26 18:01:44, sof wrote: > On 2013/11/25 01:26:40, Yoshi wrote: > > Except for ...
7 years ago (2013-11-27 01:27:56 UTC) #11
yosin_UTC9
https://codereview.chromium.org/83413002/diff/130001/Source/core/html/forms/InputType.cpp File Source/core/html/forms/InputType.cpp (right): https://codereview.chromium.org/83413002/diff/130001/Source/core/html/forms/InputType.cpp#newcode988 Source/core/html/forms/InputType.cpp:988: Decimal InputType::findStepBase(const Decimal& defaultValue) const The word "find" leads ...
7 years ago (2013-11-27 01:28:05 UTC) #12
tkent
On 2013/11/27 01:27:56, Yoshi wrote: > On 2013/11/26 18:01:44, sof wrote: > > On 2013/11/25 ...
7 years ago (2013-11-27 02:04:30 UTC) #13
tkent
lgtm. yosin, keishi, thank you for non-owner review!
7 years ago (2013-11-27 02:05:32 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/83413002/130001
7 years ago (2013-11-27 02:05:46 UTC) #15
commit-bot: I haz the power
Change committed as 162733
7 years ago (2013-11-27 03:12:59 UTC) #16
sof
7 years ago (2013-11-27 09:21:02 UTC) #17
Message was sent while issue was closed.
On 2013/11/27 02:04:30, tkent wrote:
> On 2013/11/27 01:27:56, Yoshi wrote:
> > On 2013/11/26 18:01:44, sof wrote:
> > > On 2013/11/25 01:26:40, Yoshi wrote:
> > > > Except for RangeInputType, implementations of
InputType::createStepRange()
> > are
> > > > identical. Can we share implementation of createStepRange()?
> > > > 
> > > 
> > > Looks plausible, but I've not addressed this.
> > 
> > Rather than introducing simple helper, I think it is time to re-factor this.
> > Each implementation of createStepRange() looks very similar.
> 
> I don't recommend doing a behavior change and refactoring at once.  Smaller
> patch is better.

Thanks everyone, appreciated :) I can follow up on the refactoring.

Powered by Google App Engine
This is Rietveld 408576698