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

[iOS] Search Suggestions Enhancement #24376

Merged
merged 9 commits into from
Aug 14, 2024

Conversation

soner-yuksel
Copy link
Contributor

@soner-yuksel soner-yuksel commented Jun 25, 2024

Resolves brave/brave-browser#38137

Security Ticket: https://github.com/brave/reviews/issues/1567 to evaluate the spec
https://github.com/brave/reviews/issues/1672 to evaluate the implementation iOS

The main focus:

This PR is aiming to enhance the user experience especially for the first time users for search suggestions. Some users are confused about enabling search suggestions and considering Brave doesn't have search suggestions because they can easily miss prompt for enabling it or forget about disabling it and don't know where to enable inside setting screens later.

To improve this, users from Brave Search Default regions have their search suggestions enabled by default. (They can disable from settings etc, that behaviour is not changed)

The change can be seen at InitialSearchEngine and SearchEngine classes along with the tests for it inside InitialSearchEngineTests.

To enabled this behaviour the additional changes were required

and the tests from https://github.com/brave/search/blob/a68168a840980b219c4ed9ea4cc45f3550ea6296/services/mixer/serpkit/tests/lib/utils/hw.test.ts#L95 are also added to prove the converted function acts as desired.

This conversion also included the conversion regex phrases to be used in swift with rules like

-Use a "raw" string literal to avoid double escaping backslashes that form regex escapes (you have \1 backreference and . regex escape in the pattern), so use let format = #"..."#

-Escape square brackets inside character classes in a regex pattern because square brackets "[" and "]" are both special (they are used to create character class intersections and unions).

Also some quality of life changes are made to SearchEngines class.

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:

  • Change Region to one of the Brave Search enabled default countries like Canada or USA
  • Fresh Install Brave
  • Click on URL Bar and check the search suggestion enable-disable table section is not there (it should be enabled by default)
  • Using keyboard type test and check search suggestions are displayed and delete the entry
  • Copy paste "test query" and check search suggestions are displayed (this is one of the safe queries)
  • Copy paste "test query \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t" and check search queries are not added( also this is one of the suspicious queries in the list)

Screenshots-Videos:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-06-26.at.14.37.51.mp4

@soner-yuksel soner-yuksel added the CI/skip Do not run CI builds (except noplatform) label Jun 25, 2024
@soner-yuksel soner-yuksel self-assigned this Jun 25, 2024
@soner-yuksel soner-yuksel added CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 CI/skip-macos-arm64 Do not run CI builds for macOS arm64 and removed CI/skip Do not run CI builds (except noplatform) labels Jun 26, 2024
@soner-yuksel soner-yuksel marked this pull request as ready for review June 26, 2024 19:06
@soner-yuksel soner-yuksel requested a review from a team as a code owner June 26, 2024 19:06
@soner-yuksel soner-yuksel force-pushed the ios/enhancement/search-suggestions branch 3 times, most recently from e32662c to 1944047 Compare June 26, 2024 19:50
@soner-yuksel soner-yuksel force-pushed the ios/enhancement/search-suggestions branch from 1944047 to 5a47cf7 Compare July 2, 2024 14:40
Copy link
Contributor

@iccub iccub left a comment

Choose a reason for hiding this comment

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

lets wait for sec/priv review

@soner-yuksel soner-yuksel force-pushed the ios/enhancement/search-suggestions branch from 399c737 to 69ebf35 Compare July 10, 2024 17:05
@brave-builds
Copy link
Collaborator

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

@soner-yuksel soner-yuksel force-pushed the ios/enhancement/search-suggestions branch 3 times, most recently from 7595442 to b3e0c37 Compare July 10, 2024 18:19
@soner-yuksel soner-yuksel force-pushed the ios/enhancement/search-suggestions branch 3 times, most recently from 1cd62f3 to 335f688 Compare July 22, 2024 18:03
@soner-yuksel soner-yuksel force-pushed the ios/enhancement/search-suggestions branch from 335f688 to b3f5eec Compare August 7, 2024 17:17
@soner-yuksel soner-yuksel force-pushed the ios/enhancement/search-suggestions branch from b3f5eec to 65c4e09 Compare August 13, 2024 20:52
Copy link
Contributor

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

