cocoa: allow omnibox decorations to become key

chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm

Index: chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
diff --git a/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm b/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
index 4b59a6a66a91374d3070697eb15adbd6afd3e568..bed1cbcebb9aae9317d3e2cb44ebd7357952e356 100644
--- a/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
+++ b/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
@@ -742,11 +742,29 @@ new ManagePasswordsDecoration(command_updater, this)),
     LocationBarDecoration* decoration) {
   if (!decoration->IsVisible())
     return;
-  NSRect r =
+  NSRect apparent_frame =
       [[field_ cell] frameForDecoration:decoration inFrame:[field_ frame]];
-  [decoration->GetAccessibilityView() setFrame:r];
-  [decoration->GetAccessibilityView() setNeedsDisplayInRect:r];
-  decoration->UpdateAccessibilityView();
+
+  // This is a bit subtle:
+  // The decorations' accessibility views can become key to allow keyboard
+  // access to the location bar decorations, but Cocoa's automatic key view loop
+  // sorts by top-left coordinate. Since the omnibox's top-left coordinate is

[lgrey, 2017/04/25 21:11:39]

Do you know if the top-left thing holds in RTL?

If the horizontal order of the loop is flipped in RTL could we do this same move
but move the "real frames" up instead of left? That way, it'd work the same for
both.

If they're *not* flipped I really have no idea what we could do short of
flipping the frames to their LTR equivalents. I'd have to think there's some
allowance, otherwise we'd have to do this for absolutely everything.

[Elly Fong-Jones, 2017/04/26 16:50:01]

It seems to proceed from the top right instead.
 
Moving them up doesn't seem to work in the LTR case - it looks like Cocoa wants
to sort views by x-then-y. I think an RTL special case might be in order here :\
Investigation reveals that the accessibility views are themselves mis-positioned
in RTL layouts. I'm not quite sure why - maybe a deficiency in
AutocompleteTextFieldCell? Per our offline discussion I changed the owner of
this TODO to you and filed https://crbug.com/715627 for this.

+  // before its leading decorations, the omnibox would sort before its own
+  // leading decorations, which was logical but visually unintuitive. Therefore,
+  // for leading decorations, this method moves their frame to be "just before"
+  // the omnibox in automatic key view loop order, and gives them an apparent
+  // frame (see DecorationAccessibilityView) so that they still paint their
+  // focus rings at the right place.
+  //
+  // TODO(ellyjones): what happens in RTL layouts?
+  NSRect real_frame = apparent_frame;
+  int left_index = [[field_ cell] leadingDecorationIndex:decoration];
+  if (left_index != -1)
+    real_frame.origin.x = [field_ frame].origin.x - left_index - 1;

[Mark Mentovai, 2017/04/25 20:44:31]

Can we have so many decorations that something’s x-coordinate might be on the
wrong side of some other nearby view?

[Elly Fong-Jones, 2017/04/26 16:50:01]

Added a DCHECK.

+
+  decoration->UpdateAccessibilityView(apparent_frame);
+  [decoration->GetAccessibilityView() setFrame:real_frame];
+  [decoration->GetAccessibilityView() setNeedsDisplayInRect:apparent_frame];
 }
 
 std::vector<LocationBarDecoration*> LocationBarViewMac::GetDecorations() {