Missing focus ring on new text fields

• Component: Forms
• Status: RESOLVED FIXED

[Adele Peterson]

[Adele Peterson]

I tried adding a case for TextFieldAppearance in RenderObject::paintOutline, and the outline it draws is the wrong size - its inside the NSTextFieldCell border.

[Darin Adler]

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.

[Adele Peterson]

Created attachment 6517 [details]
test case

[Adele Peterson]

Created attachment 6518 [details]
focus patch

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.

[Adele Peterson]

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.

[Adele Peterson]

Created attachment 6522 [details]
patch w/ Changelog

[Justin Garcia]

return !style->hasAppearance() || style->appearance() == TextFieldAppearance

is a little nicer than:

if (!style->hasAppearance() || style->appearance() == TextFieldAppearance)
   return true;
return false;

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.

[Adele Peterson]

I've made justin's recommended changes

[Darin Adler]

Comment on attachment 6522 [details]
patch w/ Changelog

Looks fine.

RenderTheme::engineShouldDrawFocusRing could be written as just a return statement, since if (x) return true; return false; is the same as return x;

[Dave Hyatt]

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.

[Adele Peterson]

Dave's suggestion makes sense.  I will make that change before committing.