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];
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.
style-queue ran check-webkit-style on attachment 45318 [details] without any errors.
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.
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.
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.
Created attachment 48138 [details] Proposed patch (rev.2)
(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 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 on attachment 48138 [details] Proposed patch (rev.2) The buttons don't quite look centered vertically
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.
Created attachment 49568 [details] Proposed patch (rev.3)
(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.
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.
Attachment 49568 [details] was posted by a committer and has review+, assigning to Kent Tamura for commit.
Committed r58561: <http://trac.webkit.org/changeset/58561>