Summary: | Type=number UI part 2: Mac implementation of spin-button | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||
Component: | Forms | Assignee: | Kent Tamura <tkent> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | adele, commit-queue, eric, mjs, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | OS X 10.5 | ||||||||||
Bug Depends on: | 31821 | ||||||||||
Bug Blocks: | 27968, 35686 | ||||||||||
Attachments: |
|
Description
Kent Tamura
2009-12-20 22:47:07 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. 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> |