[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..2f355d9c0c092abe074d1d36078b238a3ba13f70 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
@@ -759,12 +759,43 @@ void OmniboxViewWin::UpdatePopup() {
 }
 
 void OmniboxViewWin::SetFocus() {
+  // Restore caret visibility if focused explicitly. We need to do this here
+  // because if we already have invisible focus, the ::SetFocus() call below
+  // will short-circuit, preventing us from reaching

[Peter Kasting, 2012/12/05 02:28:50]

You did verify this short-circuiting occurs on Windows, right?  I just want to
make sure we actually need this.

[Mathieu, 2012/12/05 03:57:21]

Modified the comment to better reflect that in certain cases (such as when a new
tab is created, OnSetFocus is not called.

+  // OmniboxEditModel::OnSetFocus(), which handles restoring visibility when we
+  // didn't previously have focus.
   ::SetFocus(m_hWnd);
-}
-
-void OmniboxViewWin::ApplyFocusVisibility() {
-  // TODO(mathp): implement for Windows.
-  NOTIMPLEMENTED();
+  model()->SetCaretVisibility(true);

[Peter Kasting, 2012/12/05 02:28:50]

Nit: This should be next to the comment.

[Mathieu, 2012/12/05 03:57:21]

Done.

+}
+
+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 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()).
+  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) {
@@ -1688,6 +1719,11 @@ void OmniboxViewWin::OnLButtonDown(UINT keys, const CPoint& point) {
   OnAfterPossibleChange();
 
   gaining_focus_.reset();
+
+  // Restore caret visibility whenever the user clicks in the the omnibox. This
+  // is not always covered by OnSetFocus() because when clicking while the
+  // omnibox has invisible focus does not trigger a new OnSetFocus() call.
+  model()->SetCaretVisibility(true);

[Peter Kasting, 2012/12/05 02:28:50]

A couple things.

(1) Left clicks aren't enough, you need right clicks too.
(2) You also need to simulate the "gaining focus" behavior so we modify the
selection properly.

I think you may be able to achieve both of these by removing the call here and
instead modifying OnMouseActivate() to:
(1) Check "if ((!model()->has_focus() || !model()->is_caret_visible()) && ..."
(2) Place this call inside that conditional, next to the setting of
|gaining_focus_|.

You should test this to verify that it works.  You should probably try to rig up
a test environment like follows:

* Every n seconds, we ask the omnibox to make its focus invisible
* Then, test with the omnibox empty; containing text where the caret was
previously at the beginning/middle/end; and containing text with a partial and a
complete selection
* Test left/right/middle clicks, drags, double and triple clicks

[Mathieu, 2012/12/05 03:57:21]

Your proposal was almost correct; I need to check if it already has focus
(invisible focus) and the proposed "if" clause would have not done the trick.
Let me know what you think of this implementation.

As far as testing, I have tested this with left click and right click, triple
and double clicks. Dragging something on the omnibox does not activate this
event. I can't reproduce a middle click on this RDP setup (I'm at home at the
moment). I have not implemented the "invisible focus every n seconds"  test
because we should never have text in the omnibox and have invisible focus, and
would like to worry about these cases in another effort if possible. If there is
an essential case I missed or if you disagree, let me know.

 }
 
 void OmniboxViewWin::OnLButtonUp(UINT keys, const CPoint& point) {
@@ -1899,6 +1935,12 @@ 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). This is because the
+  // underlying edit control will automatically re-create the caret when it
+  // receives a paint event, e.g. a window resize event.

[Peter Kasting, 2012/12/05 02:28:50]

A resize event and a paint event aren't the same thing -- maybe you mean "when
it receives certain events that trigger repaints, e.g. window resize events"?

[Mathieu, 2012/12/05 03:57:21]

Done.

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