Bug 32813 – Type=number UI part 2: Mac implementation of spin-button

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

[Kent Tamura]

This bug was split from Bug#27968.

A review comment from Eric Seidel:
(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];

[Kent Tamura]

Created attachment 45318 [details]
Proposed patch

(In reply to comment #0)
> 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}

I followed other similar functions in this file. I removed it in the updated patch.

> No need:
> +    static NSStepperCell* cell;
> +    if (!cell) {
> +        cell = [[NSStepperCell alloc] init];
> +    }
> Just do:
> static NSStepperCell* cell = [[NSStepperCell alloc] init];

Fixed.

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

Fixed.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45318 [details] without any errors.

[Maciej Stachowiak]

Comment on attachment 45318 [details]
Proposed patch

Why is it ok that the stepper buttons are not aligned with their text fields? that seems like a bug.

[Kent Tamura]

Thank you for the comment.

(In reply to comment #3)
> (From update of attachment 45318 [details])
> Why is it ok that the stepper buttons are not aligned with their text fields?
> that seems like a bug.

Because the HTML/CSS in the test has no alignment code. The steppers are just <DIV>s.

Such layout should be done automatically without HTML/CSS hack.  I already have the code and it will be included in a future patch.

[Adele Peterson]

Could you include the alignment code?  Is it platform-specific, or general code?  I think it would help to review it at the same time, to make sure the approach is sensible.

[Kent Tamura]

Created attachment 48138 [details]
Proposed patch (rev.2)

[Kent Tamura]

(In reply to comment #5)
> Could you include the alignment code?  Is it platform-specific, or general
> code?  I think it would help to review it at the same time, to make sure the
> approach is sensible.

ok, I added the layout code to the patch.

[Darin Adler]

Comment on attachment 48138 [details]
Proposed patch (rev.2)

Looks good to me, but I think Adele may be the right person to review this. She's out until Monday.

[Adele Peterson]

Comment on attachment 48138 [details]
Proposed patch (rev.2)

The buttons don't quite look centered vertically

[Adele Peterson]

Comment on attachment 48138 [details]
Proposed patch (rev.2)

I agree that the rest of this looks pretty good.  tkent is going to upload a new patch to fix the centering issue.

[Kent Tamura]

Created attachment 49568 [details]
Proposed patch (rev.3)

[Kent Tamura]

(In reply to comment #11)
> Created an attachment (id=49568) [details]
> Proposed patch (rev.3)

- Updated vertical centering code
- Change the layout of input-appearance-spinbutton.html so that all text fields are in the viewport.

[Kent Tamura]

Thanks for r+.
I don't land this patch for now because the button has no functionsy. It's confusing.

I'll post another patch to add step-up/step-down functions.

[WebKit Commit Bot]

Attachment 49568 [details] was posted by a committer and has review+, assigning to Kent Tamura for commit.

[Kent Tamura]

Committed r58561: <http://trac.webkit.org/changeset/58561>