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

Toned down interstitials for top-level blocking. #8603

Merged
merged 1 commit into from
Apr 22, 2021

Conversation

mkarolin
Copy link
Collaborator

@mkarolin mkarolin commented Apr 22, 2021

Also:

  • switched to Chromium's information (i) icon instead of the red exclamation point.
  • changes the "Proceed" button to be the default (highlighted).

Fixes brave/brave-browser#15433

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@mkarolin mkarolin self-assigned this Apr 22, 2021
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++

@simonhong
Copy link
Member

simonhong commented Apr 22, 2021

I think we need sub issue from brave/brave-browser#15189 for this partial fix.

Copy link
Member

@fmarier fmarier left a comment

Choose a reason for hiding this comment

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

Would it be easy to change the default button too? In the screenshots, the "Proceed" button is the one that's highlighted.

</message>
<message name="IDS_DOMAIN_BLOCK_PRIMARY_PARAGRAPH" desc="Main paragraph of domain block interstitial page.">
Brave has prevented the following site from loading:
</message>
<message name="IDS_DOMAIN_BLOCK_EXPLANATION" desc="Second paragraph of domain block interstitial page.">
This site may be fraudulent, bounce you between various sites to set tracking cookies, or perform other privacy invasive actions.
This page may be trying to link some of your personal data with a cookie before taking you to your intended destination. You can choose to allow this potential privacy leak and proceed to the next page, or you can block this tracking attempt by going back to the previous page.
Copy link
Member

Choose a reason for hiding this comment

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

Let's use @karenkliu 's wording (see the screenshots instead of the GitHub comments) instead of what I proposed.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I should have commented to that effect on the issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fmarier the heading as well?

@mkarolin mkarolin force-pushed the maxk-fix-domain-block-interstitial branch from 2809e82 to d227118 Compare April 22, 2021 02:00
@mkarolin mkarolin requested review from fmarier and simonhong April 22, 2021 02:02
@mkarolin
Copy link
Collaborator Author

mkarolin commented Apr 22, 2021

Switched "Proceed" button to be the default.
Switched to the text from the screenshot in brave/brave-browser#15189
Created a sub-issue that this PR fixes (brave/brave-browser#15433)

image

Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++

Also, switched to Chromium's information (i) icon instead of the red
exclamation point.

Fixes brave/brave-browser#15433
@mkarolin mkarolin force-pushed the maxk-fix-domain-block-interstitial branch from d227118 to f6335b3 Compare April 22, 2021 04:09
Copy link
Member

@fmarier fmarier left a comment

Choose a reason for hiding this comment

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

Thanks @mkarolin !

@mkarolin
Copy link
Collaborator Author

CI browser test failures on Linux and MacOS are known and unrelated to this PR.

@mkarolin mkarolin merged commit a2c1be4 into master Apr 22, 2021
@mkarolin mkarolin deleted the maxk-fix-domain-block-interstitial branch April 22, 2021 17:14
@mkarolin mkarolin added this to the 1.25.x - Nightly milestone Apr 22, 2021
brave-builds pushed a commit that referenced this pull request Apr 22, 2021
mkarolin pushed a commit that referenced this pull request Apr 26, 2021
@kjozwiak
Copy link
Member

Verification PASSED on Win 10 x64 using the following build:

Brave | 1.25.42 Chromium: 90.0.4430.85 (Official Build) nightly (64-bit)
-- | --
Revision | 5bc145d831c180d9ff94f29a0d7a2e1cbd30ef36-refs/branch-heads/4430@{#1311}
OS | Windows 10 OS Version 2009 (Build 19042.928)

Screenshot 2021-04-26 221757

kylehickinson pushed a commit that referenced this pull request Jan 12, 2024
…8633)

* Update `SwapTokenStore` & `SwapTokenStoreTests` for Jupiter v6 changes.

* Update BraveCore to v1.63.107
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toned down interstitial text and icon for top-level blocking.
4 participants