Bug 38568

Summary: <input type=number> UI: Support disabled/readonly states
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, adele, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 27968, 41924, 41925    
Attachments:
Description Flags
Proposed patch
none
Patch 2
none
Patch 3
none
Patch 4 fishd: review+

Description Kent Tamura 2010-05-05 00:51:04 PDT
<input type=number> UI: Support disabled/readonly states
Comment 1 Kent Tamura 2010-05-05 01:34:23 PDT
Created attachment 55098 [details]
Proposed patch
Comment 2 Adam Barth 2010-06-21 18:19:25 PDT
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.
Comment 3 Kent Tamura 2010-06-24 04:23:34 PDT
Created attachment 59633 [details]
Patch 2
Comment 4 WebKit Review Bot 2010-06-24 04:25:00 PDT
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.
Comment 5 Kent Tamura 2010-06-24 04:28:42 PDT
Created attachment 59634 [details]
Patch 3
Comment 6 Kent Tamura 2010-06-24 04:29:01 PDT
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.
Comment 7 Adam Barth 2010-07-07 00:57:45 PDT
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?
Comment 8 Kent Tamura 2010-07-11 07:55:20 PDT
Created attachment 61176 [details]
Patch 4
Comment 9 Darin Fisher (:fishd, Google) 2010-07-13 23:32:38 PDT
Comment on attachment 61176 [details]
Patch 4

r=me
Comment 10 Kent Tamura 2010-07-14 00:43:30 PDT
Committed r63286: <http://trac.webkit.org/changeset/63286>