|
Description
Kent Tamura
2009-08-03 21:28:01 PDT
Created attachment 34811 [details]
Windows screenshot
This appearance was produced by my code (not a mock!)
Created attachment 34812 [details]
Mac screenshot
Drawn by actual code
Created attachment 34822 [details]
Proposed patch
---
33 files changed, 777 insertions(+), 46 deletions(-)
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:. That Windows screenshot seems to be missing a line on top of the top arrow. 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. I'll split the patch into 4. They need to be committed in order. 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
Created attachment 39538 [details]
Patch part 2: Mac implementation of outer-spin-button appearance
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.
Created attachment 39540 [details]
Patch part 4: Rendering of spin buttons and behavior for type=number
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?
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];
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.
Why are these called spin buttons if the Mac APIs call them steppers? Does windows call these "Spin buttons"? 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. 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. I'm moving all 4 patches to their own bugs and making this meta-bug. Sorry for the mess. http://trac.webkit.org/changeset/58564 might have broken Qt Linux ARMv5 Release and Qt Linux ARMv7 Release I think we have completed <input type=number>. |