Skip to content
New issue

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

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

Already on GitHub? # to your account

Allow popups from inside brave://getting-started iframe #27497

Merged
merged 2 commits into from
Feb 5, 2025

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Feb 4, 2025

Fixes brave/brave-browser#43719

Security review https://github.com/brave/reviews/issues/1858

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • 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 wiki
    • npm run presubmit wiki, 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:

  1. Launch Brave
  2. Visit brave://getting-started
  3. Scroll down to very bottom and click button Learn more
  4. Link should open in a new tab
  5. Go back to brave://getting-started
  6. Scroll down (mid-way) to See a better web button and click it
  7. Link should open in a new tab

@bsclifton bsclifton requested a review from zenparsing February 4, 2025 21:05
@github-actions github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Feb 4, 2025
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@bsclifton
Copy link
Member Author

Built locally and confirmed this works great!

Thanks for the review @zenparsing and thanks for the security review @diracdeltas 😄

@bsclifton bsclifton merged commit 63bde3c into master Feb 5, 2025
18 checks passed
@bsclifton bsclifton deleted the bsc-getting-started-target-blank branch February 5, 2025 04:38
@github-actions github-actions bot added this to the 1.77.x - Nightly milestone Feb 5, 2025
@brave-builds
Copy link
Collaborator

Released in v1.77.17

@kjozwiak
Copy link
Member

kjozwiak commented Feb 11, 2025

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.77.33 Chromium: 133.0.6943.54 (Official Build) nightly (64-bit)
-- | --
Revision | f3ef3d8f315fd70c52dacb37981f31a53a8f0b49
OS | Windows 11 Version 24H2 (Build 26100.2894)

Using the STR/Cases outlined via #27497 (comment) & brave/brave-browser#43719 (comment), ensured that both See a better Web & Learn more were opening new tabs as per the following:

  • ensured that clicking on See a better Web was opening https://search.brave.com/search?q=Best%20cheap%20laptops&source=welcomePages&summary=1 in a new tab without issues
  • ensured that clicking on Learn more was opening https://brave.com/whats-new/ in a new tab without issues
Recording.2025-02-11.172123.mp4

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links in brave://getting-started don't open properly
4 participants