It looks like what's happening here is that when you focus the fakebox, the omnibox is losing focus and shows the placeholder text "Search Google or type URL". But then the omnibox regains focus and immediately hides the placeholder text.

What should happen I believe, is that when you focus the fakebox, the omnibox should lose focus and *stay* unfocused until the first keystroke.

kmilka: Adding you because it looks like you've done some recent work on the fakebox. Is there anything that might have changed recently to alter the interactions between the fakebox and the omnibox?

I think though I'm definitely not sure that this used to work differently until recently. For example, the fakebox was disappearing on input but now it remains visible.

Interesting, none of the recent NTP changes have touched the way the fakebox handles focus (through searchboxApiHandle.startCapturingKeyStrokes(), searchboxApiHandle.stopCapturingKeyStrokes(), searchboxApiHandle.onkeycapturechange, etc). Has the Omnibox recently changed how it interfaces with the API?


The only thing that should be different is the Google logo is no longer hidden when text is entered.

krb: Can you take a look at this? I'm not sure what's going on but we can discuss offline tomorrow morning.

The "flicker" first shows up in 68.0.3434.0-ish, I'm guessing from 1050185, which adds some logic to OnOmniboxFocused().

However, the flickering text doesn't show up until 69.0.3448.0-ish, perhaps from 1082794.

The gist is, I think it has always cycled focus, but has recently gotten 2 gradually more visible changes when focused.

btw, I don't know how far back it goes (i.e. earlier than this) but the fakebox flickers as well.

treib: can you advise on this NTP issue or point us at someone who can?

I think the fundamental problem here is that when the fakebox is clicked on, it's setting focus in the omnibox. In the past we didn't care because there was no visible effect of this focus when the omnibox is empty. But now (in GM2 Refresh) there's a focus ring and hint text in the omnibox. Setting focus enables the focus ring and hides the hint text.

But we'd like neither of those things to happen until there's actual input in the fakebox (when the fakebox is hidden). Can this be done easily?

I'm not too familiar with this, but focus is sent to Omnibox, and keystrokes are captured here: https://cs.chromium.org/chromium/src/chrome/renderer/searchbox/searchbox.h?l=111. Perhaps modifying the order of these would help avoid the flicker? Likely change in https://cs.chromium.org/chromium/src/out/Debug/gen/chrome/common/search.mojom.cc?l=240.

Also note that this should be tested after crrev.com/c/1127488 lands.

When the fakebox is clicked, it sets a special OMNIBOX_FOCUS_INVISIBLE state on the omnibox. I think that state needs to be treated specially - the omnibox should render as if it didn't have focus, because we're pretending that the fakebox on the NTP has focus.

krb: can you take a look at what would be required to skip showing the focus ring and hint text in the OMNIBOX_FOCUS_INVISIBLE case? I know you don't have a lot of time this week, so if the fix isn't simple I can help out.

We already handle this case. The issue is that we're being sent FOCUS_NONE, FOCUS_VISIBLE (twice! but we ignore the second) and finally FOCUS_INVISIBLE (which we honor).

I tried teasing out why we send all of them, after ramyan's comment hinting that there were 2 messages. I haven't made much progress on that path yet.

+Yana FYI.

My tracing showed a request of focus "invisible" getting translated into a request for "visible". This location appears to be the issue: OmniboxView::SetFocus doesn't accept a parameter of "invisible". It looks like SetFocus() and SetCaret...() need to be made atomic (and maybe SelectAll() too) so that we don't see the intermediate state.

