Bug 89439 - [Forms] Make step action of SpinButtonElement replaceable
Summary: [Forms] Make step action of SpinButtonElement replaceable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL:
Keywords:
Depends on:
Blocks: 88970
  Show dependency treegraph
 
Reported: 2012-06-19 00:20 PDT by yosin
Modified: 2012-06-20 22:11 PDT (History)
2 users (show)

See Also:


Attachments
Patch 1 (10.73 KB, patch)
2012-06-19 01:02 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 2 (12.00 KB, patch)
2012-06-19 02:14 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 3 (21.66 KB, patch)
2012-06-19 18:45 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 4 (11.70 KB, patch)
2012-06-20 20:40 PDT, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-06-19 00:20:23 PDT
To introduce time fields UI, we would like to use SpinButtonElement and handle step action different from current implementation, it is HTMLInputElement::stepFromRenderer.
Comment 1 yosin 2012-06-19 01:02:55 PDT
Created attachment 148278 [details]
Patch 1
Comment 2 yosin 2012-06-19 01:03:56 PDT
Comment on attachment 148278 [details]
Patch 1

Could you review this patch?
Thanks in advance.

* This is a part of time fields UI.
Comment 3 Kent Tamura 2012-06-19 01:25:32 PDT
Comment on attachment 148278 [details]
Patch 1

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

> Source/WebCore/html/TextFieldInputType.h:87
>  
> +    virtual void spinButtonStepDown() OVERRIDE;
> +    virtual void spinButtonStepUp() OVERRIDE;

