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

Added BraveHelpBubbleView as part of the new onboarding flow #15198

Merged
merged 18 commits into from
Dec 7, 2023

Conversation

nullhook
Copy link
Contributor

@nullhook nullhook commented Sep 23, 2022

Resolves brave/brave-browser#27490

This PR adds a new UI element inside brave/browser/ui, the BraveHelpBubbleView is part of the new onboarding experience. The BraveHelpBubbleView is a ring that pulsates around a tracked element with an anchored bubble.

The BrowserView currently owns the BraveHelpBubbleView, which allows it to be absolutely positioned on top. The BraveHelpBubbleDelegate has a customized bubble border with an arrow.

The following logic has been added:

  1. The BraveHelpBubbleView is a one-shot view only and should be safely destroyed
  2. The BraveHelpBubbleView should transfer user clicks to the target (in this case, the Shield's icon)
  3. The BraveHelpBubbleView position should be updated on window resize, and updated positions of the icon should be sent
  4. The BraveHelpBubbleView should be hidden when the location bar is in an active state of typing
  5. The BraveHelpBubbleView should be unique to the browser window and not part of the devtools or any other window
  6. The BraveHelpBubbleView should only be visible when a tracked element is visible
  7. The BraveHelpBubbleView should show the dialog only when Shields is active on the tab

image

Platform: MacOS

image

Platform: Linux

image

Upstream border rendering issue

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
  • 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, 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:

If you installed your browser within the past 7 days, you will see the bubble. If not, you can use the command line option --force-first-run to view it. Note that the bubble can only be viewed once. After closing it for the first time, it will not be shown again. However, for testing purposes, you can start the browser instance with the option --user_data_dir_name= to create a new profile and local state file.

It's also important to test with --disable-gpu flag. In this condition you shouldn't be able to see the pulse animation.

@nullhook nullhook changed the title Added custom widget Added custom widget for onboarding UI Sep 29, 2022
@nullhook nullhook force-pushed the views/pulse branch 3 times, most recently from 4fbad01 to fd09231 Compare October 4, 2022 15:42
@nullhook nullhook marked this pull request as ready for review October 17, 2022 21:44
Copy link
Contributor

@sangwoo108 sangwoo108 left a comment

Choose a reason for hiding this comment

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

hi @nullhook . Sorry for delayed response. I guess this is still on its way but I left some comments. Thanks :)

@nullhook nullhook force-pushed the views/pulse branch 3 times, most recently from aab162d to 8611851 Compare December 15, 2022 00:04
@nullhook
Copy link
Contributor Author

nullhook commented Dec 21, 2022

Note to self:

How to fade the browser window?

This requires a WidgetFadeAnimator attached to the browser frame

std::unique_ptr<views::WidgetFadeAnimator> fade_animator_;

fade_animator_ = std::make_unique<views::WidgetFadeAnimator>(frame_);
fade_animator_->set_fade_out_duration(base::Milliseconds(1000));
fade_animator_->FadeOut();

@nullhook nullhook changed the title Added custom widget for onboarding UI Added custom widget as part of the new onboarding flow Dec 21, 2022
@nullhook nullhook changed the title Added custom widget as part of the new onboarding flow Added BraveHelpBubbleView as part of the new onboarding flow Dec 22, 2022
@nullhook nullhook force-pushed the views/pulse branch 3 times, most recently from aeb9369 to eb474b4 Compare December 29, 2022 07:02
@nullhook nullhook force-pushed the views/pulse branch 4 times, most recently from 16a3d5b to d11845a Compare January 3, 2023 15:57
@nullhook nullhook force-pushed the views/pulse branch 3 times, most recently from a11fd68 to e66c311 Compare January 17, 2023 00:16
Copy link
Contributor

github-actions bot commented Dec 5, 2023

[puLL-Merge] - brave/brave-core@15198

Description

This pull request introduces the implementation of an onboarding experience for new users of the Brave browser. It is focused on highlighting the Brave Shields feature, which blocks trackers and ads. Onboarding includes a help bubble that provides users with contextual information about the number of trackers blocked, including those from specific companies like Google, Amazon, and Facebook.

