brave / brave-browser

Brave browser for Android, iOS, Linux, macOS, Windows.
https://brave.com
Mozilla Public License 2.0
17.63k stars 2.3k forks source link

Improve clarity in search engine settings UI #39167

Open libertyteeth opened 3 months ago

libertyteeth commented 3 months ago

Description

The current UI for adding search engines in Brave is confusing. The "Site search" section appears separate from the main search engine list, leading to confusion about where to add custom search engines. Enhancing clarity (or merging these sections?) could improve user experience.

Steps to reproduce

  1. Go to brave://settings/searchEngines
  2. Try to add a new search engine.
  3. There are two separate sections, which are labeled "Search engines" and "Site search".
  4. There is only an "Add" button in the second section.

Actual result

I wanted to add Yandex. See screenshot (which is of my completed task, i.e., includes Yandex).

I tried adding it to the second section but it "looked wrong" e.g., others started with "@" and this started with ":" like those in the first list.

I didn't click the hamburger menu to see that I could add it as default, until after several minutes of exploration, with ChatGPT's help.

Brave search engine - Screenshot from 2024-06-19 12-17-22

Expected result

The change can be either UI, or functionality.

Either:

  1. There should be instructions saying "Add a new search engine to the second list, use the hamburger menu [or other description] and select 'Make default' to move it to the 'Search engines' list."

Or:

  1. There should be a button to add a search engine to the top list, the "Search engines" list.

Reproduces how often

Easily reproduced

Brave version (brave://version info)

1.67.116 Chromium: 126.0.6478.71 (Official Build) (64-bit)

Channel information

Reproducibility

Miscellaneous information

ChatGPT also got confused trying to assist. So the documentation on the Internet also seems it can use improvement, but I don't have a list of what it searched in an attempt to assist. Here's the chat where I asked for help, if this can help add value to the UI:

https://chatgpt.com/share/88a43edd-8652-4cde-946d-33a2ade5913a

ajayseeker commented 3 months ago

Hi @rebron, @libertyteeth and @sangwoo108 Can I explore this issue?

We can probably relocate the Add button in case of Site Search and insert an Add button for search engines section.

ajayseeker commented 3 months ago

Hi @sangwoo108 Looking at the code, I see that there is no functionality to add to the default search engines list. By Default Search Engines List I mean following list image

However, there is functionality to add to the Site Search list, i.e. the following list image

In case, this is intensional, moving the Add button below the Site search list should suffice. Like in the following UI. image

Does the above mentioned change looks ok? or Should we add an "Add" button below the Default Search Engines list and also add the functionality to add to the list?

sangwoo108 commented 3 months ago

Default Search Engines list and also add the functionality to add to the list?

I believe you got it right. Default search engine list isn't customizable as far as I know. So, I think

cc. @rebron

ajayseeker commented 3 months ago

Hi @sangwoo108, I've couple of questions:

  1. The Add button change is on following page chrome/browser/resources/settings/search_engines_page/search_engines_page.html, but AFAIU, we avoid changing chromium's code. For cpp code, we could overide classes in brave part of code. How do we go about in html? Also, I don't see any brave_search_engine_page.html file.
  2. Regarding adding more instructions, I'm not sure if it is needed. Boxed line in following image already mentions the steps. What are your thoughts? image
sangwoo108 commented 2 months ago

Hi @ajayseeker , I think you can try adding search_engine.ts file to src/brave/browser/resources/settings/brave_overrides directory to override.

ajayseeker commented 2 months ago

Hi @sangwoo108

src/brave/browser/resources/settings/brave_overrides/README.md

README.md file mentions that for overriding we need to perform three steps:

  1. Create .js file in brave_overrides folder. Override template, style etc by using necessary methods.
  2. Update index.js with file path
  3. Create an entry in settings_resources.grd

I observe that settings_resources.grd file is missing from the master. Also, post following (1) and (2), running build complains stating unable to find the .js file updated in index.js. Is there a change in method to override? or Have I missed something significant? Can you please comment on this?

sangwoo108 commented 2 months ago

@fallaciousreasoning Could you give some advice on this if you don't mind? https://github.com/brave/brave-browser/issues/39167#issuecomment-2223311310

fallaciousreasoning commented 2 months ago

The missing settings_resources.grd sounds like maybe you didn't run the build steps properly:

  1. Clone git clone <repo> into ~/dev/brave-browser/src/brave (it's super important that you clone into brave-browser/src/brave folder because we need to clone Chromium into the src dir and depot_tools into brave-browser.
  2. Install packages (in brave-browser/src/brave) npm i - Make sure you're using node 21 not 22 (which breaks)
  3. Init the repo npm run init
  4. Sync the repo (you should do this every time you fetch) npm run sync
  5. Build the repo npm run build
  6. Start the browser npm run start

https://github.com/brave/brave-browser?tab=readme-ov-file#clone-and-initialize-the-repo

ajayseeker commented 2 months ago

The missing settings_resources.grd sounds like maybe you didn't run the build steps properly:

  1. Clone git clone <repo> into ~/dev/brave-browser/src/brave (it's super important that you clone into brave-browser/src/brave folder because we need to clone Chromium into the src dir and depot_tools into brave-browser.
  2. Install packages (in brave-browser/src/brave) npm i - Make sure you're using node 21 not 22 (which breaks)
  3. Init the repo npm run init
  4. Sync the repo (you should do this every time you fetch) npm run sync
  5. Build the repo npm run build
  6. Start the browser npm run start

https://github.com/brave/brave-browser?tab=readme-ov-file#clone-and-initialize-the-repo

Hi @sangwoo108 and @fallaciousreasoning Sorry, for such a late revert, got busy with work. @fallaciousreasoning sure, I'll do as you suggested.

However, I've a clarifying question, does browser/resources/settings/settings_resources.grd gets generated during build process. I ask this because, if you observe this CL, changes are being made to settings_resources.grd. But, the file does exist in Brave_Core repo at path (brave_core/browser/resources/settings)

fallaciousreasoning commented 2 months ago

That PR was from 4 years ago, so I think we must have changed the way Chromium overrides work since then - sorry looking a bit closer I think settings_resources.grd is a red herring.

Try:

  1. Create a search_engine.ts file in src/brave/browser/resources/settings/brave_overrides
  2. Add ./search_engine.ts to src/brave/browser/resources/settings/brave_overrides.ts
  3. Add "search_engine.ts" to the brave_settings_non_web_component_files array in src/brave/browser/resources/settings/sources.gni

You file should be loaded on the settings page! If you look at the other files in the brave_overrides directory it should give you some idea about how to override the upstream WebComponents :smile:

ajayseeker commented 1 month ago

Hi @fallaciousreasoning and @sangwoo108! I've raised a PR . Please review it and share your feedback. I've moved the button below the Active Search Engines list.