[Web Payments] Implement proper focusing in Payment Request

[Web Payments] Implement proper focusing in Payment Request

BUG=717322
Review-Url: https://codereview.chromium.org/2853163002
Cr-Commit-Position: refs/heads/master@{#468630}
Committed: https://chromium.googlesource.com/chromium/src/+/818ada352c5ec2c3b9da4baf2c9da3472f016fcc

[anthonyvd, 2017-05-02 00:24:46]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 00:25:42]

Dry run: 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-05-02 01:12:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 01:12:22]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-05-02 11:48:25]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 11:48:39]

Dry run: 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-05-02 12:33:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 12:33:14]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-05-02 13:00:21]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 13:00:40]

Dry run: CQ is trying da patch.

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

[anthonyvd, 2017-05-02 13:00:49]

anthonyvd@chromium.org changed reviewers:
+ mathp@chromium.org

[anthonyvd, 2017-05-02 13:00:50]

Hi Math, PTAL!

[Mathieu, 2017-05-02 13:16:10]

Prettty clean! For other screens that are not touched here, does the focus
happen in the order that the views are added to the payment sheet (e.g.
editors?). 

lgtm

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_row_view.cc (right):

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_row_view.cc:36:
set_background(views::Background::CreateSolidBackground(SK_ColorLTGRAY));
We should get the proper color from the theme to use here and above, since it's
good for accessibility (e.g. high contrast mode)

https://cs.chromium.org/chromium/src/ui/native_theme/native_theme.h?rcl=eb669...

I'm thinking HoveredBackground ?

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.cc
(right):

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:25: class
SheetView : public views::View, public views::FocusTraversable {
If you die, we have no idea what this does (please don't die!). Let's add a
class comment to describe why this is needed at all, capturing all your research
into this.

I just re-read the code a couple of times, and the comment from
SetFirstFocusableView could be brought up to the class level because this is the
whole point of the class.

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.h
(right):

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:130: // Hold
on to the ScrollView because it must be explicitly laid out in some
update/split comment to reflect more variables

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right):

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:444:
std::unique_ptr<views::View> summary_row = CreatePaymentSheetSummaryRow();
need this change?

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/view_stack.cc (right):

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/view_stack.cc:52: RequestFocus();
I'm curious, does this still work even though the animation is not done?

[anthonyvd, 2017-05-02 13:43:32]

Comments addressed.

Yep, the focus advances in the order views were added.

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_row_view.cc (right):

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_row_view.cc:36:
set_background(views::Background::CreateSolidBackground(SK_ColorLTGRAY));
On 2017/05/02 at 13:16:10, Mathieu wrote:
> We should get the proper color from the theme to use here and above, since
it's good for accessibility (e.g. high contrast mode)
> 
>
https://cs.chromium.org/chromium/src/ui/native_theme/native_theme.h?rcl=eb669...
> 
> I'm thinking HoveredBackground ?

Oh good call! Done.

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.cc
(right):

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:25: class
SheetView : public views::View, public views::FocusTraversable {
On 2017/05/02 at 13:16:10, Mathieu wrote:
> If you die, we have no idea what this does (please don't die!). Let's add a
class comment to describe why this is needed at all, capturing all your research
into this.
> 
> I just re-read the code a couple of times, and the comment from
SetFirstFocusableView could be brought up to the class level because this is the
whole point of the class.

Done.

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.h
(right):

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:130: // Hold
on to the ScrollView because it must be explicitly laid out in some
On 2017/05/02 at 13:16:10, Mathieu wrote:
> update/split comment to reflect more variables

Done.

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right):

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:444:
std::unique_ptr<views::View> summary_row = CreatePaymentSheetSummaryRow();
On 2017/05/02 at 13:16:10, Mathieu wrote:
> need this change?

Oops, not anymore.

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/view_stack.cc (right):

https://codereview.chromium.org/2853163002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/view_stack.cc:52: RequestFocus();
On 2017/05/02 at 13:16:10, Mathieu wrote:
> I'm curious, does this still work even though the animation is not done?

Yeah, the other view gets focused right away but doesn't process events until
the animation is done because of the CanProcessEventsWithinSubtree() override.

[anthonyvd, 2017-05-02 13:43:40]

The CQ bit was unchecked by anthonyvd@chromium.org

[anthonyvd, 2017-05-02 13:43:46]

The CQ bit was checked by anthonyvd@chromium.org

[commit-bot: I haz the power, 2017-05-02 13:43:58]

CQ is trying da patch.

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

[anthonyvd, 2017-05-02 13:45:37]

The CQ bit was unchecked by anthonyvd@chromium.org

[anthonyvd, 2017-05-02 13:47:22]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-05-02 13:47:22]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2853163002/#ps60001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-05-02 13:47:27]

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-05-02 14:33:59]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493732842243750,
"parent_rev": "0bfbbd44d9d10e86fff9ed75141ad09d65ec42ed", "commit_rev":
"818ada352c5ec2c3b9da4baf2c9da3472f016fcc"}

[commit-bot: I haz the power, 2017-05-02 14:34:10]

Description was changed from

==========
[Web Payments] Implement proper focusing in Payment Request

BUG=717322
==========

to

==========
[Web Payments] Implement proper focusing in Payment Request

BUG=717322

Review-Url: https://codereview.chromium.org/2853163002
Cr-Commit-Position: refs/heads/master@{#468630}
Committed:
https://chromium.googlesource.com/chromium/src/+/818ada352c5ec2c3b9da4baf2c9d...
==========

[commit-bot: I haz the power, 2017-05-02 14:34:11]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/818ada352c5ec2c3b9da4baf2c9d...

[findit-for-me, 2017-05-02 16:02:19]

Findit (https://goo.gl/kROfz5) identified this CL at revision 468630 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[stgao, 2017-05-02 17:31:05]

On 2017/05/02 16:02:19, findit-for-me wrote:
> Findit (https://goo.gl/kROfz5) identified this CL at revision 468630 as the
> culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

The failed build is
https://luci-milo.appspot.com/buildbot/chromium.chromiumos/ChromiumOS%20x86-g...

[anthonyvd, 2017-05-02 17:37:29]

On 2017/05/02 at 17:31:05, stgao wrote:
> On 2017/05/02 16:02:19, findit-for-me wrote:
> > Findit (https://goo.gl/kROfz5) identified this CL at revision 468630 as the
> > culprit for
> > failures in the build cycles as shown on:
> >
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
> 
> The failed build is
https://luci-milo.appspot.com/buildbot/chromium.chromiumos/ChromiumOS%20x86-g...

Filed a bug (https://bugs.chromium.org/p/chromium/issues/detail?id=717612) and
got a fix on the way.