Changes

browser/brave_local_state_prefs.cc and browser/brave_tab_helpers.cc

  • Added conditional preprocessor blocks for TOOLKIT_VIEWS to include the new onboarding helper header and to register onboarding preferences within the local state preferences.

browser/onboarding/BUILD.gn

  • Created a new build file for the onboarding unit tests.

browser/onboarding/domain_map.*

  • Implemented a domain map for company tracking domains to human-readable company names.

browser/onboarding/onboarding_tab_helper.*

  • Created an OnboardingTabHelper which attaches to the WebContents and performs checks to decide whether to show the help bubble for Brave Shields.

browser/onboarding/onboarding_unittest.cc

  • Introduced unit tests for the OnboardingTabHelper functionality and the domain map logic.

browser/onboarding/pref_names.h

  • Added a preference name for tracking the last time the Brave Shields onboarding help bubble was highlighted.

browser/onboarding/sources.gni

  • Included sources and dependencies to be built for the onboarding feature.

browser/sources.gni

  • Updated to integrate the onboarding sources into the browser build.

browser/ui/BUILD.gn

  • Added shields action view and help bubble view UI components to the UI source set.

browser/ui/brave_browser_window.*

  • Extended the BraveBrowserWindow class interface to include a method for showing the brave help bubble view.

browser/ui/views/brave_actions/brave_shields_action_view.*

  • Extended the BraveShieldsActionView to include properties for the shields action icon, facilitating element identification for the help bubble.

