Bug 32813 - Type=number UI part 2: Mac implementation of spin-button
Summary: Type=number UI part 2: Mac implementation of spin-button
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on: 31821
Blocks: 27968 35686
  Show dependency treegraph
 
Reported: 2009-12-20 22:47 PST by Kent Tamura
Modified: 2010-04-30 01:49 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (43.57 KB, patch)
2009-12-21 03:04 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.2) (76.78 KB, patch)
2010-02-04 07:33 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.3) (69.79 KB, patch)
2010-02-26 00:16 PST, Kent Tamura
no flags 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-12-20 22:47:07 PST
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];
Comment 1 Kent Tamura 2009-12-21 03:04:27 PST
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.
Comment 2 WebKit Review Bot 2009-12-21 03:05:13 PST
style-queue ran check-webkit-style on attachment 45318 [details] without any errors.
Comment 3 Maciej Stachowiak 2010-01-22 02:14:00 PST
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.
Comment 4 Kent Tamura 2010-01-22 02:25:08 PST
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.
Comment 5 Adele Peterson 2010-02-01 22:06:10 PST
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.
Comment 6 Kent Tamura 2010-02-04 07:33:55 PST
Created attachment 48138 [details]
Proposed patch (rev.2)
Comment 7 Kent Tamura 2010-02-04 07:36:29 PST
(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.
Comment 8 Darin Adler 2010-02-05 09:22:47 PST
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.
Comment 9 Adele Peterson 2010-02-25 22:20:40 PST
Comment on attachment 48138 [details]
Proposed patch (rev.2)

The buttons don't quite look centered vertically
Comment 10 Adele Peterson 2010-02-25 22:59:25 PST
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.
Comment 11 Kent Tamura 2010-02-26 00:16:07 PST
Created attachment 49568 [details]
Proposed patch (rev.3)
Comment 12 Kent Tamura 2010-02-26 00:18:40 PST
(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.
Comment 13 Kent Tamura 2010-03-02 00:42:41 PST
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.
Comment 14 WebKit Commit Bot 2010-03-05 14:05:00 PST
Attachment 49568 [details] was posted by a committer and has review+, assigning to Kent Tamura for commit.
Comment 15 Kent Tamura 2010-04-30 00:45:29 PDT
Committed r58561: <http://trac.webkit.org/changeset/58561>