[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 9e182989d2bdd0ff756e523be7dbbf2bd9de40c3..17e57e064029254abdb7d6868973e707c75db039 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_win.cc
@@ -466,6 +466,7 @@ OmniboxViewWin::OmniboxViewWin(OmniboxEditController* controller,
       tracking_double_click_(false),
       double_click_time_(0),
       can_discard_mousemove_(false),
+      caret_height_(0),

[Peter Kasting, 2012/12/01 01:48:31]

Probably unnecessary, see below.

[Mathieu, 2012/12/03 02:29:04]

Done.

       ignore_ime_messages_(false),
       delete_at_end_pressed_(false),
       font_(parent_view->font()),
@@ -606,6 +607,7 @@ void OmniboxViewWin::Update(const WebContents* tab_for_state_restoring) {
         tab_for_state_restoring->GetUserData(&kAutocompleteEditStateKey));
     if (state) {
       model()->RestoreState(state->model_state);
+      ApplyFocusVisibility();
 
       // Restore user's selection.  We do this after restoring the user_text
       // above so we're selecting in the correct string.
@@ -760,6 +762,14 @@ void OmniboxViewWin::UpdatePopup() {
 
 void OmniboxViewWin::SetFocus() {
   ::SetFocus(m_hWnd);
+  model()->set_is_focus_visible(true);
+  ApplyFocusVisibility();

[Peter Kasting, 2012/12/01 01:48:31]

Rather than calling this unconditionally, we should probably only do it when the
focus visibility status changes.

One way to handle this would be to make the edit model setter call back to the
view when the requested visibility actually differs.  This can also allow us to
get rid of SetInvisibleFocus() entirely if its current callers either are
expected to do "view->SetFocus(); model->SetFocusVisible(false);", or if the
callers are expected to only ask for invisible focus when the omnibox already
has focus.

[Mathieu, 2012/12/03 02:29:04]

Good idea. Done.

+}
+
+void OmniboxViewWin::SetInvisibleFocus() {
+  ::SetFocus(m_hWnd);
+  model()->set_is_focus_visible(false);
+  ApplyFocusVisibility();
 }
 
 void OmniboxViewWin::SetDropHighlightPosition(int position) {
@@ -844,6 +854,8 @@ void OmniboxViewWin::OnRevertTemporaryText() {
 }
 
 void OmniboxViewWin::OnBeforePossibleChange() {
+  model()->set_is_focus_visible(true);
+  ApplyFocusVisibility();
   // Record our state.
   text_before_change_ = GetText();
   GetSelection(sel_before_change_);
@@ -1894,6 +1906,12 @@ void OmniboxViewWin::OnPaint(HDC bogus_hdc) {
          rect.left, rect.top, SRCCOPY);
   memory_dc.SelectBitmap(old_bitmap);
   edit_hwnd = old_edit_hwnd;
+
+  // Determine the caret height and apply the current focus visibility. This is
+  // done in every paint event to account for window resizes that repaint the
+  // Omnibox.
+  caret_height_ = rect.Height() - 3;
+  ApplyFocusVisibility();
 }
 
 void OmniboxViewWin::OnPaste() {
@@ -2753,3 +2771,14 @@ int OmniboxViewWin::WidthNeededToDisplay(const string16& text) const {
   // PosFromChar(i) might return 0 when i is greater than 1.
   return font_.GetStringWidth(text) + GetHorizontalMargin();
 }
+
+void OmniboxViewWin::ApplyFocusVisibility() {
+  if (!chrome::search::IsInstantExtendedAPIEnabled(parent_view_->profile()))
+    return;
+  HideCaret();

[Peter Kasting, 2012/12/01 01:48:31]

Why are we hiding the caret unconditionally instead of in an else clause?

[Mathieu, 2012/12/03 02:29:04]

Ah, the Windows API... If DestroyCaret is called at the moment when the caret is
in the visible part of the blinking, it will stay a solid, non-blinking bar.
Hiding it prior to destroying solves that.

+  ::DestroyCaret();

[Peter Kasting, 2012/12/01 01:48:31]

Do we have to DestroyCaret() and later CreateCaret()?  Why is just
HideCaret()/ShowCaret() insufficient?  Note that the omnibox will never
dynamically change height.

[Mathieu, 2012/12/03 02:29:04]

The Hide/Show Windows API is broken in this way: when the view is repainted, the
caret is automatically Show()'ed again (More specifically, HideCaret is called
by Windows in BeginPaint, and ShowCaret is called in EndPaint; this is to avoid
showing the caret when a character is painted). Therefore, we cannot really
trust Hide to stick for a long time (a dirty omnibox, such as in the case of a
window resize, will make the caret show again). Destroying and Creating it again
solves that.

[Peter Kasting, 2012/12/03 23:03:32]

Interesting.  You should add comments about this.

We're already hooking Begin/EndPaint (see OnPaint()).  Can we just insert a
HideCaret() call (if it should be currently hidden) after the DefWindowProc()
call in that function?

The advantage to this is that we won't need |caret_height_| and we won't have to
worry about anything weird where the caret style or color or something is for
some crazy reason different than what we're specifying here.

[Mathieu, 2012/12/03 23:23:32]

I've now added comments where I felt appropriate. Should have done it before,
sorry.

As for your suggestion, this was indeed my first approach and this one is based
off it (ApplyFocusVisibility is called at the end of OnPaint, after the
DefWindowProc). I've moved to the Destroy/Create approach because Hiding and
Showing wasn't consistent, to the point where Philippe and I were almost pulling
our hair out. According to the Windows API documentation, Hide and Show are
cumulative -- calling Hide five times will need five Show calls. In our
experience, it wasn't the case and was very inconsistent (we several scenarios,
including gating the Hide and Show calls to make sure they were only called to
cancel each other out). I believe that Destroy/Create is our only hope at this
moment.

As for different Caret shapes or color, the APIs do not exist on Windows (I wish
changing the caret color existed!). The only call that can modify the shape of
the caret is CreateCaret, and the only place it's called in Chromium is here 

http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/base/win/ime_input.cc&e...


And the shape is not altered.

+  if (model()->is_focus_visible()) {
+    ::CreateCaret(m_hWnd, (HBITMAP) NULL, 1, caret_height_);
+    ShowCaret();
+  }
+}