I tried adding a case for TextFieldAppearance in RenderObject::paintOutline, and the outline it draws is the wrong size - its inside the NSTextFieldCell border.
These bugs that block us switching to the new text field shoul not be marked P1.
Once we turn it on, these would be P1/major bugs, of course, but at the moment they are just part of the "switch to a new text field" task and should not be in the P1 list.
Created attachment 6517 [details]
Created attachment 6518 [details]
We'll have to file followup bugs about minor differences in focus ring appearance. The ring is the same as the one we draw for contenteditable divs, but its not quite the same as the one AppKit draws.
Note- I explored using AppKit to draw this focus ring, but the default mechanism in NSTextFieldCell expects a contentView and a field editor. I think having the engine draw the ring is ultimately the right solution, and we'll have to iron out the visual details.
Created attachment 6522 [details]
patch w/ Changelog
return !style->hasAppearance() || style->appearance() == TextFieldAppearance
is a little nicer than:
if (!style->hasAppearance() || style->appearance() == TextFieldAppearance)
Also I think that shouldEngineDrawFocusRingForStyle, might be better than engineShouldDrawFocusRing. engineShouldDrawFocusRing sounds like you're telling someone something, not asking a question and shouldXXX seems to be the convention in WebCore.
I've made justin's recommended changes
Comment on attachment 6522 [details]
patch w/ Changelog
RenderTheme::engineShouldDrawFocusRing could be written as just a return statement, since if (x) return true; return false; is the same as return x;
I'd rather you just called this supportsFocusRing and inverted it... you're asking the theme if it can draw the focus ring. It should answer YES or NO. I don't like asking the theme if it "can't" draw the focus ring. Invert the API.
Dave's suggestion makes sense. I will make that change before committing.