Bug 38568 – <input type=number> UI: Support disabled/readonly states

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

<input type=number> UI: Support disabled/readonly states

[Kent Tamura]

Created attachment 55098 [details]
Proposed patch

[Adam Barth]

Comment on attachment 55098 [details]
Proposed patch

Mostly questions and nits.

WebCore/rendering/TextControlInnerElements.cpp:244
 +      bool insideBox = local.x() >= 0 && local.x() < box->width() && local.y() >= 0 && local.y() < box->height();
Why this manual comparison instead of the contains() call you're replacing below?

WebCore/rendering/TextControlInnerElements.cpp: 
 +              if (local.y() < renderBox()->y() + renderBox()->height() / 2)
What happened to the renderBox()->y() part of this computation?

WebCore/rendering/TextControlInnerElements.h:81
 +      virtual bool isEnabledFormControl() const { return static_cast<Element*>(const_cast<SpinButtonElement*>(this)->shadowAncestorNode())->isEnabledFormControl(); }
const_cast?  We should be able to write this code without const_cast.

[Kent Tamura]

Created attachment 59633 [details]
Patch 2

[WebKit Review Bot]

Attachment 59633 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/TextControlInnerElements.cpp:288:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kent Tamura]

Created attachment 59634 [details]
Patch 3

[Kent Tamura]

Thank you for reviewing.

(In reply to comment #2)
> (From update of attachment 55098 [details])
> Mostly questions and nits.
> 
> WebCore/rendering/TextControlInnerElements.cpp:244
>  +      bool insideBox = local.x() >= 0 && local.x() < box->width() && local.y() >= 0 && local.y() < box->height();
> Why this manual comparison instead of the contains() call you're replacing below?

I don't remember what I thought...
contains() should be ok.  I reverted this part.

> WebCore/rendering/TextControlInnerElements.cpp: 
>  +              if (local.y() < renderBox()->y() + renderBox()->height() / 2)
> What happened to the renderBox()->y() part of this computation?

'local' is relative to this RenderBox.  So we don't need to add renderBox()->y().

> WebCore/rendering/TextControlInnerElements.h:81
>  +      virtual bool isEnabledFormControl() const { return static_cast<Element*>(const_cast<SpinButtonElement*>(this)->shadowAncestorNode())->isEnabledFormControl(); }
> const_cast?  We should be able to write this code without const_cast.

I agree.  We need to make Node::shadowAncestroNode() const.  I'd like to address it in a separated bug.

[Adam Barth]

Comment on attachment 59634 [details]
Patch 3

I tried to review this patch, but I don't understand enough about rendering.

LayoutTests/fast/forms/input-appearance-spinbutton-disabled-readonly.html:4
 +  <link rel="stylesheet" href="../../../../fast/js/resources/js-test-style.css">
Woah, that's a crazy relative path.  Maybe we should copy that file into this directory?

[Kent Tamura]

Created attachment 61176 [details]
Patch 4

[Darin Fisher (:fishd, Google)]

Comment on attachment 61176 [details]
Patch 4

r=me

[Kent Tamura]

Committed r63286: <http://trac.webkit.org/changeset/63286>