[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);
+  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) {
   if (drop_highlight_position_ != position) {
     RepaintDropHighlight(drop_highlight_position_);
     drop_highlight_position_ = position;
     RepaintDropHighlight(drop_highlight_position_);
   }
 }
 
@@ skipping 64 matching lines @@
   TextChanged();
   return true;
 }
 
 void OmniboxViewWin::OnRevertTemporaryText() {
   SetSelectionRange(original_selection_);
   TextChanged();
 }
 
 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_);
 }
 
 bool OmniboxViewWin::OnAfterPossibleChange() {
   return OnAfterPossibleChangeInternal(false);
 }
 
 bool OmniboxViewWin::OnAfterPossibleChangeInternal(bool force_text_changed) {
@@ skipping 1030 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;
+
+  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() {
   // 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();
 }