[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..295f44d3bb392d4bf36703557e81e8f191b83e5c 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
@@ -760,11 +760,32 @@ void OmniboxViewWin::UpdatePopup() {
 
 void OmniboxViewWin::SetFocus() {
   ::SetFocus(m_hWnd);
-}
-
-void OmniboxViewWin::ApplyFocusVisibility() {
-  // TODO(mathp): implement for Windows.
-  NOTIMPLEMENTED();
+  model()->SetFocusVisibility(true);
+}
+
+void OmniboxViewWin::ApplyCaretVisibility() {
+  // We hide the caret just before destroying it, since destroying a caret that
+  // is in the "solid" phase of its blinking will leave a solid vertical bar.
+  // We also hide and destroy the caret before re-creating it, to avoid a solid
+  // vertical bar.

[Peter Kasting, 2012/12/04 22:45:35]

Nit: This second sentence might be clearer as:

"We even hide and destroy the caret if we're going to create it again below.  If
the caret was already visible on entry to this function, the CreateCaret() call
(which first destroys the old caret) might leave a solid vertical bar for the
same reason as above.  Unconditionally hiding prevents this.  The caret could be
visible on entry to this function if the underlying edit control had re-created
it automatically (see comments in OnPaint())."

Please correct me if I got the precise reasoning here incorrect.

[Mathieu, 2012/12/05 01:51:20]

Done. It's precisely right and well worded.

+  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. 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 because of the unpredictability of this
+  // Windows API. According to the documentation, it should be a cumulative call
+  // e.g. 5 hide calls should be balanced by 5 show calls. We have not found
+  // this to be true, which may be explained by the fact that this API is called
+  // internally in Windows, as well.
+  ::DestroyCaret();
+  if (model()->is_focus_visible()) {
+    ::CreateCaret(m_hWnd, (HBITMAP) NULL, 1, font_.GetHeight());
+    // According to the Windows API documentation, a newly created caret needs
+    // ShowCaret to be visible.
+    ShowCaret();
+  }
 }
 
 void OmniboxViewWin::SetDropHighlightPosition(int position) {
@@ -849,6 +870,10 @@ void OmniboxViewWin::OnRevertTemporaryText() {
 }
 
 void OmniboxViewWin::OnBeforePossibleChange() {
+  // We set the focus to visible here to cover two cases. This is called when
+  // the user directly clicks on the omnibox as well as when a character is
+  // about to be entered.
+  model()->SetFocusVisibility(true);
   // Record our state.
   text_before_change_ = GetText();
   GetSelection(sel_before_change_);
@@ -1899,6 +1924,11 @@ void OmniboxViewWin::OnPaint(HDC bogus_hdc) {
          rect.left, rect.top, SRCCOPY);
   memory_dc.SelectBitmap(old_bitmap);
   edit_hwnd = old_edit_hwnd;
+
+  // This needs to be called regardless of the current state of the caret, even
+  // if reaffirming a current state (hidden or shown). Otherwise, the caret may
+  // show when it is supposed to be hidden (i.e. it is re-created by the OS).

[Peter Kasting, 2012/12/04 22:45:35]

Nit: If you could be clearer in this second sentence when this happens, it'd be
good, e.g.

"This is because the underlying edit control will automatically re-create the
caret when it receives a size/move event."

This also suggests that maybe instead of putting this in OnPaint() we could put
it in some other more specific handler...

[Mathieu, 2012/12/05 01:51:20]

Done. I modified a little bit to say "paint events", of which resize is an
example. The move event is not a good example because Windows does not repaint
the window in this case (it probably just moves it in graphical memory).
Generally, if the textfield view is dirty, it will get repainted.

+  ApplyCaretVisibility();
 }
 
 void OmniboxViewWin::OnPaste() {