Bug 216090 - [macOS] Support stepping using keyboard in date inputs
Summary: [macOS] Support stepping using keyboard in date inputs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Safari Technology Preview
Hardware: Mac Unspecified
: P2 Normal
Assignee: Aditya Keerthi
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-02 12:20 PDT by Aditya Keerthi
Modified: 2020-09-03 09:29 PDT (History)
8 users (show)

See Also:


Attachments
Patch (10.20 KB, patch)
2020-09-02 13:24 PDT, Aditya Keerthi
darin: review+
Details | Formatted Diff | Diff
Patch for landing (10.68 KB, patch)
2020-09-03 08:20 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2020-09-02 12:20:13 PDT
Increment and decrement the value of the current focused field, in response to pressing the up and down arrow keys respectively.
Comment 1 Aditya Keerthi 2020-09-02 13:24:45 PDT
Created attachment 407795 [details]
Patch
Comment 2 Darin Adler 2020-09-02 21:13:36 PDT
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 >=
Comment 3 Aditya Keerthi 2020-09-03 08:20:09 PDT
Created attachment 407882 [details]
Patch for landing
Comment 4 Aditya Keerthi 2020-09-03 08:22:07 PDT
(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.
Comment 5 EWS 2020-09-03 09:28:17 PDT
Committed r266524: <https://trac.webkit.org/changeset/266524>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407882 [details].
Comment 6 Radar WebKit Bug Importer 2020-09-03 09:29:13 PDT
<rdar://problem/68283770>