Description

This PR introduces changes to improve search functionality and security in the Brave iOS app. It adds a new URLBarHelper class to detect suspicious search queries, updates the search suggestion behavior, and modifies how default search engines are determined based on region.

Possible Issues

  • The new URLBarHelper functionality may potentially block legitimate search queries if the detection algorithm is too aggressive.
  • Changes to default search engine behavior could affect user experience in certain regions.

Security Hotspots

  1. URLBarHelper.swift: The new isSuspiciousQuery function processes user input to detect potentially sensitive information. Ensure it doesn't introduce any security vulnerabilities or performance issues.

  2. AppDelegate.swift: New logic for enabling search suggestions based on region could potentially expose user location information if not handled carefully.

Changes

Changes

  1. ios/brave-ios/App/iOS/Delegates/AppDelegate.swift

    • Added logic to enable search suggestions for BraveSearch default countries
    • Set preferences for showing search suggestions opt-in based on region
  2. ios/brave-ios/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+ToolbarDelegate.swift

    • Updated setSearchQuery to include a check for showing search suggestions
  3. ios/brave-ios/Sources/Brave/Frontend/Browser/Search/InitialSearchEngines.swift

    • Added isBraveSearchDefaultRegion property
    • Updated region-specific search engine configurations
  4. ios/brave-ios/Sources/Brave/Frontend/Browser/Search/SearchEngines.swift

    • Added isBraveSearchDefaultRegion property
  5. ios/brave-ios/Sources/Brave/Frontend/Browser/Search/SearchViewController.swift

    • Modified setSearchQuery to conditionally query suggestions based on input
  6. ios/brave-ios/Sources/Brave/Frontend/Browser/TabManager.swift

    • Removed tabEventHandlers property
  7. ios/brave-ios/Sources/Brave/Frontend/Browser/Toolbars/UrlBar/TopToolbarView.swift

    • Added locationLastReplacement property
  8. ios/brave-ios/Sources/Brave/Frontend/ClientPreferences.swift

    • Made search-related preferences public
  9. ios/brave-ios/Sources/Brave/Frontend/Widgets/AutocompleteTextField.swift

    • Made lastReplacement property public
  10. ios/brave-ios/Sources/Brave/Helpers/TabEventHandlers.swift

    • Deleted file
  11. ios/brave-ios/Sources/Brave/Helpers/URLBarHelper.swift and URLBarHelperConstants.swift

    • Added new files for URLBarHelper functionality
  12. ios/brave-ios/Tests/ClientTests/Helpers/URLBarHelperTests.swift

    • Added new test file for URLBarHelper
  13. ios/brave-ios/Tests/ClientTests/InitialSearchEnginesTests.swift

    • Added tests for BraveSearch default region functionality

The PR also includes some file moves and renames in the test directory.

@brave-builds
Copy link
Collaborator

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

@kjozwiak
Copy link
Member

Verification PASSED on iPhone 11 running iOS 17.5.1 using the following build(s):

Brave | 1.71.33 Chromium: 128.0.6613.85 (Official Build) nightly (64-bit) 
--- | ---
Revision | f1416b4a9837...
OS | iOS

Region with Brave SE as Default (Canada)

Using the STR/Cases outlined via #24376 (comment), ensured the following:

  • ensured that the Enable Search Suggestions modal isn't being displayed when typing into the omnibox
  • ensured that the search suggestions are being displayed without any issues/failures
  • ensured that typing in test correctly displayed the search suggestions as mentioned/displayed above
  • ensured that pasting test query into the omnibox correctly displayed the search suggestions as mentioned/displayed above
  • ensured that pasting test query \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t \t into the omnibox didn't display any search suggestions due to the query being flagged as suspicious
RPReplay_Final1724723589.MP4

Region without Brave Search as Default (Poland)

  • ensured that Search Suggestions is disabled by default via Search Settings for regions that don't have Brave SE as default
  • ensured that enabling search suggestions via the omnibox enables the feature
    • ensured that it appears enabled under Search Settings
Example Example
IMG_0497 IMG_0498

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-macos-arm64 Do not run CI builds for macOS arm64 CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 needs-security-review puLL-Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search Suggestions Enhancement
6 participants