[rAC, OSX] Align bubble arrow based on location.

chrome/browser/ui/cocoa/autofill/autofill_details_container.mm

Index: chrome/browser/ui/cocoa/autofill/autofill_details_container.mm
diff --git a/chrome/browser/ui/cocoa/autofill/autofill_details_container.mm b/chrome/browser/ui/cocoa/autofill/autofill_details_container.mm
index ee4ee05773429ab1bbd2400f997e1dab892a649e..c3bda1916b76c3dfc6e34a389c708b6d3317be3e 100644
--- a/chrome/browser/ui/cocoa/autofill/autofill_details_container.mm
+++ b/chrome/browser/ui/cocoa/autofill/autofill_details_container.mm
@@ -10,6 +10,7 @@
 #include "chrome/browser/ui/autofill/autofill_dialog_view_delegate.h"
 #import "chrome/browser/ui/cocoa/autofill/autofill_bubble_controller.h"
 #import "chrome/browser/ui/cocoa/autofill/autofill_section_container.h"
+#import "chrome/browser/ui/cocoa/info_bubble_view.h"
 
 @implementation AutofillDetailsContainer
 
@@ -157,14 +158,6 @@
   }
 }
 
-- (NSPoint)anchorPointFromView:(NSView*)view {
-  // All math done in window coordinates, since views might be flipped.
-  NSRect viewRect = [view convertRect:[view bounds] toView:nil];
-  NSPoint anchorPoint =
-      NSMakePoint(NSMidX(viewRect), NSMinY(viewRect));
-  return [[view window] convertBaseToScreen:anchorPoint];
-}
-
 - (void)errorBubbleWindowWillClose:(NSNotification*)notification {
   DCHECK_EQ([notification object], [errorBubbleController_ window]);
 
@@ -195,7 +188,24 @@
 
   // Compute anchor point (in window coords - views might be flipped).
   NSRect viewRect = [field convertRect:[field bounds] toView:nil];
-  NSPoint anchorPoint = NSMakePoint(NSMidX(viewRect), NSMinY(viewRect));
+
+  // If a bubble at maximum size with a left-aligned edge would exceed the
+  // window width, align the right edge of bubble and view. In all other
+  // cases, left edge of bubble and view align.

[Ilya Sherman, 2013/11/22 04:32:10]

nit: Suggested rephrasing: "In all other cases, align the left edge of the
bubble and the view." (To better match the previous sentence.)  Feel free to
tweak further, of course :)

[groby-ooo-7-16, 2013/11/22 04:50:15]

Done.

+  NSPoint anchorPoint;
+  if ((NSMinX(viewRect) + [errorBubbleController_ maxWidth]) >

[Ilya Sherman, 2013/11/22 04:32:10]

Hmm, why maxWidth rather than actual width?

[groby-ooo-7-16, 2013/11/22 04:50:15]

If I don't do that, the arrow/edge alignment might change based on the message
text. 

If you look at the CVC validation screenshots on the bug, "Field is required"
would otherwise have the arrow on the left. As soon as you type the first digit,
the arrow jumps to the right since the message changed. Comparing to maxWidth
ensures consistency instead.

[Ilya Sherman, 2013/11/22 04:58:09]

I see, that makes sense.  Mind recording that as comments for the code? 
Otherwise, some "helpful" fella might come along and break this later on.

[groby-ooo-7-16, 2013/11/22 19:12:13]

Done.

+      NSWidth([parentWindow frame])) {

[Ilya Sherman, 2013/11/22 04:32:10]

Hmm, you're comparing a coordinate to a width.  Is that intentional?  It seems a
little fragile to me.

[groby-ooo-7-16, 2013/11/22 04:50:15]

That is indeed intentional. The viewRect is in the window's coordinate system.
The window frame is in the screen coordinate system. If windows had bounds, it
would be NSMaxX([parentWindow bounds]). Since for all NSWindows, the bounds are
assumed to be (0,0) - (frameWidth,frameHeight) using NSWidth here is safe.

I could upconvert both into screen coordinates but I'm not sure that'd make
things actually more readable:

NSPoint rightBottomOfView = viewRect.origin;
rightBottomOfView.x += [errorBubbleController_ maxWidth];
rightBottomOfView = [parentWindow convertBaseToScreen:rightBottomOfView];
NSPoint rightBottomOfWindow = [parentWindow frame].origin;
rightBottomOfWindow.x += NSWidth([parentWindow frame]);
if (rightBottomOfView.x > rightBottomOfWindow.x)....

Alternative 2: (It's slightly nicer)
NSPoint rightBottomOfView = NSMakePoint(NSMaxX(viewRect), NSMinY(viewRect));
rightBottomOfView = [parentWindow convertBaseToScreen:rightBottomOfView];
if (rightBottomOfView.x >= NSMaxX([parentWindow frame])...

[Ilya Sherman, 2013/11/22 04:58:09]

Ah, I missed that the viewRect was guaranteed to be in window coordinates
already.  The comparison seems fine as-is, in that case.  Thanks for the
detailed explanation! :)

[groby-ooo-7-16, 2013/11/22 19:12:13]

Done.

+    anchorPoint = NSMakePoint(NSMaxX(viewRect), NSMinY(viewRect));
+    [[errorBubbleController_ bubble] setArrowLocation:info_bubble::kTopRight];
+    [[errorBubbleController_ bubble] setAlignment:
+        info_bubble::kAlignRightEdgeToAnchorEdge];
+
+  } else {
+    anchorPoint = NSMakePoint(NSMinX(viewRect), NSMinY(viewRect));
+    [[errorBubbleController_ bubble] setArrowLocation:info_bubble::kTopLeft];
+    [[errorBubbleController_ bubble] setAlignment:
+        info_bubble::kAlignLeftEdgeToAnchorEdge];
+  }
   [errorBubbleController_ setAnchorPoint:
       [parentWindow convertBaseToScreen:anchorPoint]];