Bug 216090

Summary: [macOS] Support stepping using keyboard in date inputs
Product: WebKit Reporter: Aditya Keerthi <akeerthi>
Component: FormsAssignee: Aditya Keerthi <akeerthi>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, changseok, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Mac   
OS: Unspecified   
Attachments:
Description Flags
Patch
darin: review+
Patch for landing none

Aditya Keerthi
Reported 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.
Attachments
Patch (10.20 KB, patch)
2020-09-02 13:24 PDT, Aditya Keerthi 		darin: review+
DetailsFormatted DiffDiff
Patch for landing (10.68 KB, patch)
2020-09-03 08:20 PDT, Aditya Keerthi 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Aditya Keerthi
Comment 1 2020-09-02 13:24:45 PDT
Created attachment 407795 [details] Patch
Darin Adler
Comment 2 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 >=
Aditya Keerthi
Comment 3 2020-09-03 08:20:09 PDT
Created attachment 407882 [details] Patch for landing
Aditya Keerthi
Comment 4 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.
EWS
Comment 5 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].
Radar WebKit Bug Importer
Comment 6 2020-09-03 09:29:13 PDT
<rdar://problem/68283770>
Note You need to log in before you can comment on or make changes to this bug.