Bug 27968 – <input type=number> UI - Master bug

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

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 34811 [details]
Windows screenshot

This appearance was produced by my code (not a mock!)

[Kent Tamura]

Created attachment 34812 [details]
Mac screenshot

Drawn by actual code

[Kent Tamura]

Created attachment 34822 [details]
Proposed patch


---
 33 files changed, 777 insertions(+), 46 deletions(-)

[Kent Tamura]

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:.

[Darin Adler]

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

[Kent Tamura]

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.

[Kent Tamura]

I'll split the patch into 4.
They need to be committed in order.

[Kent Tamura]

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

[Kent Tamura]

Created attachment 39538 [details]
Patch part 2: Mac implementation of outer-spin-button appearance

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 39540 [details]
Patch part 4: Rendering of spin buttons and behavior for type=number

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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];

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Why are these called spin buttons if the Mac APIs call them steppers?  Does windows call these "Spin buttons"?

[Eric Seidel (no email)]

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.

[David Levin]

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.

[Kent Tamura]

I'm moving all 4 patches to their own bugs and making this meta-bug.
Sorry for the mess.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58564 might have broken Qt Linux ARMv5 Release and Qt Linux ARMv7 Release

[Kent Tamura]

I think we have completed <input type=number>.