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

Fix shields for fingerprinting v2 android #6702

Merged
merged 1 commit into from
Sep 25, 2020

Conversation

deeppandya
Copy link
Contributor

@deeppandya deeppandya commented Sep 25, 2020

Resolves brave/brave-browser#10688
Resolves brave/brave-browser#10953
Resolves brave/brave-browser#11786

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@deeppandya deeppandya added CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Sep 25, 2020
@deeppandya deeppandya added this to the 1.16.x - Nightly milestone Sep 25, 2020
@deeppandya deeppandya self-assigned this Sep 25, 2020
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

@deeppandya deeppandya merged commit 2bb1c95 into master Sep 25, 2020
@deeppandya deeppandya deleted the sync_shields_android_with_desktop branch September 25, 2020 14:45
brave-builds pushed a commit that referenced this pull request Sep 25, 2020
@kjozwiak
Copy link
Member

kjozwiak commented Sep 29, 2020

Verification PASSED using the following two devices:

  • Samsung S10+ running Android 10 using 1.16.43 CR: 86.0.4240.55

Running through brave/brave-browser#10953:

Samsung S10+ Samsung S10+ Samsung S10+
Screenshot_20200928-233006_Brave - Nightly Screenshot_20200928-232734_Brave - Nightly Screenshot_20200928-232506_Brave - Nightly

Running through brave/brave-browser#10688:

  • ensured that Block all cookies, Block cross-site cookies or Allow all cookies appear as selected under Advanced Controls -> Block Cookies once the user has selected the setting and clicked on Done.
    • ensured that selecting the < (back) button also worked as expected
  • ensured that Fingerprinting blocked (strict, may break sites), Fingerprinting blocked (standard) or Allow all fingerprinting appear as selected under Advanced Controls -> Block fingerprinting once the user has selected the setting and clicked on Done.
    • ensured that selecting the < (back) button also worked as expected

Running through brave/brave-browser#11786:

Went through the STR/Cases outlined via brave/brave-browser#11786 (comment) and ensured that the FP settings were correctly being retained when switching/changing them.

@kjozwiak
Copy link
Member

@srirambv mind taking a look at brave/brave-browser#10953 on your tablet? I checked using 1.16.43 CR: 86.0.4240.55 and it seems like the padding looks exactly the same as your original example as per brave/brave-browser#10953 (comment). Here's what I'm seeing on the tablet when using Japanese:

Screenshot_20200929-001624_Brave - Nightly

@srirambv
Copy link
Contributor

Verification passed on Samsung Tab A with Android 10 running 1.16.43 x64 nightly build

brave/brave-browser#10688

  • Verified shields setting heading has proper padding
English (Cookies) English (Fingerprinting) Spanish (Cookies) Spanish (Fingerprinting) Japanese (Cookies) Japanese (Fingerprinting)
image image image image image image

brave/brave-browser#10953

  • Verified selecting individual settings for Cookies/Fingerprinting just says Block Cookies or Block Fingerprinting instead of selecting the individual setting text

brave/brave-browser#11786

  • Verified selecting different fingerprint settings is retained between page reloads and doesn't retain one setting even if changed to other

Logged brave/brave-browser#11905 as a followup to brave/brave-browser#10688 to add more padding to the done button so that it doesn't look bad on the translated text

@kjozwiak brave/brave-browser#10688 the title had no padding as compared to what it is now where there is enough padding for the title. Below is a comparison (maybe not very noticeable in the screenshot)

Old New
image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64
Projects
None yet
4 participants