Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2592833002:
[WebPayments] Start populating the Payment Sheet. (Closed)
Description[WebPayments] Start populating the Payment Sheet.
This change adds the header and the Order Summary section to the
Payment Sheet in the Payment Request dialog. Both sheets still need
many tweaks and extra features but this is the first step towards the
dialog actually looking like the design specs.
Mocks here:
https://folio.googleplex.com/chrome-ux/mocks/329-future-web-pay/latest/desktop#
A screenshot of the Payment Sheet is here:
https://drive.google.com/file/d/0B-DVbqI3huZGREpIaE5yMDZjZzA/view?usp=sharing
A screenshot of the Order Summary shown when clicking the Order Summary
row is here:
https://drive.google.com/file/d/0B-DVbqI3huZGM0F0cC1sS2xLTWs/view?usp=sharing
BUG=676112
Review-Url: https://codereview.chromium.org/2592833002
Cr-Commit-Position: refs/heads/master@{#441939}
Committed: https://chromium.googlesource.com/chromium/src/+/dd1727134d52f1e7aa349a360f254ec054fde0e9
Patch Set 1 #
Total comments: 24
Patch Set 2 : Address comments #
Total comments: 8
Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : Fix dependency issue by including vector_icons_public.h #Patch Set 5 : Fix failing implicit move on ChromeOS builds. #Messages
Total messages: 46 (27 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [WebPayments] Start populating the Payment Sheet. This change adds the header and the Order Summary section to the Payment Sheet in the Payment Request dialog. BUG=676112 ========== to ========== [WebPayments] Start populating the Payment Sheet. This change adds the header and the Order Summary section to the Payment Sheet in the Payment Request dialog. A screenshot of the Payment Sheet is here: https://drive.google.com/file/d/0B-DVbqI3huZGREpIaE5yMDZjZzA/view?usp=sharing A screenshot of the Order Summary shown when clicking the Order Summary row is here: https://drive.google.com/file/d/0B-DVbqI3huZGM0F0cC1sS2xLTWs/view?usp=sharing BUG=676112 ==========
Description was changed from ========== [WebPayments] Start populating the Payment Sheet. This change adds the header and the Order Summary section to the Payment Sheet in the Payment Request dialog. A screenshot of the Payment Sheet is here: https://drive.google.com/file/d/0B-DVbqI3huZGREpIaE5yMDZjZzA/view?usp=sharing A screenshot of the Order Summary shown when clicking the Order Summary row is here: https://drive.google.com/file/d/0B-DVbqI3huZGM0F0cC1sS2xLTWs/view?usp=sharing BUG=676112 ========== to ========== [WebPayments] Start populating the Payment Sheet. This change adds the header and the Order Summary section to the Payment Sheet in the Payment Request dialog. Both sheets still need many tweaks and extra features but this is the first step towards the dialog actually looking like the design specs. A screenshot of the Payment Sheet is here: https://drive.google.com/file/d/0B-DVbqI3huZGREpIaE5yMDZjZzA/view?usp=sharing A screenshot of the Order Summary shown when clicking the Order Summary row is here: https://drive.google.com/file/d/0B-DVbqI3huZGM0F0cC1sS2xLTWs/view?usp=sharing BUG=676112 ==========
anthonyvd@chromium.org changed reviewers: + rouslan@chromium.org, sky@chromium.org
Hello everyone, Can you please take a look at this first pass on the Payment Sheet? It lays out the Order Summary section and adds some utility functions that will make building the rest of the sheet a breeze. Thanks!
Description was changed from ========== [WebPayments] Start populating the Payment Sheet. This change adds the header and the Order Summary section to the Payment Sheet in the Payment Request dialog. Both sheets still need many tweaks and extra features but this is the first step towards the dialog actually looking like the design specs. A screenshot of the Payment Sheet is here: https://drive.google.com/file/d/0B-DVbqI3huZGREpIaE5yMDZjZzA/view?usp=sharing A screenshot of the Order Summary shown when clicking the Order Summary row is here: https://drive.google.com/file/d/0B-DVbqI3huZGM0F0cC1sS2xLTWs/view?usp=sharing BUG=676112 ========== to ========== [WebPayments] Start populating the Payment Sheet. This change adds the header and the Order Summary section to the Payment Sheet in the Payment Request dialog. Both sheets still need many tweaks and extra features but this is the first step towards the dialog actually looking like the design specs. A screenshot of the Payment Sheet is here: https://drive.google.com/file/d/0B-DVbqI3huZGREpIaE5yMDZjZzA/view?usp=sharing A screenshot of the Order Summary shown when clicking the Order Summary row is here: https://drive.google.com/file/d/0B-DVbqI3huZGM0F0cC1sS2xLTWs/view?usp=sharing BUG=676112 ==========
Are you following a mock here? https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/payments... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.cc:65: details_ = std::move(details); Run |details| through validatePaymentDetails(details, &error_message) first: https://cs.chromium.org/chromium/src/components/payments/payment_details_vali... This data comes from the renderer process, which the browser process should not trust. If the |details| are invalid, disconnect from the client_ and binding_, also destroy PaymentRequestImpl. https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_view_controller.cc (right): https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.cc:47: base::ASCIIToUTF16(impl()->details()->total->amount->currency)))); By the way, you will eventually need to write a C++ version of the currency string formatter and use it here and elsewhere: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... https://cs.chromium.org/chromium/src/chrome/android/junit/src/org/chromium/ch... Then I can make CurrencyStringFormatter.java call into your currency_string_formatter.h for code simplicity. https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_views_util.cc (right): https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:19: namespace { Here and elsewhere: nit: Anonymous namespace usually goes inside of the named namespace. https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:23: constexpr int kSideInsetSize = 14; Here and elsewhere: It's easier to read the code if the single-use constants are defined immediately before they are used, inside of the function. https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_views_util.h (right): https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.h:33: // | <- | Title | X | Thank you for the ASCII diagrams. They are most helpful. https://codereview.chromium.org/2592833002/diff/20001/components/autofill_str... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2592833002/diff/20001/components/autofill_str... components/autofill_strings.grdp:355: Total Use details->total->label instead. https://codereview.chromium.org/2592833002/diff/20001/components/autofill_str... components/autofill_strings.grdp:358: <ph name="TOTAL_LABEL">$1<ex>Total</ex></ph> <ph name="TOTAL_AMOUNT">$2<ex>12.34</ex></ph> <ph name="CURRENCY_CODE">$3<ex>USD</ex></ph> Is this format following a mock? If not, let's be consistent with Android format for total, which is: "[details->total->label] [details->total->amount->currency] [GetCurrencySymbol(details->total->amount->currency)][details->total->amount->value]" For example: "Total USD $12.34".
https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_view_controller.cc (right): https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.cc:59: if (sender->tag() == CLOSE_BUTTON_TAG) { no {}. optional: use a switch statement. https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_views_util.h (right): https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.h:19: enum PaymentRequestCommonTags { enum class? Style guide encourages using enum class for new code. https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:58: class PaymentSheetRow : public views::CustomButton { Do you really need to subclass here? Can't you create a CustomButton and configure it? Also, I don't think CustomButton is focusable by default, I suspect you'll want to make it focusable for your use, otherwise you won't be able to tab through the rows. https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:144: if (sender->tag() == CLOSE_BUTTON_TAG) { no {} optional: use a switch
anthonyvd@chromium.org changed reviewers: + mathp@chromium.org
+mathp as autofill_strings.grdp OWNER Thanks for your feedback, comments addressed. A link to the mocks have been added to the description (some VisD tweaks were made since I started implementing this, I'll make a separate CL for them). Cheers! https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/payments... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.cc:65: details_ = std::move(details); On 2017/01/04 at 15:55:27, rouslan wrote: > Run |details| through validatePaymentDetails(details, &error_message) first: > > https://cs.chromium.org/chromium/src/components/payments/payment_details_vali... > > This data comes from the renderer process, which the browser process should not trust. If the |details| are invalid, disconnect from the client_ and binding_, also destroy PaymentRequestImpl. Done. https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/order_summary_view_controller.cc (right): https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.cc:47: base::ASCIIToUTF16(impl()->details()->total->amount->currency)))); On 2017/01/04 at 15:55:27, rouslan wrote: > By the way, you will eventually need to write a C++ version of the currency string formatter and use it here and elsewhere: > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... > > https://cs.chromium.org/chromium/src/chrome/android/junit/src/org/chromium/ch... > > Then I can make CurrencyStringFormatter.java call into your currency_string_formatter.h for code simplicity. Makes sense, will do. https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/order_summary_view_controller.cc:59: if (sender->tag() == CLOSE_BUTTON_TAG) { On 2017/01/04 at 17:24:45, sky wrote: > no {}. > optional: use a switch statement. Done. https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_views_util.cc (right): https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:19: namespace { On 2017/01/04 at 15:55:27, rouslan wrote: > Here and elsewhere: > > nit: Anonymous namespace usually goes inside of the named namespace. Wow I've been doing the opposite for 2 years. It looks like different parts of the code do different things (view.cc has it nested but code in c/b/profiles has it outside for example). Changed it here and will stick to your suggested style in Payments code. https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:23: constexpr int kSideInsetSize = 14; On 2017/01/04 at 15:55:27, rouslan wrote: > Here and elsewhere: > > It's easier to read the code if the single-use constants are defined immediately before they are used, inside of the function. Done. https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_views_util.h (right): https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.h:19: enum PaymentRequestCommonTags { On 2017/01/04 at 17:24:45, sky wrote: > enum class? Style guide encourages using enum class for new code. Done. This kind of makes the code messier because I use those as tags so going with enum class introduces a bunch of casts. It does make the casts explicit though, so the slight messy-ness is probably for the best. https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.h:33: // | <- | Title | X | On 2017/01/04 at 15:55:27, rouslan wrote: > Thank you for the ASCII diagrams. They are most helpful. Credit where credit is due: inspiration comes from sebsg's amazing ASCII skills! https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:58: class PaymentSheetRow : public views::CustomButton { On 2017/01/04 at 17:24:45, sky wrote: > Do you really need to subclass here? Can't you create a CustomButton and configure it? > > Also, I don't think CustomButton is focusable by default, I suspect you'll want to make it focusable for your use, otherwise you won't be able to tab through the rows. It's currently a subclass because the rows will need some logic when their state changes (hovered/pressed/etc.) which (correct me if I'm wrong) imply overriding CustomButton::StateChanged(). WDYT? As for the focus thing, thanks and done! https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:144: if (sender->tag() == CLOSE_BUTTON_TAG) { On 2017/01/04 at 17:24:45, sky wrote: > no {} > optional: use a switch Done. https://codereview.chromium.org/2592833002/diff/20001/components/autofill_str... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2592833002/diff/20001/components/autofill_str... components/autofill_strings.grdp:355: Total On 2017/01/04 at 15:55:27, rouslan wrote: > Use details->total->label instead. Done. https://codereview.chromium.org/2592833002/diff/20001/components/autofill_str... components/autofill_strings.grdp:358: <ph name="TOTAL_LABEL">$1<ex>Total</ex></ph> <ph name="TOTAL_AMOUNT">$2<ex>12.34</ex></ph> <ph name="CURRENCY_CODE">$3<ex>USD</ex></ph> On 2017/01/04 at 15:55:27, rouslan wrote: > Is this format following a mock? If not, let's be consistent with Android format for total, which is: > > "[details->total->label] [details->total->amount->currency] [GetCurrencySymbol(details->total->amount->currency)][details->total->amount->value]" > > For example: "Total USD $12.34". Changed the order to "[Label] [Currency Code] [Formatted Amount]" since the Currency Formatter returns "The currency symbol followed by a space and the formatted number.", the result should be the same as Android and the mocks one I implement the C++ formatter.
Description was changed from ========== [WebPayments] Start populating the Payment Sheet. This change adds the header and the Order Summary section to the Payment Sheet in the Payment Request dialog. Both sheets still need many tweaks and extra features but this is the first step towards the dialog actually looking like the design specs. A screenshot of the Payment Sheet is here: https://drive.google.com/file/d/0B-DVbqI3huZGREpIaE5yMDZjZzA/view?usp=sharing A screenshot of the Order Summary shown when clicking the Order Summary row is here: https://drive.google.com/file/d/0B-DVbqI3huZGM0F0cC1sS2xLTWs/view?usp=sharing BUG=676112 ========== to ========== [WebPayments] Start populating the Payment Sheet. This change adds the header and the Order Summary section to the Payment Sheet in the Payment Request dialog. Both sheets still need many tweaks and extra features but this is the first step towards the dialog actually looking like the design specs (mocks here: https://folio.googleplex.com/chrome-ux/mocks/329-future-web-pay/latest/desktop#) A screenshot of the Payment Sheet is here: https://drive.google.com/file/d/0B-DVbqI3huZGREpIaE5yMDZjZzA/view?usp=sharing A screenshot of the Order Summary shown when clicking the Order Summary row is here: https://drive.google.com/file/d/0B-DVbqI3huZGM0F0cC1sS2xLTWs/view?usp=sharing BUG=676112 ==========
Description was changed from ========== [WebPayments] Start populating the Payment Sheet. This change adds the header and the Order Summary section to the Payment Sheet in the Payment Request dialog. Both sheets still need many tweaks and extra features but this is the first step towards the dialog actually looking like the design specs (mocks here: https://folio.googleplex.com/chrome-ux/mocks/329-future-web-pay/latest/desktop#) A screenshot of the Payment Sheet is here: https://drive.google.com/file/d/0B-DVbqI3huZGREpIaE5yMDZjZzA/view?usp=sharing A screenshot of the Order Summary shown when clicking the Order Summary row is here: https://drive.google.com/file/d/0B-DVbqI3huZGM0F0cC1sS2xLTWs/view?usp=sharing BUG=676112 ========== to ========== [WebPayments] Start populating the Payment Sheet. This change adds the header and the Order Summary section to the Payment Sheet in the Payment Request dialog. Both sheets still need many tweaks and extra features but this is the first step towards the dialog actually looking like the design specs. Mocks here: https://folio.googleplex.com/chrome-ux/mocks/329-future-web-pay/latest/desktop# A screenshot of the Payment Sheet is here: https://drive.google.com/file/d/0B-DVbqI3huZGREpIaE5yMDZjZzA/view?usp=sharing A screenshot of the Order Summary shown when clicking the Order Summary row is here: https://drive.google.com/file/d/0B-DVbqI3huZGM0F0cC1sS2xLTWs/view?usp=sharing BUG=676112 ==========
https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:111: SetFocusForPlatform(); Hmm it looks like this isn't the only call needed to enable focus. Will investigate.
lgtm % comments https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_views_util.cc (right): https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_views_util.cc:19: namespace { On 2017/01/04 19:08:55, anthonyvd wrote: > On 2017/01/04 at 15:55:27, rouslan wrote: > > Here and elsewhere: > > > > nit: Anonymous namespace usually goes inside of the named namespace. > > Wow I've been doing the opposite for 2 years. It looks like different parts of > the code do different things (view.cc has it nested but code in c/b/profiles has > it outside for example). Changed it here and will stick to your suggested style > in Payments code. Thank you. As long as we're consistent, I'm OK with what you prefer. https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/payments... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.cc:66: if (!payments::validatePaymentDetails(details, &error)) { LOG(ERROR) << error; https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_dialog.cc (right): https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:30: nit: no blank line https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:36: nit: no blank line
This looks good, but I'm not approving given your comment about investigating focus. https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:58: class PaymentSheetRow : public views::CustomButton { On 2017/01/04 19:08:55, anthonyvd wrote: > On 2017/01/04 at 17:24:45, sky wrote: > > Do you really need to subclass here? Can't you create a CustomButton and > configure it? > > > > Also, I don't think CustomButton is focusable by default, I suspect you'll > want to make it focusable for your use, otherwise you won't be able to tab > through the rows. > > It's currently a subclass because the rows will need some logic when their state > changes (hovered/pressed/etc.) which (correct me if I'm wrong) imply overriding > CustomButton::StateChanged(). WDYT? Indeed you are right. As you didn't override any functions I assumed you didn't need to subclass. > As for the focus thing, thanks and done!
On 2017/01/04 20:56:32, sky wrote: > This looks good, but I'm not approving given your comment about investigating > focus. > > https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): > > https://codereview.chromium.org/2592833002/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:58: class > PaymentSheetRow : public views::CustomButton { > On 2017/01/04 19:08:55, anthonyvd wrote: > > On 2017/01/04 at 17:24:45, sky wrote: > > > Do you really need to subclass here? Can't you create a CustomButton and > > configure it? > > > > > > Also, I don't think CustomButton is focusable by default, I suspect you'll > > want to make it focusable for your use, otherwise you won't be able to tab > > through the rows. > > > > It's currently a subclass because the rows will need some logic when their > state > > changes (hovered/pressed/etc.) which (correct me if I'm wrong) imply > overriding > > CustomButton::StateChanged(). WDYT? > > Indeed you are right. As you didn't override any functions I assumed you didn't > need to subclass. > > > As for the focus thing, thanks and done! Also, the rows may be getting focus with the focus change, but it's likely the border that is responsible for drawing focus so that you may now see it. I could certainly be wrong here though.
Dug a little more about focus and replied to my comment with the explanation. Let me know what you think. Thanks! https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/payments... File chrome/browser/payments/payment_request_impl.cc (right): https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/payments... chrome/browser/payments/payment_request_impl.cc:66: if (!payments::validatePaymentDetails(details, &error)) { On 2017/01/04 at 19:17:11, rouslan wrote: > LOG(ERROR) << error; Done. https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_dialog.cc (right): https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_dialog.cc:30: On 2017/01/04 at 19:17:11, rouslan wrote: > nit: no blank line Done. https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:36: On 2017/01/04 at 19:17:11, rouslan wrote: > nit: no blank line Done. https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:111: SetFocusForPlatform(); On 2017/01/04 at 19:15:30, anthonyvd wrote: > Hmm it looks like this isn't the only call needed to enable focus. Will investigate. It turns out focus is hard! From what I can gather, the ViewStack is getting focus when the dialog is shown. However, it's GetNextFocusableView() function returns nullptr, even after a child view is added to it (which I thought would set up the focus chain properly, but I'm probably missing something). The rows themselves never receive focus. In any case, I realized there's a lot more work that will be required to get focus to work properly in the dialog. Since the ViewStack can hide and show different views, this code will need a way to handle the case where a new view is pushed on the stack while focus somewhere on a child of the view being hidden. There seem to be 2 ways of doing this: 1. Manually setting up the focus chain in views being pushed on ViewStack and calling RequestFocus() appropriately in ViewStack::Push/Pop(). This would probably have the unwanted side-effect of always having something focused (which might be avoidable with an invisible dummy view, I don't know). 2. Having ViewStack implement FocusTraversable and supply a custom FocusSearch capable of finding the correct focus target in the stack. I don't know anything about that mechanism so I might be wrong but this seems to be one of the cases that require it. sky@ WDYT? Unless I'm missing something that makes this much easier, I'd also like to implement this properly in another CL so I removed the SetFocusForPlatform() call here.
On 2017/01/05 16:45:42, anthonyvd wrote: > Dug a little more about focus and replied to my comment with the explanation. > Let me know what you think. > > Thanks! > > https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/payments... > File chrome/browser/payments/payment_request_impl.cc (right): > > https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/payments... > chrome/browser/payments/payment_request_impl.cc:66: if > (!payments::validatePaymentDetails(details, &error)) { > On 2017/01/04 at 19:17:11, rouslan wrote: > > LOG(ERROR) << error; > > Done. > > https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/payments/payment_request_dialog.cc (right): > > https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_request_dialog.cc:30: > On 2017/01/04 at 19:17:11, rouslan wrote: > > nit: no blank line > > Done. > > https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): > > https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:36: > On 2017/01/04 at 19:17:11, rouslan wrote: > > nit: no blank line > > Done. > > https://codereview.chromium.org/2592833002/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:111: > SetFocusForPlatform(); > On 2017/01/04 at 19:15:30, anthonyvd wrote: > > Hmm it looks like this isn't the only call needed to enable focus. Will > investigate. > > It turns out focus is hard! > > From what I can gather, the ViewStack is getting focus when the dialog is shown. I'm surprised the ViewStack is focused by default. By default the focus behavior for a View is never, so, how does ViewStack end up with focus? > However, it's GetNextFocusableView() function returns nullptr, even after a > child view is added to it (which I thought would set up the focus chain > properly, but I'm probably missing something). The rows themselves never receive > focus. You only use SetNextFocusableView if you need to specify an explicit focus order. The default behavior is to determine focus order based on order views are added, which works in most cases. > In any case, I realized there's a lot more work that will be required to get > focus to work properly in the dialog. Since the ViewStack can hide and show > different views, this code will need a way to handle the case where a new view > is pushed on the stack while focus somewhere on a child of the view being > hidden. There seem to be 2 ways of doing this: > > 1. Manually setting up the focus chain in views being pushed on ViewStack and > calling RequestFocus() appropriately in ViewStack::Push/Pop(). This would > probably have the unwanted side-effect of always having something focused (which > might be avoidable with an invisible dummy view, I don't know). > 2. Having ViewStack implement FocusTraversable and supply a custom FocusSearch > capable of finding the correct focus target in the stack. I don't know anything > about that mechanism so I might be wrong but this seems to be one of the cases > that require it. > > sky@ WDYT? Unless I'm missing something that makes this much easier, I'd also > like to implement this properly in another CL so I removed the > SetFocusForPlatform() call here. I would hope you don't need to manually alter the focus change. What you likely need to do is if focus is in a descendant of the ViewStack and you push/pop, then move focus to a child of the new view being animated in. Another option is to move focus to the ViewStack itself in this situation. I think you'll also want accelerators that can trigger going back. Part of the problem likely is because you don't change the visibility of any of the views in stack_. Really only the view at the top of the stack should be visible, all others should be hidden. If you don't do this then the focus chain will include all views in the stack_, which you don't want. I'm ok with investigating this separately, and I'm happy to answer more specific questions as I'm glossing over some bits. LGTM
lgtm sorry for the delay https://codereview.chromium.org/2592833002/diff/60001/components/autofill_str... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2592833002/diff/60001/components/autofill_str... components/autofill_strings.grdp:355: <ph name="TOTAL_LABEL">$1<ex>Total</ex></ph> <ph name="CURRENCY_CODE">$2<ex>USD</ex></ph> <ph name="FORMATTED_TOTAL_AMOUNT">$3<ex>$ 12.34</ex></ph> nit: the <ex> doesn't look like the final result, shouldn't it be USD 12.34? Also, are there plans to add the $ sign?
On 2017/01/05 19:06:15, Mathieu Perreault wrote: > lgtm sorry for the delay > > https://codereview.chromium.org/2592833002/diff/60001/components/autofill_str... > File components/autofill_strings.grdp (right): > > https://codereview.chromium.org/2592833002/diff/60001/components/autofill_str... > components/autofill_strings.grdp:355: <ph > name="TOTAL_LABEL">$1<ex>Total</ex></ph> <ph > name="CURRENCY_CODE">$2<ex>USD</ex></ph> <ph > name="FORMATTED_TOTAL_AMOUNT">$3<ex>$ 12.34</ex></ph> > nit: the <ex> doesn't look like the final result, shouldn't it be USD 12.34? "$ 12.34" is right. "USD" is in <ex> for "CURRENCY_CODE". > Also, are there plans to add the $ sign? Yep ;-)
On 2017/01/05 19:08:31, rouslan wrote: > On 2017/01/05 19:06:15, Mathieu Perreault wrote: > > lgtm sorry for the delay > > > > > https://codereview.chromium.org/2592833002/diff/60001/components/autofill_str... > > File components/autofill_strings.grdp (right): > > > > > https://codereview.chromium.org/2592833002/diff/60001/components/autofill_str... > > components/autofill_strings.grdp:355: <ph > > name="TOTAL_LABEL">$1<ex>Total</ex></ph> <ph > > name="CURRENCY_CODE">$2<ex>USD</ex></ph> <ph > > name="FORMATTED_TOTAL_AMOUNT">$3<ex>$ 12.34</ex></ph> > > nit: the <ex> doesn't look like the final result, shouldn't it be USD 12.34? > > "$ 12.34" is right. "USD" is in <ex> for "CURRENCY_CODE". > > > Also, are there plans to add the $ sign? > > Yep ;-) Sorry I got confused because the screenshot of the feature has 55.00 USD and doesn't have the $. My bad!
https://codereview.chromium.org/2592833002/diff/60001/components/autofill_str... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2592833002/diff/60001/components/autofill_str... components/autofill_strings.grdp:355: <ph name="TOTAL_LABEL">$1<ex>Total</ex></ph> <ph name="CURRENCY_CODE">$2<ex>USD</ex></ph> <ph name="FORMATTED_TOTAL_AMOUNT">$3<ex>$ 12.34</ex></ph> On 2017/01/05 at 19:06:15, Mathieu Perreault wrote: > nit: the <ex> doesn't look like the final result, shouldn't it be USD 12.34? > > Also, are there plans to add the $ sign? The full formatted text will be something like "Total USD $12.34". Since the "symbol+amount" part is the output of the currency formatter, we only need 3 parts: "Total" (label), "USD" (currency code), and "$12.34" (formatted total amount). This is what the 3 <ex>'s here are conveying. The issue, as you noticed, is that the c++ formatter isn't implemented so the code consuming this format string isn't passing it the correct last piece currently. Once it's implemented, the code using this format string will be fixed, the <ex> will be correct, and the $ sign (or other currency symbols) will be present!
Thanks everyone for the review! sky: I should have said that the ViewStack has its RequestFocus function called, my bad. It doesn't actually get the focus. I'll investigate your suggestion and definitely take you up on your offer for help as I make sense of all of it :)
The CQ bit was checked by anthonyvd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2592833002/#ps60001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by anthonyvd@chromium.org
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by anthonyvd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, rouslan@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2592833002/#ps120001 (title: "Fix failing implicit move on ChromeOS builds.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483713714860090,
"parent_rev": "800554636cb71f11b241026b44557097bab34458", "commit_rev":
"dd1727134d52f1e7aa349a360f254ec054fde0e9"}
Message was sent while issue was closed.
Description was changed from ========== [WebPayments] Start populating the Payment Sheet. This change adds the header and the Order Summary section to the Payment Sheet in the Payment Request dialog. Both sheets still need many tweaks and extra features but this is the first step towards the dialog actually looking like the design specs. Mocks here: https://folio.googleplex.com/chrome-ux/mocks/329-future-web-pay/latest/desktop# A screenshot of the Payment Sheet is here: https://drive.google.com/file/d/0B-DVbqI3huZGREpIaE5yMDZjZzA/view?usp=sharing A screenshot of the Order Summary shown when clicking the Order Summary row is here: https://drive.google.com/file/d/0B-DVbqI3huZGM0F0cC1sS2xLTWs/view?usp=sharing BUG=676112 ========== to ========== [WebPayments] Start populating the Payment Sheet. This change adds the header and the Order Summary section to the Payment Sheet in the Payment Request dialog. Both sheets still need many tweaks and extra features but this is the first step towards the dialog actually looking like the design specs. Mocks here: https://folio.googleplex.com/chrome-ux/mocks/329-future-web-pay/latest/desktop# A screenshot of the Payment Sheet is here: https://drive.google.com/file/d/0B-DVbqI3huZGREpIaE5yMDZjZzA/view?usp=sharing A screenshot of the Order Summary shown when clicking the Order Summary row is here: https://drive.google.com/file/d/0B-DVbqI3huZGM0F0cC1sS2xLTWs/view?usp=sharing BUG=676112 Review-Url: https://codereview.chromium.org/2592833002 Cr-Commit-Position: refs/heads/master@{#441939} Committed: https://chromium.googlesource.com/chromium/src/+/dd1727134d52f1e7aa349a360f25... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/dd1727134d52f1e7aa349a360f25... |
