bsep: I think this is a regression from r504209 . There, Textfield::CalculatePreferredSize() stopped accounting for insets.height().

But I'm not really sure why other textfields aren't affected. There's nothing too special about the CVC textfield:

views::Textfield* CreateCvcTextfield() {
  views::Textfield* textfield = new views::Textfield();
  textfield->set_placeholder_text(
      l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_PLACEHOLDER_CVC));
  textfield->set_default_width_in_chars(8);
  textfield->SetTextInputType(ui::TextInputType::TEXT_INPUT_TYPE_NUMBER);
  return textfield;
}

Ah, actually I think Textfield only has insets when it gets a proper border. FocusRing::Install(..) doesn't add one. Maybe it should be adding an empty border? How is this not broken everywhere.... #baffled

Hmm yeah that's pretty odd. I don't think it's that patch though; if I recall correctly the reason I removed insets.height() was because they were being applied twice somehow. Also in that case I'd expect the bug to show up pre-Harmony too.

I wonder if it's being clipped by the bounds of its parent view? I think Allen was fixing a similar problem in https://chromium-review.googlesource.com/c/chromium/src/+/546697

Is there a browsertest InvokeDialog_ test that we can use to bring this dialog up to repro?

If not, here's an example of how you make one: https://chromium-review.googlesource.com/c/chromium/src/+/731617/9/chrome/browser/download/download_danger_prompt_browsertest.cc

Example invocation:

out\Default\browser_tests.exe --verbose --gtest_filter="BrowserDialogTest.Invoke" --interactive --dialog=ManagePasswordsBubbleDialogViewTest.DISABLED_InvokeDialog_ManagePasswordBubble

I think we have such a test but not for this flow, I'll add one. 

In the meantime, patch this CL and trigger autofill on https://rsolomakhin.github.io/autofill (huge hack):  https://chromium-review.googlesource.com/c/chromium/src/+/741685

Yeah there's one for the card unmask modal prompt. But it doesn't have the focus ring issue

browser_tests --gtest_filter=BrowserDialogTest.Invoke --dialog=CardUnmaskPromptViewBrowserTest.InvokeDialog_expired --interactive --enable-features=SecondaryUiMd --draw-view-bounds-rects

It's using a box layout

  input_row_->SetLayoutManager(
      new views::BoxLayout(views::BoxLayout::kHorizontal, gfx::Insets(), 5));


It does look as though it's drawing outside its bounds though.

I haven't been able to get this dialog to appear.

Deleted: Screen Shot 2017-11-03 at 11.04.17 am.png 29.0 KB

	Screen Shot 2017-11-03 at 11.04.17 am.png 29.0 KB View Download

Deleted: Screen Shot 2017-11-03 at 11.07.43 am.png 17.3 KB

	Screen Shot 2017-11-03 at 11.07.43 am.png 17.3 KB View Download

It does seem like you were able to bring up the Card Unmask dialog, which indeed doesn't have the bug.

The dialog that has the bug is the "save_card" bubble. My draft CL posted in #c5 should allow you to make it appear without effort (simply triggering autofill).

I had however not previously specified the flags:
--secondary-ui-md
--enable-autofill-credit-card-upload-cvc-prompt

Hm - it's probably code in ViewStack::ViewStack that's causing the clipping:

  // Paint to a layer and Mask to Bounds, otherwise descendant views that paint
  // to a layer themselves will still paint while they're being animated out and
  // are out of bounds of their parent.
  SetPaintToLayer();
  layer()->SetMasksToBounds(true);



We need to think about the right way to solve this without throwing off the Harmony layout. Simply insetting the ViewStack or the TextField in its layout is going to move it out of alignment.

