[Search] Implementation of the invisible focus on Windows

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/omnibox/omnibox_view_win.h"
 
 #include <algorithm>
 #include <locale>
 #include <string>
 
@@ skipping 742 matching lines @@
   //   * The user has just pasted in something that replaced all the text
   //   * The user is trying to compose something in an IME
   CHARRANGE sel;
   GetSel(sel);
   model()->StartAutocomplete(sel.cpMax != sel.cpMin,
                             (sel.cpMax < GetTextLength()) || IsImeComposing());
 }
 
 void OmniboxViewWin::SetFocus() {
   ::SetFocus(m_hWnd);
+  // Restore caret visibility if focused explicitly. We need to do this here
+  // because the ::SetFocus() call will not reach
+  // OmniboxEditModel::OnSetFocus() in cases where we already had focus, such as
+  // when a new tab is created and the previous tab had invisible focus.

[Peter Kasting, 2012/12/05 04:10:57]

Given that this apparently does short-circuit, I think the comment in the Views
impl is actually somewhat clearer here, despite not having an example (we don't
need one).

[Mathieu, 2012/12/05 04:28:35]

Done.

+  model()->SetCaretVisibility(true);

[Peter Kasting, 2012/12/05 04:10:57]

The Views impl calls this before setting focus.  I don't care which order you
guys use, but be consistent.  It's much easier to maintain things later if each
platform is as close to the other platforms' implementations as possible in the
small details like this.

[Mathieu, 2012/12/05 04:28:35]

This is definitely the right order for Windows, since we can't act on a caret if
we don't have focus. ChromeOS doesn't differentiate. I will leave this part as
is.

[Peter Kasting, 2012/12/05 04:31:17]

K.  See if you can get the other CL to change to match, then.

I wonder if we should have a comment here noting what you just noted...

 }
 
-void OmniboxViewWin::ApplyFocusVisibility() {
-  // TODO(mathp): implement for Windows.
-  NOTIMPLEMENTED();
+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) {
   if (drop_highlight_position_ != position) {
     RepaintDropHighlight(drop_highlight_position_);
     drop_highlight_position_ = position;
     RepaintDropHighlight(drop_highlight_position_);
   }
 }
 
@@ skipping 956 matching lines @@
   // First, give other handlers a chance to handle the message to see if we are
   // actually going to activate and gain focus.
   LRESULT result = DefWindowProc(WM_MOUSEACTIVATE,
                                  reinterpret_cast<WPARAM>(window),
                                  MAKELPARAM(hit_test, mouse_message));
   // Check if we're getting focus from a click.  We have to do this here rather
   // than in OnXButtonDown() since in many scenarios OnSetFocus() will be
   // reached before OnXButtonDown(), preventing us from detecting this properly
   // there.  Also in those cases, we need to already know in OnSetFocus() that
   // we should not restore the saved selection.
-  if (!model()->has_focus() &&
-      ((mouse_message == WM_LBUTTONDOWN || mouse_message == WM_RBUTTONDOWN)) &&
+  if (((mouse_message == WM_LBUTTONDOWN || mouse_message == WM_RBUTTONDOWN)) &&
       (result == MA_ACTIVATE)) {
-    if (gaining_focus_) {
-      // On Windows 8 in metro mode, we get two WM_MOUSEACTIVATE messages when
-      // we click on the omnibox with the mouse.
-      DCHECK(base::win::IsMetroProcess());
-      return result;
+    if (model()->has_focus()) {

[Peter Kasting, 2012/12/05 04:10:57]

I don't understand why you did things this way instead of the way I suggested. 
I was very intentional about wanting you to trigger the gaining_focus_
manipulation because that in turn is what triggers the click-to-select-all etc.
behavior.  I don't see advantage in going this route, it's just monotonically
less correct.  Even if you don't have the ability to test the with-text case
it's much better to write code that's likely correct instead of code that's
certainly not correct.  But frankly, no, I don't want you to worry about those
cases later, I'd like things to be correct from the start.

I also don't understand your "the proposed if clause would not have done the
trick" comment.  The proposed if clause explicitly checked for invisible focus. 
You need to explain in more detail if something about it wasn't right.

[Mathieu, 2012/12/05 04:28:35]

Discussed offline. I had misunderstood the original proposal.

+      if (!model()->is_caret_visible()) {
+        // Explicitely setting visibility of the focus as a result of directly
+        // clicking on the omnibox while it has invisible focus.
+        SetCaretVisibility(true);
+      }
+    } else {
+      if (gaining_focus_) {
+        // On Windows 8 in metro mode, we get two WM_MOUSEACTIVATE messages when
+        // we click on the omnibox with the mouse.
+        DCHECK(base::win::IsMetroProcess());
+        return result;
+      }
+      gaining_focus_.reset(new ScopedFreeze(this, GetTextObjectModel()));
+      // NOTE: Despite |mouse_message| being WM_XBUTTONDOWN here, we're not
+      // guaranteed to call OnXButtonDown() later!  Specifically, if this is the
+      // second click of a double click, we'll reach here but later call
+      // OnXButtonDblClk().  Make sure |gaining_focus_| gets reset both places,
+      // or we'll have visual glitchiness and then DCHECK failures.
+
+      // Don't restore saved selection, it will just screw up our interaction
+      // with this edit.
+      saved_selection_for_focus_change_.cpMin = -1;
     }
-    gaining_focus_.reset(new ScopedFreeze(this, GetTextObjectModel()));
-    // NOTE: Despite |mouse_message| being WM_XBUTTONDOWN here, we're not
-    // guaranteed to call OnXButtonDown() later!  Specifically, if this is the
-    // second click of a double click, we'll reach here but later call
-    // OnXButtonDblClk().  Make sure |gaining_focus_| gets reset both places,
-    // or we'll have visual glitchiness and then DCHECK failures.
-
-    // Don't restore saved selection, it will just screw up our interaction
-    // with this edit.
-    saved_selection_for_focus_change_.cpMin = -1;
   }
   return result;
 }
 
 void OmniboxViewWin::OnMouseMove(UINT keys, const CPoint& point) {
   if (possible_drag_) {
     StartDragIfNecessary(point);
     // Don't fall through to default mouse handling, otherwise a second
     // drag session may start.
     return;
@@ skipping 119 matching lines @@
 
   // Draw the drop highlight.
   if (drop_highlight_position_ != -1)
     DrawDropHighlight(memory_dc, rect, paint_clip_rect);
 
   // Blit the memory DC to the actual paint DC and clean up.
   BitBlt(paint_dc, rect.left, rect.top, rect.Width(), rect.Height(), memory_dc,
          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 certain events that trigger repaints, e.g. window resize events.
+  ApplyCaretVisibility();
 }
 
 void OmniboxViewWin::OnPaste() {
   // Replace the selection if we have something to paste.
   const string16 text(GetClipboardText());
   if (!text.empty()) {
     // Record this paste, so we can do different behavior.
     model()->on_paste();
     // Force a Paste operation to trigger the text_changed code in
     // OnAfterPossibleChange(), even if identical contents are pasted into the
@@ skipping 839 matching lines @@
   return (rect.left - client_rect.left) + (client_rect.right - rect.right);
 }
 
 int OmniboxViewWin::WidthNeededToDisplay(const string16& text) const {
   // Use font_.GetStringWidth() instead of
   // PosFromChar(location_entry_->GetTextLength()) because PosFromChar() is
   // apparently buggy. In both LTR UI and RTL UI with left-to-right layout,
   // PosFromChar(i) might return 0 when i is greater than 1.
   return font_.GetStringWidth(text) + GetHorizontalMargin();
 }