Bug 216090

Created attachment 407795 [details]
Patch

Comment on attachment 407795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407795&action=review

> Source/WebCore/html/shadow/DateTimeFieldElement.cpp:102
> +    if (key == "Up") {
> +        keyboardEvent.setDefaultHandled();
> +        stepUp();
> +        return;
> +    }
> +
> +    if (key == "Down") {
> +        keyboardEvent.setDefaultHandled();
> +        stepDown();
> +        return;
> +    }

Code above calls setDefaultHandled after handling. This code and the code below calls it before handling. I suggest we consider a consistent pattern of doing setDefaultHandled last.

> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:102
> +    m_typeAheadBuffer.clear();
> +    setValueAsInteger(newValue, DispatchInputAndChangeEvents);

Would be nice to share this between up and down. I know it’s just two lines of code.

> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:82
> +    int newValue = hasValue() ? m_selectedIndex - 1 : m_symbols.size() - 1;
> +    if (newValue < 0)
> +        newValue = m_symbols.size() - 1;

Can m_symbols be empty?

> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:89
> +    if (newValue == static_cast<int>(m_symbols.size()))

Nicer to use >=

Created attachment 407882 [details]
Patch for landing

(In reply to Darin Adler from comment #2)
> Comment on attachment 407795 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407795&action=review
> 
> > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:102
> > +    if (key == "Up") {
> > +        keyboardEvent.setDefaultHandled();
> > +        stepUp();
> > +        return;
> > +    }
> > +
> > +    if (key == "Down") {
> > +        keyboardEvent.setDefaultHandled();
> > +        stepDown();
> > +        return;
> > +    }
> 
> Code above calls setDefaultHandled after handling. This code and the code
> below calls it before handling. I suggest we consider a consistent pattern
> of doing setDefaultHandled last.

Updated to call setDefaultHandled last.
 
> > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:102
> > +    m_typeAheadBuffer.clear();
> > +    setValueAsInteger(newValue, DispatchInputAndChangeEvents);
> 
> Would be nice to share this between up and down. I know it’s just two lines
> of code.

Added a method: setValueAsIntegerByStepping.
 
> > Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:82
> > +    int newValue = hasValue() ? m_selectedIndex - 1 : m_symbols.size() - 1;
> > +    if (newValue < 0)
> > +        newValue = m_symbols.size() - 1;
> 
> Can m_symbols be empty?

It cannot. There is an assertion in the constructor and m_symbols is never modified.
 
> > Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:89
> > +    if (newValue == static_cast<int>(m_symbols.size()))
> 
> Nicer to use >=

Done.

Committed r266524: <https://trac.webkit.org/changeset/266524>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407882 [details].

<rdar://problem/68283770>