Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saved Credit card info modal shows Sign in to Chrome #1363

Closed
srirambv opened this issue Oct 1, 2018 · 7 comments · Fixed by brave/brave-core#588
Closed

Saved Credit card info modal shows Sign in to Chrome #1363

srirambv opened this issue Oct 1, 2018 · 7 comments · Fixed by brave/brave-core#588

Comments

@srirambv
Copy link
Contributor

srirambv commented Oct 1, 2018

Test Plan

  1. Visit https://mqi.ie/donate/
  2. Enter details
  3. Ensure no sign in message is shown on save credit card info modal

Description

Reported by @johnnyryan ,Saved Credit card info modal shows Sign in to Chrome

Steps to Reproduce

  1. Enter credit card details on a page
  2. Click on save prompt
  3. Shows sign in to Chrome

Actual result:

image

Expected result:

Should not show Sign in to Chrome message

Reproduces how often:

Easy

Brave version (chrome://version info)

Brave 0.55.10 Chromium: 70.0.3538.22 (Official Build) beta (64-bit)
Revision ac9418ba9c3bd7f6baaffa0b055dfe147e0f8364-refs/branch-heads/3538@{#468}
OS All

Reproducible on current release:

N/A

Website problems only:

  • Does the issue resolve itself when disabling Brave Shields?
  • Is the issue reproducible on the latest version of Chrome?

Additional Information

cc: @bbondy for milestone prioritization

@srirambv srirambv added this to the 1.0 (0.56.x) milestone Oct 1, 2018
@bbondy bbondy modified the milestones: 1.0 (0.56.x), Releasable builds 0.55.x Oct 1, 2018
@hferreiro
Copy link
Contributor

@srirambv I cannot get Brave to save my credit card details, where are you testing?

@rebron
Copy link
Collaborator

rebron commented Oct 3, 2018

@hferreiro
Copy link
Contributor

@bbondy in this case, I cannot just modify the strings involved. The dialog is created with a BubbleSyncPromoView which needs a format string and another string which will be linked to the Google sign in: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/autofill/save_card_manage_cards_bubble_views.cc?l=56

I guess we can delete or replace the dialog completely.

@bbondy
Copy link
Member

bbondy commented Oct 5, 2018

We were talking about this yesterday morning but just syncing here:
I think you can use a chromium_src override to just change the string that is used to another one from our own grd in src/brave using #define / #undef

I assume you mean for this:

 <message name="IDS_AUTOFILL_SIGNIN_PROMO_MESSAGE_DICE_DISABLED" desc="Text of the sign in promo bubble and manage cards footnote.">
<ph name="SIGN_IN_LINK">$1<ex></ex></ph>
  </message>

@hferreiro
Copy link
Contributor

Just modifying the strings is not enough. IDS_AUTOFILL_SIGNIN_PROMO_MESSAGE_DICE_DISABLED is supposed to contain a placeholder and IDS_AUTOFILL_SIGNIN_PROMO_LINK_DICE_DISABLED needs to be a non-empty string to be styled as a link.

@hferreiro
Copy link
Contributor

Sorry, the issue was closed when pushing to a fork. Btw, I used src/components/test/data/autofill/credit_card_upload_form_address_and_cc.html to test this issue.

@srirambv
Copy link
Contributor Author

srirambv commented Oct 11, 2018

Verification Passed on

Brave 0.55.12 Chromium: 70.0.3538.45 (Official Build) beta (64-bit)
Revision cbdc32e4334458954e9def214d7e5fa1ca1960eb-refs/branch-heads/3538@{#830}
OS Linux

image

Verification Passed on

Brave 0.55.12 Chromium: 70.0.3538.45 (Official Build) beta (64-bit)
Revision cbdc32e4334458954e9def214d7e5fa1ca1960eb-refs/branch-heads/3538@{#830}
OS Windows

Verified passed with

Brave 0.55.14 Chromium: 70.0.3538.54 (Official Build) beta(64-bit)
Revision 4f8e578b6680574714e9ed3bb9f02922b4dde40d-refs/branch-heads/3538@{#937}
OS Mac OS X

screen shot 2018-10-16 at 4 00 06 pm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants