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

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

 // Copyright (c) 2013 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.
 
 #import "chrome/browser/ui/cocoa/autofill/autofill_details_container.h"
 
 #include <algorithm>
 
 #include "base/mac/foundation_util.h"
 #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
 
 - (id)initWithDelegate:(autofill::AutofillDialogViewDelegate*)delegate {
   if (self = [super init]) {
     delegate_ = delegate;
   }
   return self;
 }
 
@@ skipping 127 matching lines @@
   [scrollView_ reflectScrolledClipView:clipView];
   [self updateErrorBubble];
 }
 
 - (void)updateErrorBubble {
   if (!delegate_->ShouldShowErrorBubble()) {
     [errorBubbleController_ close];
   }
 }
 
-- (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]);
 
   NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
   [center removeObserver:self
                     name:NSWindowWillCloseNotification
                   object:[errorBubbleController_ window]];
   errorBubbleController_ = nil;
 }
 
@@ skipping 10 matching lines @@
 
   // Handle bubble self-deleting.
   NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
   [center addObserver:self
              selector:@selector(errorBubbleWindowWillClose:)
                  name:NSWindowWillCloseNotification
                object:[errorBubbleController_ window]];
 
   // 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]];
 
   [errorBubbleController_ showWindow:self];
 }
 
 - (void)hideErrorBubble {
   [errorBubble_ setHidden:YES];
 }
 
@@ skipping 11 matching lines @@
   }
 
   if ([field invalid]) {
     [self showErrorBubbleForField:field];
   } else {
     [errorBubbleController_ close];
   }
 }
 
 @end