Bug 79754

Summary: [Forms] Spin button sometimes ignores Indeterminate of m_upDownState
Product: WebKit Reporter: yosin
Component: FormsAssignee: yosin
Status: RESOLVED FIXED    
Severity: Normal CC: tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch 1
none
Patch 2
none
Patch 3 none

Description yosin 2012-02-27 23:46:39 PST
SpinButton implementation contains code like below:
  input->stepUpFromRenderer(m_upDownState == Up ? 1 : -1);

Member variable m_upDownState is type of enum UpDownState which has three values, Indeterminate, Down, and Up.
Above code can pass -1 to input->setUpFromRenderer if m_upDownState is Down or Indeterminate. We should not call input->setUpFromRenderer if m_upDownState is Indeterminate.
Comment 1 yosin 2012-02-28 00:43:37 PST
Created attachment 129209 [details]
Patch 1
Comment 2 Kent Tamura 2012-02-28 01:51:49 PST
Comment on attachment 129209 [details]
Patch 1

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

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:-297
> -                ASSERT(m_upDownState != Indeterminate);

If you think this assertion should be removed, please write a reason in ChangeLog.

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:299
> +                if (m_upDownState != Indeterminate) {
> +                  input->stepUpFromRenderer(m_upDownState == Up ? 1 : -1);
> +                  if (renderer())

Wrong indentation.
Comment 3 yosin 2012-02-28 02:05:34 PST
Created attachment 129217 [details]
Patch 2
Comment 4 Kent Tamura 2012-02-28 02:33:11 PST
Comment on attachment 129217 [details]
Patch 2

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

> Source/WebCore/ChangeLog:14
> +        m_upDownState can be Indeterminate at mousedown event if mouse pointer is on
> +        spin button when it is displayed.
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests. No behavior change.

This patch changes a behavior, and it seems you know how to reproduce the behavior change.  Please write a test.

Also, usual position of "Reviewed by .." line is just after the bug URL, before the long description.
Comment 5 yosin 2012-02-28 18:13:07 PST
Created attachment 129369 [details]
Patch 3
Comment 6 Kent Tamura 2012-02-28 18:21:27 PST
Comment on attachment 129369 [details]
Patch 3

Looks good.  Thanks!
Comment 7 WebKit Review Bot 2012-02-28 22:17:25 PST
Comment on attachment 129369 [details]
Patch 3

Clearing flags on attachment: 129369

Committed r109193: <http://trac.webkit.org/changeset/109193>
Comment 8 WebKit Review Bot 2012-02-28 22:17:30 PST
All reviewed patches have been landed.  Closing bug.