Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1032193003:
Autofill OSX: Show error message if expiration date is in the past. (Closed)
|
Created:
5 years, 9 months ago by bondd Modified:
5 years, 9 months ago CC:
chromium-reviews, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@cup_09_add_error_text_03 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAutofill OSX: Show error message if expiration date is in the past.
Show error message and disable Verify button if expiration date is in
the past. Logic is a direct port from CardUnmaskPromptViews.
BUG=448572
Committed: https://crrev.com/47295d7a4118361bb558f3be13b683f9df2acf4e
Cr-Commit-Position: refs/heads/master@{#322449}
Patch Set 1 #
Total comments: 6
Messages
Total messages: 13 (3 generated)
bondd@chromium.org changed reviewers: + estade@chromium.org, groby@chromium.org
bondd@chromium.org changed required reviewers: + groby@chromium.org
https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (left): https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:214: DCHECK(controller); After looking deeper into dialog close+destruction code I no longer think this DCHECK is necessary. I'm removing it in this CL to be consistent with -expirationDateIsValid. (It would be confusing to DCHECK in some methods and not others.) https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:230: DCHECK(controller); Same here. https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:244: [self setRetriableErrorMessage:base::string16()]; Open CL https://codereview.chromium.org/1038503003/ makes it so that -setRetriableErrorMessage no longer calls -performLayout. I'll add the -performLayout call to this method during the rebase of whichever CL doesn't land first.
https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:245: } else if ([monthPopup_ indexOfSelectedItem] != monthPopupDefaultIndex_ && Why would the default month/year not be validated?
https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:245: } else if ([monthPopup_ indexOfSelectedItem] != monthPopupDefaultIndex_ && On 2015/03/25 21:44:58, groby wrote: > Why would the default month/year not be validated? Yeah, I noticed this too. My guess is that it's because we don't want the message to show up after the user changes one of the popups away from the default, but hasn't had a chance to do the other one yet. This is the same logic as in the Views version. Maybe estade@ can comment on whether this is a feature or a bug. FWIW the behavior makes sense to me.
https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:245: } else if ([monthPopup_ indexOfSelectedItem] != monthPopupDefaultIndex_ && On 2015/03/26 00:42:27, bondd wrote: > On 2015/03/25 21:44:58, groby wrote: > > Why would the default month/year not be validated? > > Yeah, I noticed this too. My guess is that it's because we don't want the > message to show up after the user changes one of the popups away from the > default, but hasn't had a chance to do the other one yet. > This is the same logic as in the Views version. Maybe estade@ can comment on > whether this is a feature or a bug. FWIW the behavior makes sense to me. Is the default item at least not selectable? (I.e. once I picked a year, I can't go back to "Year"?)
On 2015/03/26 01:24:41, groby wrote: > https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... > File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): > > https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:245: } else > if ([monthPopup_ indexOfSelectedItem] != monthPopupDefaultIndex_ && > On 2015/03/26 00:42:27, bondd wrote: > > On 2015/03/25 21:44:58, groby wrote: > > > Why would the default month/year not be validated? > > > > Yeah, I noticed this too. My guess is that it's because we don't want the > > message to show up after the user changes one of the popups away from the > > default, but hasn't had a chance to do the other one yet. > > This is the same logic as in the Views version. Maybe estade@ can comment on > > whether this is a feature or a bug. FWIW the behavior makes sense to me. > > Is the default item at least not selectable? (I.e. once I picked a year, I can't > go back to "Year"?) Nope, default item is always selectable (on Linux and Mac at least). Do we have any Mac dialogs where we do that? If so, and Evan's okay with it (Evan?), I can do that here.
On 2015/03/26 02:23:27, bondd wrote: > On 2015/03/26 01:24:41, groby wrote: > > > https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... > > File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm > (right): > > > > > https://codereview.chromium.org/1032193003/diff/1/chrome/browser/ui/cocoa/aut... > > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:245: } else > > if ([monthPopup_ indexOfSelectedItem] != monthPopupDefaultIndex_ && > > On 2015/03/26 00:42:27, bondd wrote: > > > On 2015/03/25 21:44:58, groby wrote: > > > > Why would the default month/year not be validated? > > > > > > Yeah, I noticed this too. My guess is that it's because we don't want the > > > message to show up after the user changes one of the popups away from the > > > default, but hasn't had a chance to do the other one yet. this > > > This is the same logic as in the Views version. Maybe estade@ can comment on > > > whether this is a feature or a bug. FWIW the behavior makes sense to me. > > > > Is the default item at least not selectable? (I.e. once I picked a year, I > can't > > go back to "Year"?) > > Nope, default item is always selectable (on Linux and Mac at least). Do we have > any Mac dialogs where we do that? If so, and Evan's okay with it (Evan?), I can > do that here. I think that's orthogonal and probably not worth the effort
I'm definitely not happy with popup titles being selectable, but LGTM for this CL - I agree it's orthogonal. But we _should_ fix it. It makes for a better UI.
The CQ bit was checked by bondd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1032193003/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/47295d7a4118361bb558f3be13b683f9df2acf4e Cr-Commit-Position: refs/heads/master@{#322449} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
