[Search] Implementation of the invisible focus on Windows

chrome/browser/ui/views/omnibox/omnibox_view_win.cc

Index: chrome/browser/ui/views/omnibox/omnibox_view_win.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_win.cc b/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
index cdfce5fc6b9f1a7eb3e29d7aae290b38491a33b1..1ee986b6e1c1dbbfab07c5374406fbfba1fffca2 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
@@ -760,11 +760,30 @@ void OmniboxViewWin::UpdatePopup() {
 
 void OmniboxViewWin::SetFocus() {
   ::SetFocus(m_hWnd);
+  model()->SetFocusVisibility(true);

[Peter Kasting, 2012/12/04 02:18:30]

See question in the other CL about this particular call.

[Mathieu, 2012/12/04 17:00:14]

Acknowledged. Will wait for the outcome of that comment.

 }
 
 void OmniboxViewWin::ApplyFocusVisibility() {
-  // TODO(mathp): implement for Windows.
-  NOTIMPLEMENTED();
+  if (!chrome::search::IsInstantExtendedAPIEnabled(parent_view_->profile()))

[Peter Kasting, 2012/12/04 02:18:30]

I don't think it's appropriate to check this here.  The omnibox view shouldn't
second-guess the model on whether it's appropriate to do this.  The instant
controller, if anywhere, seems like a better place for this check.

[Mathieu, 2012/12/04 17:00:14]

I was trying to minimize the manipulation of the Caret for users that don't have
instant extended enabled.

[Peter Kasting, 2012/12/04 21:24:43]

If they don't have instant extended enabled, this should never be called, should
it?

I just think this is a layering violation.  If we need this functionality for
some other feature someday, we're not going to want to reach into the
platform-specific implementations to modify some safety checks.

[Mathieu, 2012/12/04 22:30:51]

Well there is an ApplyCaretVisibility() call in OnPaint, so it will get called.
You're right that it is a violation, though.

+    return;
+  // We hide the caret just before destroying it, since destroying a caret that

[Peter Kasting, 2012/12/04 02:18:30]

I still don't see why the hide/destroy part is done unconditionally as opposed
to being done iff focus is not visible.

[Mathieu, 2012/12/04 17:00:14]

Moved the DestroyCaret() call in an else, but that's the most I can do. Not
hiding the caret before REcreating it causes issues (a solid caret).

[Peter Kasting, 2012/12/04 21:24:43]

Then don't put any of it in an else, and add comments about this.

[Mathieu, 2012/12/04 22:30:51]

Done.

+  // is in the "solid" phase of its blinking will leave a solid vertical bar.
+  HideCaret();
+  // We use DestroyCaret/CreateCaret instead of simply HideCaret/ShowCaret
+  // because HideCaret is not sticky across paint events, e.g. a window resize
+  // will effectively restore caret visibility, regardless of whether HideCaret
+  // was called before.

[Peter Kasting, 2012/12/04 02:18:30]

Nit: And add "While we do catch and handle these paint events (see OnPaint()),
it doesn't seem to be enough to simply call HideCaret() while handling them. 
..." and then a brief explanation of why/what problems you have.

[Mathieu, 2012/12/04 17:00:14]

Done.

+  ::DestroyCaret();
+  if (model()->is_focus_visible()) {
+    // Get caret height from omnibox height.
+    CRect rect;
+    GetClientRect(&rect);
+    int caret_height = rect.Height() - 3;

[Peter Kasting, 2012/12/04 02:18:30]

I suspect this calculation is wrong to hardcode "client height - 3".  You
probably want font_.GetBaseline() - font_y_adjustment_, but test how that looks
with very large system default fonts (which should be shown with the tops and
bottoms clipped off in the omnibox).

[Mathieu, 2012/12/04 17:00:14]

The 3 points seem odd to me too, I agree. I looked into it, and referring to
line 2459 or so and below, we can see that the scheme_rect, used to draw a slash
in the scheme, is created with a height declared as the following:

font_top + font_.GetBaseline() + kAdditionalSpaceOutsideFont 

In my test scenario, 
font_top = client_rect.top (0) + font_y_adjustment_ (1) = 1
font_.GetBaseline() = 16
kAdditionalSpaceOutsideFont = 3, set unconditionally in the previous lines as
SkIntToScalar(2) * 1.5f

for a total of 1 + 16 + 3 = 20. 

My previous approach, which gives the correct caret height, is:
rect.Height() - 3 = 20

So we see the constant "3" points in existing code. I like the rect.Height()
approach because it is in relation to the height of the omnibox, and not of font
adjustments. I've tried making the text bigger on this Windows machine, but
could only find the option to "Make text and other items larger or smaller",
which is a DPI scaling and in which case the points measurements above still
hold. 

If you know of a way to test unreasonably large fonts on windows, or someone who
could help, I'd be interested in knowing. 

[Peter Kasting, 2012/12/04 21:24:43]

You don't want to refer to that rect.  It's not guaranteed to have any relation
to the cursor height.

My directions before weren't right either, though, because for some reason I was
thinking the cursor should extend down to the baseline.  It doesn't, it takes
the full font height.  So I think what you want here is either just
font_.GetHeight() or maybe std::min(font_.GetHeight(), client_rect_height). 
Testing instructions below.
 
Control Panel -> Appearance and Personalization -> Change window glass colors ->
Advanced appearance settings... -> change the Message Box font size to something
larger or smaller. 

[Mathieu, 2012/12/04 22:30:51]

Wow, it was indeed font_.GetHeight()!

I tested with the biggest font size I could, and this works. Thanks!

+    ::CreateCaret(m_hWnd, (HBITMAP) NULL, 1, caret_height);
+    // According to the Windows API documentation, a newly created caret needs
+    // ShowCaret to be visible.
+    ShowCaret();
+  }
 }
 
 void OmniboxViewWin::SetDropHighlightPosition(int position) {
@@ -849,6 +868,7 @@ void OmniboxViewWin::OnRevertTemporaryText() {
 }
 
 void OmniboxViewWin::OnBeforePossibleChange() {
+  model()->SetFocusVisibility(true);

[Peter Kasting, 2012/12/04 02:18:30]

Nit: Add comments about this.

[Mathieu, 2012/12/04 17:00:14]

Done.

   // Record our state.
   text_before_change_ = GetText();
   GetSelection(sel_before_change_);
@@ -1899,6 +1919,8 @@ void OmniboxViewWin::OnPaint(HDC bogus_hdc) {
          rect.left, rect.top, SRCCOPY);
   memory_dc.SelectBitmap(old_bitmap);
   edit_hwnd = old_edit_hwnd;
+
+  ApplyFocusVisibility();

[Peter Kasting, 2012/12/04 02:18:30]

Do you actually need to do this on every paint call?  I thought since you
destroyed the caret that you'd be fine until the visibility changed?

[Mathieu, 2012/12/04 17:00:14]

I've tested this, and it is not so. This call needs to stay, or the caret shows
up on a window resize. I guess we can see Destroy and Create as Hide/Show calls
that actually work.

[Peter Kasting, 2012/12/04 21:24:43]

Then you need to add commentary about this too.

The theme here is that your code should contain comments about every single
piece of information you found throughout your testing and implementation
process.  Someone later isn't going to have this knowledge otherwise and will be
mystified.

[Mathieu, 2012/12/04 22:30:51]

Got it. Makes sense, thanks.

 }
 
 void OmniboxViewWin::OnPaste() {