Bug 38568 - <input type=number> UI: Support disabled/readonly states
Summary: <input type=number> UI: Support disabled/readonly states
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 27968 41924 41925
  Show dependency treegraph
 
Reported: 2010-05-05 00:51 PDT by Kent Tamura
Modified: 2010-07-14 00:43 PDT (History)
3 users (show)

See Also:


Attachments
Proposed patch (37.25 KB, patch)
2010-05-05 01:34 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (37.87 KB, patch)
2010-06-24 04:23 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (37.75 KB, patch)
2010-06-24 04:28 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 4 (37.16 KB, patch)
2010-07-11 07:55 PDT, Kent Tamura
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>