nit: Add a comment like "// SpinButtonElement::Callback functions."

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:301
>                  if (m_upDownState != Indeterminate) {
> -                    input->stepUpFromRenderer(m_upDownState == Up ? 1 : -1);
> +                    if (m_upDownState == Up)
> +                        m_callback.spinButtonStepUp();
> +                    else
> +                        m_callback.spinButtonStepDown();

How do you make sure m_callback object is live after spinButtonStepUp/Down calls, which dispatch 'change' event and JavaScript code change the input type.

> Source/WebCore/html/shadow/TextControlInnerElements.h:99
> +    class Callback {

The name 'Callback' sounds too generic.
It should be more specific.  e.g. ValueUpdater, ValueChangeDelegate, ...

> Source/WebCore/html/shadow/TextControlInnerElements.h:102
> +        virtual void spinButtonStepDown() = 0;
> +        virtual void spinButtonStepUp() = 0;

Why do you separate the functionality of stepUpFromRenderer() into two functions? It makes the patch larger.
Comment 4 yosin 2012-06-19 01:34:44 PDT
(In reply to comment #3)
> > Source/WebCore/html/shadow/TextControlInnerElements.h:102
> > +        virtual void spinButtonStepDown() = 0;
> > +        virtual void spinButtonStepUp() = 0;
> 
> Why do you separate the functionality of stepUpFromRenderer() into two functions? It makes the patch larger.

* We support only 1 or -1.
** It seems stepUp(-1) == stepDown(1) isn't so clear.
** We may not want to check amount is zero or not.
** In stepUpFromRenderer, we have ASSERT(n) and if (!n) return
* DateTimeNumericFieldElement, we may not want to write
https://bugs.webkit.org/attachment.cgi?id=147253&action=prettypatch

void DateTimeNumericFieldElement::spinButtonStep(int amount)
{
    if (!amount)
       return;

    if (m_hasValue)
        if (amount > 0)
            m_value = m_value == m_minimum ? m_maximum : ensureInRange(m_value -   1);
        else
            m_value = m_value == m_maximum ? m_minimum : ensureInRange(m_value +  1);

    else
        m_value = amount > 0 ? m_maximum : m_minimum;

    m_hasValue = true;
    updateText();
}
Comment 5 yosin 2012-06-19 02:14:30 PDT
Created attachment 148290 [details]
Patch 2
Comment 6 yosin 2012-06-19 02:16:24 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > > Source/WebCore/html/shadow/TextControlInnerElements.h:102
> > > +        virtual void spinButtonStepDown() = 0;
> > > +        virtual void spinButtonStepUp() = 0;
> > 
> > Why do you separate the functionality of stepUpFromRenderer() into two functions? It makes the patch larger.
> 
> * We support only 1 or -1.
> ** It seems stepUp(-1) == stepDown(1) isn't so clear.
> ** We may not want to check amount is zero or not.
> ** In stepUpFromRenderer, we have ASSERT(n) and if (!n) return
> * DateTimeNumericFieldElement, we may not want to write
> https://bugs.webkit.org/attachment.cgi?id=147253&action=prettypatch
> 
> void DateTimeNumericFieldElement::spinButtonStep(int amount)
> {
>     if (!amount)
>        return;
> 
>     if (m_hasValue)
>         if (amount > 0)
>             m_value = m_value == m_minimum ? m_maximum : ensureInRange(m_value -   1);
>         else
>             m_value = m_value == m_maximum ? m_minimum : ensureInRange(m_value +  1);
> 
>     else
>         m_value = amount > 0 ? m_maximum : m_minimum;
> 
>     m_hasValue = true;
>     updateText();
> }

Another option is using StepDirection enum, although we need to use switch with default ASSERT_NOT_REACHED. It is too long.
Comment 7 yosin 2012-06-19 02:18:29 PDT
Comment on attachment 148290 [details]
Patch 2

Could you review this patch?
Thanks in advance.

= Changes since last patch =
* Rename SpinButtonElement::Callback to StepActionHandler
* SpinButtonElement keeps m_stepActionHandler as pointer and checks it isn't nullptr before calling action.
* Add SpinButtonElement::removeStepActionHandler
** This method will be called from TextFieldInputType dtor and destroyShadowSubtree
Comment 8 Build Bot 2012-06-19 02:43:06 PDT
Comment on attachment 148290 [details]
Patch 2

Attachment 148290 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12988019
Comment 9 yosin 2012-06-19 03:51:05 PDT
Comment on attachment 148290 [details]
Patch 2

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

> Source/WebCore/html/InputType.h:299
> +    void stepUpFromRenderer(int);

We should move stepUpFromRenderer to TextFieldInputType, because only TextFieldInputType uses it.
Note: We need to expose applyStep as protected for this.

> Source/WebCore/html/shadow/TextControlInnerElements.h:101
> +        virtual void spinButtonStepDown() = 0;

We should have virtual destructor, clang issues warning for this.
Comment 10 yosin 2012-06-19 18:45:23 PDT
Created attachment 148480 [details]
Patch 3
Comment 11 yosin 2012-06-19 18:52:51 PDT
Comment on attachment 148480 [details]
Patch 3

Could you review this patch?
Thanks in advance.

* I would like to use separate methods for Step Up and Step Down. Because, 
** it isn't Intuitive that stepUp(-1)==stepDown(1)
** We don't want to check parameter isn't zero and to have ASSERT(n).
** I guess if we have stepUpFromRenderer and stepDownFromRenderer, it code could be simpler.

= Changes since last patch =
* Rename SpinButtonElement::Callback to StepActionHandler
* SpinButtonElement keeps m_stepActionHandler as pointer and checks it isn't nullptr before calling action.
* Add SpinButtonElement::removeStepActionHandler
** This method will be called from TextFieldInputType dtor and destroyShadowSubtree
* Add virtual destructor to StepButtonElement for Mac-clang
* Move InputType::stepUpFromRenderer to TextFieldInputType
* Expose InputType::applyStep as protected for stepUpFromRenderer
Comment 12 yosin 2012-06-19 21:23:15 PDT
Comment on attachment 148480 [details]
Patch 3

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

> Source/WebCore/html/TextFieldInputType.cpp:-155
> -    int step = 0;

Not using variable step makes patch size bigger, but new code is smaller than current and there is no extra variable initialization, although it will be removed by compiler but readers need to track how variable step is used.

> Source/WebCore/html/TextFieldInputType.cpp:-170
> -    int step = 0;

Not using variable step makes patch size bigger, but new code is smaller than current and there is no extra variable initialization, although it will be removed by compiler but readers need to track how variable step is used.
Comment 13 Kent Tamura 2012-06-20 02:37:54 PDT
Comment on attachment 148480 [details]
Patch 3

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

> Source/WebCore/ChangeLog:29
> +        (WebCore::TextFieldInputType::stepUpFromRenderer): Moved from InputType.

I object moving it.  Non-textfield UI might use this feature in the future.  This feature is not related to "text field" essentially.

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:303
> +                    if (m_stepActionHandler) {
> +                        if (m_upDownState == Up)
> +                            m_stepActionHandler->spinButtonStepUp();
> +                        else
> +                            m_stepActionHandler->spinButtonStepDown();
> +                    }

We had better share the code with the next part.

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:375
> +    if (!m_stepActionHandler)
> +        return;
> +
> +    if (amount > 0)
> +        m_stepActionHandler->spinButtonStepUp();
> +    else if (amount < 0)
> +        m_stepActionHandler->spinButtonStepDown();

We had better share the code with the previous part.

> Source/WebCore/html/shadow/TextControlInnerElements.h:106
> +    static PassRefPtr<SpinButtonElement> create(Document*, StepActionHandler&);

We should mention that a StepActionHandler implementation must call removeStepActionHandler().
Comment 14 yosin 2012-06-20 18:27:25 PDT
(In reply to comment #13)
> (From update of attachment 148480 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148480&action=review
> 
> > Source/WebCore/ChangeLog:29
> > +        (WebCore::TextFieldInputType::stepUpFromRenderer): Moved from InputType.
> 
> I object moving it.  Non-textfield UI might use this feature in the future.  This feature is not related to "text field" essentially.

stepUpFromRenderer is implementation of spin button event action and spin button is part of TextFieldInputType at this time.

If another feature wants to use stepUpFromRenderer, it will move back into InputType or another good place.

If we could have Steppable class, we would move stepUpFromRenderer to there. Although, RangeInputType should be Steppable but it is different from other Steppable classes.

I believe limiting scope of identifier helps us for future modification and maintenance.

Is stepUpBySpinButton better name?
Comment 15 yosin 2012-06-20 20:40:15 PDT
Created attachment 148722 [details]
Patch 4
Comment 16 yosin 2012-06-20 20:42:25 PDT
Comment on attachment 148722 [details]
Patch 4

Could you review this patch?
Thanks in advance.

== Changes since last review ==
* Don't move InputType::stepUpFromRenderer => No changes in InputType.{cpp,h} in this patch.
* Introduce SpinButtonElement::doStepAction
* Added comment for owner of SpinButtonElement must call removeActionHandler.
Comment 17 Kent Tamura 2012-06-20 21:14:33 PDT
Comment on attachment 148722 [details]
Patch 4

looks good.
Comment 18 WebKit Review Bot 2012-06-20 22:11:43 PDT
Comment on attachment 148722 [details]
Patch 4

Clearing flags on attachment: 148722

Committed r120903: <http://trac.webkit.org/changeset/120903>
Comment 19 WebKit Review Bot 2012-06-20 22:11:48 PDT
All reviewed patches have been landed.  Closing bug.