cocoa: allow omnibox decorations to become key

cocoa: allow omnibox decorations to become key

Originally, this behavior was disabled because leading decorations were
appearing in the key view loop after the omnibox itself, which led to
an unintuitive tab order. This change:

1) Allows omnibox decorations to become key when they are accepting mouse clicks
   again
2) Mangles the automatic key view loop slightly so that leading decorations
   appear before the omnibox in the tab order

BUG=623883
Review-Url: https://codereview.chromium.org/2839893003
Cr-Commit-Position: refs/heads/master@{#467363}
Committed: https://chromium.googlesource.com/chromium/src/+/88246ce0f7ae1b9a65c1056e9f2c09fe9e0244fe

[Elly Fong-Jones, 2017-04-25 20:37:02]

Description was changed from

==========
cocoa: allow omnibox decorations to become key

Originally, this behavior was disabled because leading decorations were
appearing in the key view loop after the omnibox itself, which led to
an unintuitive tab order. This change:

1) Allows omnibox decorations to become key when they are accepting mouse clicks
   again
2) Mangles the automatic key view loop slightly so that leading decorations
   appear before the omnibox in the tab order

BUG=623883
==========

to

==========
cocoa: allow omnibox decorations to become key

Originally, this behavior was disabled because leading decorations were
appearing in the key view loop after the omnibox itself, which led to
an unintuitive tab order. This change:

1) Allows omnibox decorations to become key when they are accepting mouse clicks
   again
2) Mangles the automatic key view loop slightly so that leading decorations
   appear before the omnibox in the tab order

BUG=623883
==========

[Elly Fong-Jones, 2017-04-25 20:37:03]

ellyjones@chromium.org changed reviewers:
+ lgrey@chromium.org, mark@chromium.org

[Elly Fong-Jones, 2017-04-25 20:37:19]

mark: ptal? :)
lgrey: what do you think of the RTL TODO I left myself? What is the right way to
handle that?

[Mark Mentovai, 2017-04-25 20:44:32]

https://codereview.chromium.org/2839893003/diff/1/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/2839893003/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:740: for
(size_t i = 0; i < leadingDecorations_.size(); ++i)
I like to use std::find and std::distance for this kind of thing, but you’re
free to ignore the suggestion. Some people like the explicit loop better, and
that’s OK too.

https://codereview.chromium.org/2839893003/diff/1/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/2839893003/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:763:
real_frame.origin.x = [field_ frame].origin.x - left_index - 1;
Can we have so many decorations that something’s x-coordinate might be on the
wrong side of some other nearby view?

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

https://codereview.chromium.org/2839893003/diff/1/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/2839893003/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:751: // sorts by
top-left coordinate. Since the omnibox's top-left coordinate is
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]

mark: ptal?

I think this CL is not going to fix RTL support - there's something more subtle
wrong with omnibox RTL, which is still behind the ExperimentalMacRTL feature
flag anyway. I filed a bug and added a TODO for lgrey.

https://codereview.chromium.org/2839893003/diff/1/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/2839893003/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:740: for
(size_t i = 0; i < leadingDecorations_.size(); ++i)
On 2017/04/25 20:44:31, Mark Mentovai wrote:
> I like to use std::find and std::distance for this kind of thing, but you’re
> free to ignore the suggestion. Some people like the explicit loop better, and
> that’s OK too.

I prefer the explicit loop, but I'm not quite sure why.

https://codereview.chromium.org/2839893003/diff/1/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/2839893003/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:751: // sorts by
top-left coordinate. Since the omnibox's top-left coordinate is
On 2017/04/25 21:11:39, lgrey wrote:
> Do you know if the top-left thing holds in RTL?

It seems to proceed from the top right instead.
 
> 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.

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 :\

> 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.

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.

https://codereview.chromium.org/2839893003/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:763:
real_frame.origin.x = [field_ frame].origin.x - left_index - 1;
On 2017/04/25 20:44:31, Mark Mentovai wrote:
> Can we have so many decorations that something’s x-coordinate might be on the
> wrong side of some other nearby view?

Added a DCHECK.

[Mark Mentovai, 2017-04-26 16:54:15]

LGTM

[Elly Fong-Jones, 2017-04-26 16:55:15]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2017-04-26 16:55:50]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 17:25:56]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493225715392690,
"parent_rev": "931a93d3bb0be1ca991c0e40bd72fc2ce642549a", "commit_rev":
"88246ce0f7ae1b9a65c1056e9f2c09fe9e0244fe"}

[commit-bot: I haz the power, 2017-04-26 17:26:07]

Description was changed from

==========
cocoa: allow omnibox decorations to become key

Originally, this behavior was disabled because leading decorations were
appearing in the key view loop after the omnibox itself, which led to
an unintuitive tab order. This change:

1) Allows omnibox decorations to become key when they are accepting mouse clicks
   again
2) Mangles the automatic key view loop slightly so that leading decorations
   appear before the omnibox in the tab order

BUG=623883
==========

to

==========
cocoa: allow omnibox decorations to become key

Originally, this behavior was disabled because leading decorations were
appearing in the key view loop after the omnibox itself, which led to
an unintuitive tab order. This change:

1) Allows omnibox decorations to become key when they are accepting mouse clicks
   again
2) Mangles the automatic key view loop slightly so that leading decorations
   appear before the omnibox in the tab order

BUG=623883

Review-Url: https://codereview.chromium.org/2839893003
Cr-Commit-Position: refs/heads/master@{#467363}
Committed:
https://chromium.googlesource.com/chromium/src/+/88246ce0f7ae1b9a65c1056e9f2c...
==========

[commit-bot: I haz the power, 2017-04-26 17:26:08]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/88246ce0f7ae1b9a65c1056e9f2c...