Bug 6812 - Missing focus ring on new text fields
Summary: Missing focus ring on new text fields
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Adele Peterson
Depends on:
Blocks: 6986
  Show dependency treegraph
Reported: 2006-01-25 18:34 PST by Adele Peterson
Modified: 2006-02-16 14:45 PST (History)
0 users

See Also:

test case (319 bytes, text/html)
2006-02-15 16:09 PST, Adele Peterson
no flags Details
focus patch (1.94 KB, patch)
2006-02-15 16:11 PST, Adele Peterson
no flags Details | Formatted Diff | Diff
patch w/ Changelog (2.82 KB, patch)
2006-02-15 17:24 PST, Adele Peterson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adele Peterson 2006-01-25 18:34:20 PST
Comment 1 Adele Peterson 2006-01-25 20:38:43 PST
I tried adding a case for TextFieldAppearance in RenderObject::paintOutline, and the outline it draws is the wrong size - its inside the NSTextFieldCell border.
Comment 2 Darin Adler 2006-01-28 17:12:37 PST
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.
Comment 3 Adele Peterson 2006-02-15 16:09:58 PST
Created attachment 6517 [details]
test case
Comment 4 Adele Peterson 2006-02-15 16:11:32 PST
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.
Comment 5 Adele Peterson 2006-02-15 17:10:38 PST
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.
Comment 6 Adele Peterson 2006-02-15 17:24:46 PST
Created attachment 6522 [details]
patch w/ Changelog
Comment 7 Justin Garcia 2006-02-16 00:32:03 PST
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.
Comment 8 Adele Peterson 2006-02-16 10:16:03 PST
I've made justin's recommended changes
Comment 9 Darin Adler 2006-02-16 12:59:05 PST
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;
Comment 10 Dave Hyatt 2006-02-16 13:08:20 PST
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.
Comment 11 Adele Peterson 2006-02-16 13:16:47 PST
Dave's suggestion makes sense.  I will make that change before committing.