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

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

Ensure that the error bubble never goes outside the dialog, and that
one of the bubble edges is aligned with the corresponding edge of the
associated field. (Screenshots on bug)

BUG=305378
R=isherman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236831

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

https://codereview.chromium.org/82593002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_details_container.mm (right):

https://codereview.chromium.org/82593002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:194: // cases,
left edge of bubble and view align.
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 :)

https://codereview.chromium.org/82593002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:196: if
((NSMinX(viewRect) + [errorBubbleController_ maxWidth]) >
Hmm, why maxWidth rather than actual width?

https://codereview.chromium.org/82593002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:197:
NSWidth([parentWindow frame])) {
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:14]

Let me know if you still want me to change the comparison. It seems obvious to
me, but I might have spent too much of my life thinking about coordinate systems
;)

https://codereview.chromium.org/82593002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_details_container.mm (right):

https://codereview.chromium.org/82593002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:194: // cases,
left edge of bubble and view align.
On 2013/11/22 04:32:10, Ilya Sherman wrote:
> 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 :)

Done.

https://codereview.chromium.org/82593002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:196: if
((NSMinX(viewRect) + [errorBubbleController_ maxWidth]) >
On 2013/11/22 04:32:10, Ilya Sherman wrote:
> Hmm, why maxWidth rather than actual width?

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.

https://codereview.chromium.org/82593002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:197:
NSWidth([parentWindow frame])) {
On 2013/11/22 04:32:10, Ilya Sherman wrote:
> Hmm, you're comparing a coordinate to a width.  Is that intentional?  It seems
a
> little fragile to me.
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]

LGTM, thanks.

https://codereview.chromium.org/82593002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_details_container.mm (right):

https://codereview.chromium.org/82593002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:196: if
((NSMinX(viewRect) + [errorBubbleController_ maxWidth]) >
On 2013/11/22 04:50:15, groby wrote:
> On 2013/11/22 04:32:10, Ilya Sherman wrote:
> > Hmm, why maxWidth rather than actual width?
> 
> 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.

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.

https://codereview.chromium.org/82593002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:197:
NSWidth([parentWindow frame])) {
On 2013/11/22 04:50:15, groby wrote:
> On 2013/11/22 04:32:10, Ilya Sherman wrote:
> > Hmm, you're comparing a coordinate to a width.  Is that intentional?  It
seems
> a
> > little fragile to me.
> 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])...

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]

https://codereview.chromium.org/82593002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_details_container.mm (right):

https://codereview.chromium.org/82593002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:196: if
((NSMinX(viewRect) + [errorBubbleController_ maxWidth]) >
On 2013/11/22 04:58:09, Ilya Sherman wrote:
> On 2013/11/22 04:50:15, groby wrote:
> > On 2013/11/22 04:32:10, Ilya Sherman wrote:
> > > Hmm, why maxWidth rather than actual width?
> > 
> > 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.
> 
> 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.

Done.

https://codereview.chromium.org/82593002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:197:
NSWidth([parentWindow frame])) {
On 2013/11/22 04:58:09, Ilya Sherman wrote:
> On 2013/11/22 04:50:15, groby wrote:
> > On 2013/11/22 04:32:10, Ilya Sherman wrote:
> > > Hmm, you're comparing a coordinate to a width.  Is that intentional?  It
> seems
> > a
> > > little fragile to me.
> > 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])...
> 
> 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! :)

Done.

[commit-bot: I haz the power, 2013-11-22 19:14:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/82593002/160001

[commit-bot: I haz the power, 2013-11-22 21:27:02]

Change committed as 236831