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

See Also:


Attachments
Windows screenshot (3.33 KB, image/png)
2009-08-13 22:33 PDT, Kent Tamura
no flags Details
Mac screenshot (4.83 KB, image/png)
2009-08-13 22:36 PDT, Kent Tamura
no flags Details
Proposed patch (61.06 KB, patch)
2009-08-14 02:09 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Screen shot of a dialog in Vista (50.56 KB, image/png)
2009-08-17 21:42 PDT, Kent Tamura
no flags Details
Patch part 1: definition of pseudo classes and appearances (12.94 KB, patch)
2009-09-14 01:48 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch part 2: Mac implementation of outer-spin-button appearance (8.85 KB, patch)
2009-09-14 01:49 PDT, Kent Tamura
eric: review-
Details | Formatted Diff | Diff
Patch part 3: Windows implementation of inner-spin-button appearance (9.26 KB, patch)
2009-09-14 01:50 PDT, Kent Tamura
no flags 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 PDT, Kent Tamura
levin: 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 2009-08-03 21:28:01 PDT
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 Kent Tamura 2009-08-13 22:33:37 PDT
Created attachment 34811 [details]
Windows screenshot

This appearance was produced by my code (not a mock!)
Comment 2 Kent Tamura 2009-08-13 22:36:55 PDT
Created attachment 34812 [details]
Mac screenshot

Drawn by actual code
Comment 3 Kent Tamura 2009-08-14 02:09:48 PDT
Created attachment 34822 [details]
Proposed patch


---
 33 files changed, 777 insertions(+), 46 deletions(-)
Comment 4 Kent Tamura 2009-08-14 02:26:22 PDT
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 Darin Adler 2009-08-14 10:37:03 PDT
That Windows screenshot seems to be missing a line on top of the top arrow.
Comment 6 Kent Tamura 2009-08-17 21:42:57 PDT
Created attachment 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 Kent Tamura 2009-09-14 01:44:40 PDT
I'll split the patch into 4.
They need to be committed in order.
Comment 8 Kent Tamura 2009-09-14 01:48:04 PDT
Created attachment 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 Kent Tamura 2009-09-14 01:49:07 PDT
Created attachment 39538 [details]
Patch part 2: Mac implementation of outer-spin-button appearance
Comment 10 Kent Tamura 2009-09-14 01:50:41 PDT
Created attachment 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 Kent Tamura 2009-09-14 01:52:03 PDT
Created attachment 39540 [details]
Patch part 4: Rendering of spin buttons and behavior for type=number
Comment 12 Eric Seidel 2009-09-23 10:31:35 PDT
Comment on attachment 39537 [details]
Patch part 1: definition of pseudo classes and appearances

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 Eric Seidel 2009-09-23 10:34:10 PDT
Comment on attachment 39538 [details]
Patch part 2: Mac implementation of outer-spin-button appearance

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 Eric Seidel 2009-09-23 10:36:01 PDT
Comment on attachment 39539 [details]
Patch part 3: Windows implementation of inner-spin-button appearance

Adam Roben, or someone familiar with the windows code should review this.
Comment 15 Eric Seidel 2009-09-23 10:36:37 PDT
Why are these called spin buttons if the Mac APIs call them steppers?  Does windows call these "Spin buttons"?
Comment 16 Eric Seidel 2009-09-23 10:38:18 PDT
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 David Levin 2009-09-23 15:42:27 PDT
Comment on attachment 39540 [details]
Patch part 4: Rendering of spin buttons and behavior for type=number

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 Kent Tamura 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 WebKit Review Bot 2010-04-30 02:11:02 PDT
http://trac.webkit.org/changeset/58564 might have broken Qt Linux ARMv5 Release and Qt Linux ARMv7 Release
Comment 20 Kent Tamura 2011-03-01 22:57:08 PST
I think we have completed <input type=number>.