browser/ui/views/brave_help_bubble/*

  • Created UI components (BraveHelpBubbleDelegateView and BraveHelpBubbleHostView) for showing help bubbles related to the onboarding experience.

browser/ui/views/frame/brave_browser_view.*

  • Integrated the help bubble host view within the browser view, showing and hiding the bubble based on the active tab change.

components/resources/brave_shields_strings.grdp

  • Included new string IDs for labels used in the onboarding help bubble.

test/BUILD.gn

  • Updated test build configuration to include the onboarding unit tests.

Security Hotspots

  • browser/onboarding/domain_map.cc: Ensure that the domain map contains accurate information and prevents any possible spoofing of domain names.
  • browser/ui/views/brave_help_bubble/brave_help_bubble_delegate_view.cc: Secure the painting of UI elements and avoid any buffer overflow with Skia painting operations.
  • browser/ui/views/frame/brave_browser_view.cc: Double-check visibility logic to ensure that the help bubble only displays under appropriate conditions, preventing UI redressing attacks.

Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

It's looking really nice!

I did detect one issue though - the "tooltip" is showing above all other OS windows. Is there a way to fix that? I think if the user doesn't dismiss it immediately, or switches app before the page finishes loading, then it becomes confusing that it pops up without being attached at the same z-index as its browser window.

image

Other than that, looking good:
image

image

@@ -201,4 +201,20 @@
<message name="IDS_BRAVE_SHIELDS_NOT_VALID_ADDRESS" desc="Label for link about invalid pattern for content setting page dialog to add a site">
Wildcards are not allowed for Brave Shields.
</message>

<message name="IDS_BRAVE_SHIELDS_ONBOARDING_LABEL_WITH_COMPANIES" desc="Label for the onboarding bubble UI, informing users about Brave Shields blocking company-specific trackers on a website, along with the number of additional trackers blocked.">
Brave Shields blocked <ph name="COMPANY_LIST">$1</ph> and <ph name="BLOCKED_COUNT">$2</ph> other trackers on <ph name="SITE_URL">$3</ph>
Copy link
Member

Choose a reason for hiding this comment

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

nit: ideally we would use the pluralisation helper to make sure we say

"and 1 other tracker"
"and 2 other trackers"

</message>

<message name="IDS_BRAVE_SHIELDS_ONBOARDING_LABEL_WITH_ONLY_COMPANIES" desc="Label for the onboarding bubble UI, informing users about Brave Shields blocking company-specific trackers only on a website.">
Brave Shields blocked <ph name="COMPANY_LIST">$1</ph> trackers on <ph name="SITE_URL">$2</ph>
Copy link
Member

Choose a reason for hiding this comment

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

same nit

* Fixed help bubble is shown over the another app
* Plural formatting for the number of trackers
Copy link
Contributor

github-actions bot commented Dec 7, 2023

[puLL-Merge] - brave/brave-core@15198

Description

The provided PR introduces onboarding features related to the Brave Shields functionality. It seems focused on providing help and guidance to new users of the Brave browser, emphasizing the utility and functioning of Brave Shields, which is a privacy feature of the browser that blocks trackers and ads.

Changes

browser/brave_local_state_prefs.cc

  • Registers the onboarding preferences associated with the Brave browser's local state preferences.

browser/brave_tab_helpers.cc

  • Attaches onboarding helper to web contents.

browser/onboarding/BUILD.gn

  • New unit tests added for onboarding features.

browser/onboarding/domain_map.cc and .h

  • New domain mapping implementation for onboarding, mapping ad domains to company names.

browser/onboarding/onboarding_tab_helper.cc and .h

  • New tab helper for onboarding, potentially creating helper instances for web contents when conditions are met (e.g., 7 days since first run).

browser/onboarding/pref_names.h

  • New Brave onboarding preference names file.

browser/onboarding/sources.gni

  • New source set and dependencies for onboarding.

browser/ui/BUILD.gn

  • Modifies UI build script to include new Brave help bubble view.

browser/ui/brave_browser_window.[cc|h]

  • Expands class with method to show Brave help bubble view.

browser/ui/views/brave_actions/brave_shields_action_view.[cc|h]

  • Modifies shield action view to facilitate the display of the onboarding bubble.

browser/ui/views/brave_help_bubble/brave_help_bubble_delegate_view.[cc|h]

  • New delegate view for Brave help bubble.

browser/ui/views/brave_help_bubble/brave_help_bubble_host_view.[cc|h]

  • New host view for Brave help bubble.

browser/ui/views/frame/brave_browser_view.[cc|h]

  • Extends browser view with methods to handle showing of Brave help bubble view, and updating associated positions.

components/resources/brave_shields_strings.grdp

  • New localization IDs for strings related to the onboarding bubble.

test/BUILD.gn

  • Updates testing build gn file to include onboarding unit tests.

Security Hotspots

  1. browser/onboarding/domain_map.cc (GetCompanyNameFromGURL function): Ensure proper validation of URLs and defensive coding against potential manipulation of the domain for security purposes.
  2. OnboardingTabHelper (Various places where web contents are managed): As the tab helper interacts with web content, it should be audited to ensure that no cross-site scripting (XSS) vulnerabilities or unintended interactions can occur.
  3. BraveHelpBubbleDelegateView: Any new view handling user interaction should be reviewed for proper event and visibility management to prevent clickjacking or UI redressing attacks.
  4. Help bubble interactions and animations: Care should be taken to ensure that animation or UI display logic does not inadvertently expose private user data or allow for interaction that can be abused in phishing attacks.

@simonhong
Copy link
Member

It's looking really nice!

I did detect one issue though - the "tooltip" is showing above all other OS windows. Is there a way to fix that? I think if the user doesn't dismiss it immediately, or switches app before the page finishes loading, then it becomes confusing that it pops up without being attached at the same z-index as its browser window.

image

Other than that, looking good: image

image

All resolved.
Plural formating the number of trackers.
Screenshot 2023-12-07 at 10 11 58 AM
Screenshot 2023-12-07 at 10 26 01 AM
Bubble is shown behind another app.
Screenshot 2023-12-07 at 10 17 01 AM

@simonhong simonhong merged commit 8534c2c into master Dec 7, 2023
@simonhong simonhong deleted the views/pulse branch December 7, 2023 03:22
@github-actions github-actions bot added the potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false label Dec 7, 2023
@github-actions github-actions bot added this to the 1.63.x - Nightly milestone Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-security-review potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false puLL-Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add highlight to Shields button during onboarding
8 participants