Bug 27968 - <input type=number> UI - Master bug
: <input type=number> UI - Master bug
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
: http://www.whatwg.org/specs/web-apps/...
:
: 27451 29069 31821 32813 35686 38380 38381 38568 38570 42076 42440 42441 42484 43463 44411 44476
: 19264 29359
  Show dependency treegraph
 
Reported: 2009-08-03 21:28 PST by
Modified: 2011-03-01 22:57 PST (History)


Attachments
Windows screenshot (3.33 KB, image/png)
2009-08-13 22:33 PST, Kent Tamura
no flags Details
Mac screenshot (4.83 KB, image/png)
2009-08-13 22:36 PST, Kent Tamura
no flags Details
Proposed patch (61.06 KB, patch)
2009-08-14 02:09 PST, Kent Tamura
no flags Review Patch | Details | Formatted Diff | Diff
Screen shot of a dialog in Vista (50.56 KB, image/png)
2009-08-17 21:42 PST, Kent Tamura
no flags Details
Patch part 1: definition of pseudo classes and appearances (12.94 KB, patch)
2009-09-14 01:48 PST, Kent Tamura
no flags Review Patch | Details | Formatted Diff | Diff
Patch part 2: Mac implementation of outer-spin-button appearance (8.85 KB, patch)
2009-09-14 01:49 PST, Kent Tamura
eric: review-
Review Patch | Details | Formatted Diff | Diff
Patch part 3: Windows implementation of inner-spin-button appearance (9.26 KB, patch)
2009-09-14 01:50 PST, Kent Tamura
no flags Review Patch | Details | Formatted Diff | Diff
Patch part 4: Rendering of spin buttons and behavior for type=number (32.01 KB, patch)
2009-09-14 01:52 PST, Kent Tamura
levin: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-08-03 21:28:01 PST
The UI will be like http://docs.google.com/View?id=dch3zh37_0cf8kc8c4
That is to say, adding a NSStepperCell to the outside of the border on Mac, adding a spin control to the inside of the border on Windows.

I'm making a patch.
------- Comment #1 From 2009-08-13 22:33:37 PST -------
Created an attachment (id=34811) [details]
Windows screenshot

This appearance was produced by my code (not a mock!)
------- Comment #2 From 2009-08-13 22:36:55 PST -------
Created an attachment (id=34812) [details]
Mac screenshot

Drawn by actual code
------- Comment #3 From 2009-08-14 02:09:48 PST -------
Created an attachment (id=34822) [details]
Proposed patch


---
 33 files changed, 777 insertions(+), 46 deletions(-)
------- Comment #4 From 2009-08-14 02:26:22 PST -------
Oops, the patch doesn't contain LayoutTests/platform/mac/fast/forms/input-number-expected.png.  bugzilla-tool doesn't support for binary files?

I don't have a Windows XP environment for now.  So the patch doesn't have tests for Windows.

Add more Forms people to Cc:.
------- Comment #5 From 2009-08-14 10:37:03 PST -------
That Windows screenshot seems to be missing a line on top of the top arrow.
------- Comment #6 From 2009-08-17 21:42:57 PST -------
Created an attachment (id=35014) [details]
Screen shot of a dialog in Vista

> That Windows screenshot seems to be missing a line on top of the top arrow.

It's normal for this size of the control.  We can find lack of the top border on some Window's standard dialogs.  I attach an example.
The top border is drawn for larger text box.
------- Comment #7 From 2009-09-14 01:44:40 PST -------
I'll split the patch into 4.
They need to be committed in order.
------- Comment #8 From 2009-09-14 01:48:04 PST -------
Created an attachment (id=39537) [details]
Patch part 1: definition of pseudo classes and appearances

Adds ::-webkit-inner-spin-button and ::-webkit-outer-spin-button CSS selectors
Adds -webkit-appearance:inner-spin-button and -webkit-appearance:outer-spin-button
------- Comment #9 From 2009-09-14 01:49:07 PST -------
Created an attachment (id=39538) [details]
Patch part 2: Mac implementation of outer-spin-button appearance
------- Comment #10 From 2009-09-14 01:50:41 PST -------
Created an attachment (id=39539) [details]
Patch part 3: Windows implementation of inner-spin-button appearance