What might work instead, would be to instead of clipping to ViewStack bounds, clip to the parent of ViewStack. (except it doesn't have a Layer. The window does, but clipping to that will probably make the views overlap the window shadow when the slide out. So instead, you could try adding a layer to the DialogClientView -- I think that will work..)

+Anthony creator of ViewStack. 

Thanks. I sadly don't know about layers but will educate myself.

Thanks Trent, that was a good hint. I've made a change so that the ViewStack clips at the bubble's layer now, just for this dialog: https://chromium-review.googlesource.com/c/chromium/src/+/753790

It's unclear if SetMaskLayer works, but by looking at crbug.com/713359 looks like it might? I'm too lazy getting a windows machine so I'm fine checking on Canary if the change is appropriate for now.

I don't think it does work on all versions of Windows, or we'd have removed these by now[1][2]. 

[1] https://cs.chromium.org/chromium/src/ui/views/animation/ink_drop_host_view.cc?rcl=1500d1ca2245d1cede18195c33036ed963d7f9c0&l=292
[2] https://cs.chromium.org/chromium/src/ui/views/controls/native/native_view_host_aura.cc?rcl=1500d1ca2245d1cede18195c33036ed963d7f9c0&l=145

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ef522e8e25813f30e28f0a5d72cebc16372555ca

commit ef522e8e25813f30e28f0a5d72cebc16372555ca
Author: Mathieu Perreault <mathp@chromium.org>
Date: Mon Nov 06 17:16:34 2017

[Autofill] Add browsertest cases for credit card save dialog

Bug:  780989 
Test: out/Default/browser_tests --gtest_filter=BrowserDialogTest.Invoke --dialog=SaveCardBubbleControllerImplTest.InvokeDialog_Local
Change-Id: Ib883056069985a39ff6d6cf3115f3df2edb3ee28
Reviewed-on: https://chromium-review.googlesource.com/754732
Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514158}
[modify] https://crrev.com/ef522e8e25813f30e28f0a5d72cebc16372555ca/chrome/browser/ui/autofill/save_card_bubble_controller_impl_browsertest.cc

Hitting various DCHECKS when trying out the approach in #c8. The change in #c10 was to have the bubble SetPaintToLayer() and set view_stack_->layer->SetMaskLayer(layer()) but it's hitting this DCHECK[1].  In non-debug this solution fixes the issue.

Any help appreciated.

[1] https://cs.chromium.org/chromium/src/cc/layers/layer.cc?type=cs&q=layer.cc:222&sq=package:chromium&l=222

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be04c2af78b65313a1fd4c8f7e8051b52e453eeb

commit be04c2af78b65313a1fd4c8f7e8051b52e453eeb
Author: Mathieu Perreault <mathp@chromium.org>
Date: Thu Nov 09 12:13:38 2017

[Autofill] Remove ViewStack in save card dialog.

Two reasons:
* The animation was only for the middle section of the dialog which is
  not ideal and janky.
* The focus ring on the CVC field in the second step was broken.

Bug:  780989 
Test: out/Default/browser_tests --gtest_filter=BrowserDialogTest.Invoke --dialog=SaveCardBubbleControllerImplTest.InvokeDialog_Server_WithCvcStep --interactive
Change-Id: I09dec86ce250f72695823f041cb72d2d1151b851
Reviewed-on: https://chromium-review.googlesource.com/755804
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Mathieu Perreault <mathp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515145}
[modify] https://crrev.com/be04c2af78b65313a1fd4c8f7e8051b52e453eeb/chrome/browser/ui/views/autofill/save_card_bubble_views.cc
[modify] https://crrev.com/be04c2af78b65313a1fd4c8f7e8051b52e453eeb/chrome/browser/ui/views/autofill/save_card_bubble_views.h

Tested the issue using latest M64 #64.0.3264.0 on Mac 10.12.6 and linux Ubuntu 14.04 as per the steps mentioned below and couldn't observe the mentioned Dialog.

Steps Followed:
1. Launched Browser with chrome://flags#enable-autofill-credit-card-upload-cvc-prompt and chrome://flags#secondary-ui-md
2. Visited https://dump-truck.appspot.com/usecase-address_then_cc_text/
3. Clicked “Start”, “Fill with Default Values”, “Submit”, “Fill with Default Values”, “Submit”.

@mathp: Please find the attached screenshot and let us know the update.

Thanks!!

Deleted: Screen Shot 2017-11-10 at 3.09.12 PM.png 24.7 KB

	Screen Shot 2017-11-10 at 3.09.12 PM.png 24.7 KB View Download