void SearchTabHelper::FocusOmnibox(OmniboxFocusState state) {
  OmniboxView* omnibox_view = GetOmniboxView();
  if (!omnibox_view)
    return;

  switch (state) {
    case OMNIBOX_FOCUS_INVISIBLE:
**    omnibox_view->SetFocus();
      omnibox_view->model()->SetCaretVisibility(false);
      // If the user clicked on the fakebox, any text already in the omnibox
      // should get cleared when they start typing. Selecting all the existing
      // text is a convenient way to accomplish this. It also gives a slight
      // visual cue to users who really understand selection state about what
      // will happen if they start typing.
      omnibox_view->SelectAll(false);
      omnibox_view->ShowVirtualKeyboardIfEnabled();
      break;

Hey Kevin - I think your diagnosis in c#19 makes a lot of sense.

This was the CL that added the placeholder-text-hidden-on-focus logic:
https://chromium-review.googlesource.com/c/chromium/src/+/1083615/6/ui/views/controls/textfield/textfield.cc

Just spitballing - maybe we should expand the logic to account for caret visibility -- or to temporarily blank out the placeholder text while we do the above lines.

tommycli: can you take a swing at that tomorrow?

Hi Tommy, Just thinking out loud here: OmniboxViewViews::SetFocus() calls RequestFocus(), followed by SetCaretVisibility(true) (but you could just as easily call SetCaretVisibility(false) using a parameter.) The trouble is, I suspect the damage is done once RequestFocus() is called. It goes through a long stack trace and eventual RPC. From my traces, I believe this enables visibility, and is the "flicker".

However, if you could make the caret invisible *before* the RequestFocus(), then that logic might be able to check the caret state and not show the text either.

I played with some approaches in the afternoon, and the idea of making downstream recognize "invisible" focus didn't pan out. It's messier logic than typical (calling from OmniboxView, out to the window system, and back, not to mention the RPCs) so going through the usual logic but just "not showing anything" might be simplest.

Clicking the fakebox repeatedly, you can notice that the placeholder text flickers there too, which is a pre-existing condition.

I suspect the invisible-focus / fake-focus mechanism is just kind of untenable since it violates many programmer assumptions. In other words, it's a unique state that someone unfamiliar with the Omnibox would not think to accommodate.

In the long term I think the best thing to do would be to delay focusing the Omnibox until after the first keystroke and transfer that keystroke up there -- and remove the concept of the invisible focus. Or just make the fakebox a realbox. :)

In the short term I'll improve the appearance today without causing too much disruption.

FWIW, we're planning to experiment in M70 with removing the fakebox entirely.

kmilka: Excellent! Glad you guys are one step ahead of me.

In that case whatever hacks I put in today, I'll put a TODO to remove once the fakebox is refactored / gone. It will be good to get rid of the 'invisible focus' complexity.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/26c6253c48d5cccca00276cd42c812500130fe20

commit 26c6253c48d5cccca00276cd42c812500130fe20
Author: Tommy C. Li <tommycli@chromium.org>
Date: Fri Jul 20 01:47:03 2018

Omnibox UI Refresh: Make "invisible focus" apperance more consistent

On the New Tab Page, when the user clicks the fake Google search box,
i.e. the "fakebox", we give the Omnibox "invisible focus".

This is a unique state where we set the Omnibox is focused and
capturing key events, but the caret is invisible.

In the past, the caret being invisible was sufficient for the Omnibox
to be "invisibly" focused.

In MD Refresh, however, we have three new focus indicators:
 1. Focus ring
 2. Placeholder text
 3. Background color change

This CL updates all three of the above visual indicators for the
Omnibox to be keyed on the invisible-focus state rather than the
ordinary focus state.

This solves the flicker present in the below bug and actually follows
the concept of invisible-focus correctly.

The concept of invisible-focus is kind of evil, and long term, we would
like to get rid of it -- but this is a short term fix for MD Refresh.

Bug:  859826 
Change-Id: I2e0ede93e69485745bbf119a3f366b0dbfa197ab
Reviewed-on: https://chromium-review.googlesource.com/1144211
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576752}
[modify] https://crrev.com/26c6253c48d5cccca00276cd42c812500130fe20/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/26c6253c48d5cccca00276cd42c812500130fe20/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
[modify] https://crrev.com/26c6253c48d5cccca00276cd42c812500130fe20/chrome/browser/ui/views/omnibox/omnibox_view_views.h
[modify] https://crrev.com/26c6253c48d5cccca00276cd42c812500130fe20/components/omnibox/browser/omnibox_edit_model.h
[modify] https://crrev.com/26c6253c48d5cccca00276cd42c812500130fe20/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/26c6253c48d5cccca00276cd42c812500130fe20/ui/views/controls/textfield/textfield.h

Able to reproduce this issue on build without fix, hence verifying the fix on latest canary 69.0.3497.0 using Windows 10 and Debian.

Observations:
=============
1. On hitting inside fake box -- no flickering is seen in omnibox
2. On hitting continuously in fake box -- flickering of ghost text is seen in fake box

Flickering of ghost text in omnibox is fixed but flickering of ghost text in fake box is still seen.

As flickering is still seen adding Needs-Feedback label.

@tommycli: Could you please check the screencast and let us know if this needs to be fixed.

Deleted: 859826_M9.mp4 597 KB

	859826_M9.mp4 597 KB View Download

Flickering within the fakebox itself will continue to exist.

Flickering within the Omnibox should be fixed.

That seems consistent with what you are seeing, correct?

I was able to verify the fix in Canary, so requesting Merge to 69.

Your change meets the bar and is auto-approved for M69. Please go ahead and merge the CL to branch 3497 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Able to reproduce this issue on build without fix, hence verifying the fix on latest canary 70.0.3500.0 using Windows 10 and Debian.

Now no flickering of ghost text is seen in Omnibox

As fix is working as expected adding Verified labels.

Thanks!

Please merge your change to M69 branch 3497 by 4:00 PM PT today, so we can pick it up for this week M69 Dev release. Thank you.

Looks like the CL just made it into M69, so no merge required.