This has no tests because DumpRenderTree doesn't work at all on my machine.
------- Comment #11 From 2009-09-14 01:52:03 PST -------
Created an attachment (id=39540) [details]
Patch part 4: Rendering of spin buttons and behavior for type=number
------- Comment #12 From 2009-09-23 10:31:35 PST -------
(From update of attachment 39537 [details])
The change looks OK.  Where did the "*-spin-button" names come from?  What does it have to do with spinning?  Can you post a picture of what these inputs are going to look like?
------- Comment #13 From 2009-09-23 10:34:10 PST -------
(From update of attachment 39538 [details])
This change is mostly useless w/o pixel results.  If we can't see what UI you're adding, we can't really review it.  Also your ChangeLogs are still kinda empty. :(

Why have this big function if you're only going to return 0?
+const int* stepperMargins(NSControlSize controlSize)
+{
+    static const int margins[3][4] = {
+        {0, 0, 0, 0},
+        {0, 0, 0, 0},
+        {0, 0, 0, 0}
+    };
+    return margins[controlSize];
+}

No need:
+    static NSStepperCell* cell;
+    if (!cell) {
+        cell = [[NSStepperCell alloc] init];
+    }
Just do:
static NSStepperCell* cell = [[NSStepperCell alloc] init];
and that will only run once, the first time the function is called.

Needs { } due to the comment:
+    if (states & PressedState && states & SpinUpState)
+        // FIXME: There is no way to draw a NSSteperCell with the up button hilighted.
+        // Disables the hilight of the down button if the up button is pressed.
+        [cell setHighlighted:NO];
------- Comment #14 From 2009-09-23 10:36:01 PST -------
(From update of attachment 39539 [details])
Adam Roben, or someone familiar with the windows code should review this.
------- Comment #15 From 2009-09-23 10:36:37 PST -------
Why are these called spin buttons if the Mac APIs call them steppers?  Does windows call these "Spin buttons"?
------- Comment #16 From 2009-09-23 10:38:18 PST -------
Why are they called inner and outer spin buttons if they're really up and down steppers?  Please cite the spec, or talk about where these names came from.  Ideally in your ChangeLogs, since that's what reviewers read.
------- Comment #17 From 2009-09-23 15:42:27 PST -------
(From update of attachment 39540 [details])
Quick pass with some things to fix. 

Please take each comment as something to look for throughout the patch (as things usually occur more than once).

Your changelog seems a bit sparse.
Please remove commented out code.
Please remove printf's.

>+ Test for the spin control behavior in a type=numnber input.
typo: numnber

> + #define EPSILON (16.0 * std::numeric_limits<double>::epsilon())

Why is this a define instead of a const double? Also epsilon doesn't seem like the right name since it is 16 * epsilon.

> + void HTMLInputElement::stepUpFromUI(int n)
"n" A more meaningful variable name would be better.


> + void paintBoxDecorationsWithSize(PaintInfo&, int, int, int, int);

Don't include parameter names if they don't add information but in this case the names for the int parameters would add a lot of information.

This occurs in several places but I just picked out one.

> + // Center the spin button vertically, and move it to the right
Finish sentences with "."

> + const MouseEvent* mevt = static_cast<MouseEvent*>(evt);
"mevt" Use whole words.
------- Comment #18 From 2009-11-23 22:24:55 PST -------
I'm moving all 4 patches to their own bugs and making this meta-bug.
Sorry for the mess.
------- Comment #19 From 2010-04-30 02:11:02 PST -------
http://trac.webkit.org/changeset/58564 might have broken Qt Linux ARMv5 Release and Qt Linux ARMv7 Release
------- Comment #20 From 2011-03-01 22:57:08 PST -------
I think we have completed <input type=number>.