Implement Gradient background for NTP #14783

sangwoo108 · 2022-08-23T23:14:39Z

Reuse most of components used for Solid color background

Resolves brave/brave-browser#24889

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:

fallaciousreasoning

This mostly LGTM. I have one question though - could we get away without adding a new background type to the backend? Maybe instead of having separate UseSolidColorBackground and UseGradientColorBackground we could just have a UseColorBackground which stores anything which can be assigned to background in the CSS?

We could still have a separate gradient and solid section in the UI but they'd write to the same pref. What do you think?

Thinking about it we might even be able to set it to url('data:...') but that might not work in WebUI.

It'd just reduce the number of branches we have to worry about here 😆

components/brave_new_tab_ui/apiEventsToStore.ts

components/brave_new_tab_ui/stories/default/data/backgroundWallpaper.ts

* Reuse most of components used for Solid color background

sangwoo108

Could we get away without adding a new background type to the backend?

Yeah, that sounds good! Let me try again :)

components/brave_new_tab_ui/stories/default/data/backgroundWallpaper.ts

components/brave_new_tab_ui/apiEventsToStore.ts

simonhong

++ 👍🏼

browser/ui/webui/new_tab_page/brave_new_tab_page_handler.cc

sangwoo108 · 2022-08-24T01:32:41Z

One thing I'm concerned about storing css value into pref is if it could be a vulnerability for attackers to change NTP background or inject their css our NTP background - I know almost nothing about security, though. If so, we might have to keep the backend types as is, and store pref only when it's compliant with the format.

components/brave_new_tab_ui/data/colors.ts

fallaciousreasoning · 2022-08-24T02:57:01Z

Oh yeah, good call! I didn't think of that

petemill · 2022-08-24T04:37:57Z

One thing I'm concerned about storing css value into pref is if it could be a vulnerability for attackers to change NTP background or inject their css our NTP background - I know almost nothing about security, though. If so, we might have to keep the backend types as is, and store pref only when it's compliant with the format.

How about instead storing the index of the gradient in a fixed array, as a key? We can do the same for the background color. It seems strange to me to store the full code values in pref, which we can do simply because css is a dynamic language (sort of). Storing index would help with e.g. android and iOS using the same pref value.

edit: or if we're worried about using indexes because we may want to insert new items in the middle, then assigning a key to each value, e.g.:

const backgroundValues = { 
  red1: '#3C790B', 
  gradientA: 'linear-gradient(125.83deg, #392DD1 0%, #A91B78 99.09%)'
}

I'm happy if you want to combine colors and gradients, or keep them separate, internally.

sangwoo108 · 2022-08-24T04:56:55Z

How about instead storing the index of the gradient in a fixed array, as a key? ... if we're worried about using indexes because we may want to insert new items in the middle,

Yeah, from my experience, when we store index of a preset, we have migration issue in case we change the preset(add, remove, reorder, etc ...).

To give a name to each value and store the name sounds cool. I've used "https://colornamer.robertcooper.me/" once to name colors, I think we can use this this time too.

sangwoo108 · 2022-08-24T05:41:59Z

Update: security team confirmed that it's okay as is. I'd like to refactor as @fallaciousreasoning suggested :) @petemill Please let me know if you think storing key instead of value is desirable.

brave-builds · 2022-08-24T07:01:14Z

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

We don't have to keep both of them. They're not that diffrent.

sangwoo108 · 2022-08-24T10:28:37Z

@simonhong @fallaciousreasoning @petemill Merged two prefs and paths. Could you check this again, please?

brave-builds · 2022-08-24T12:46:30Z

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

fallaciousreasoning

Still LGTM

browser/ntp_background/ntp_background_prefs.cc

components/brave_new_tab_ui/components/default/page/index.tsx

components/brave_new_tab_ui/containers/newTab/settings/backgroundImage.tsx

simonhong · 2022-08-25T04:29:38Z

components/ntp_background_images/browser/ntp_custom_background_images_service.h

@@ -20,15 +20,18 @@ class GURL;

 namespace ntp_background_images {

+// TODO(sko) Rename this to NTPCustomBackgroundService. It's dealing with


FYI: Upstream has that class already :)

Oh, dear.. Let me think about better name

components/brave_new_tab_ui/brave_new_tab_page.mojom

brave-builds · 2022-08-25T06:45:45Z

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

sangwoo108 · 2022-08-25T08:34:42Z

Thank you all for reviewing this! It was really helpful, as always :)

GeetaSarvadnya · 2022-09-14T02:55:08Z

Verification PASSED on

Brave | 1.45.44 Chromium: 105.0.5195.102 (Official Build) nightly (64-bit)
-- | --
Revision | 4c16f5ffcc2da70ee2600d5db77bed423ac03a5a-refs/branch-heads/5195_55@{#4}
OS | Windows 10 Version 21H2 (Build 19044.1889)

Verified the description from the issue
Logged Bandwidth stats is not shown clearly when blue Gradients/Solid background color is selected brave-browser#25392
Logged Trackers & ads blocked stats is not shown clearly in NTP when a Gradient/Solid color is selected brave-browser#25393

1. Gradients

Confirmed that user can select the gradient background for NTP
Confirmed that user can choose the induvidual gradient background via NTP
Confirmed that same gradient background is shown on each the NTP when the induvidual gradient background is selected
Confirmed that the differenet gradient background is shown on each the NTP when Refresh on every new tab switch is ON
Confirmed that on each gradient background color, the NTP stats (Cards, clock, Top sites, Brave stats etc) are shown clearly/correctly
Confirmed that selected gradient background retained on the NTP after the browser restart
Confirmed that Check mark is shown on the selected gradient bg color
Confirmed that selected gradient background retained across all the tabs and windows in Regular window
Confirmed that selected gradient background retained only for the Profile 1
Confirmed that user has provision to select the different gradient background colors for multiple profiles
Confirmed that selecting gradient background doesn't impact the NTP SI show on NTP
Confirmed that NTP SI are shown in the order 2, 4, 8 tabs

Example	Example	Example	Example	Example

2. Solid colors

Confirmed that user can select the solid background for NTP
Confirmed that user can choose the induvidual solid background via NTP
Confirmed that same solid background is shown on each the NTP when the induvidual solid background is selected
Confirmed that the differenet solid background is shown on each the NTP when Refresh on every new tab switch is ON
Confirmed that on each solid background color, the NTP stats (Cards, clock, Top sites, Brave stats etc) are shown clearly/correctly
Confirmed that selected solid background retained on the NTP after the browser restart
Confirmed that Check mark is shown on the selected solid bg color
Confirmed that selected solid background retained across all the tabs and windows in Regular window
Confirmed that selected solid background retained only for the Profile 1
Confirmed that user has provision to select the different solid background colors for multiple profiles
Confirmed that selecting solid background doesn't impact the NTP SI show on NTP
Confirmed that NTP SI are shown in the order 2, 4, 8 tabs

Example	Example	Example	Example	Example

Regression on NTP

Ensured Tops sites can be edit/delete via NTP
Ensured that NTP stats can be hidden/shown
Ensured that a site can be added via NTP
Ensured NTP Widget cards can be added/removed
Ensured NTP SI works as expected
Ensured that click on settings (bookmarks, history, settings and brave talk) shortcuts works as expected
Ensured that the existing features upload your own and Brave background works as expected

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Aug 23, 2022

github-actions bot assigned sangwoo108 Aug 23, 2022

sangwoo108 requested review from fallaciousreasoning and petemill and removed request for fallaciousreasoning August 23, 2022 23:17

sangwoo108 added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS labels Aug 23, 2022

sangwoo108 requested a review from simonhong August 23, 2022 23:18

fallaciousreasoning approved these changes Aug 23, 2022

View reviewed changes

components/brave_new_tab_ui/apiEventsToStore.ts Outdated Show resolved Hide resolved

components/brave_new_tab_ui/stories/default/data/backgroundWallpaper.ts Outdated Show resolved Hide resolved

sangwoo108 added 2 commits August 24, 2022 09:32

Implement Gradient background for NTP

2220b4c

* Reuse most of components used for Solid color background

Address code review

f6ba210

sangwoo108 force-pushed the sko/gradient branch from 63bf7a0 to f6ba210 Compare August 24, 2022 01:06

sangwoo108 commented Aug 24, 2022

View reviewed changes

components/brave_new_tab_ui/stories/default/data/backgroundWallpaper.ts Outdated Show resolved Hide resolved

components/brave_new_tab_ui/apiEventsToStore.ts Outdated Show resolved Hide resolved

simonhong approved these changes Aug 24, 2022

View reviewed changes

browser/ui/webui/new_tab_page/brave_new_tab_page_handler.cc Outdated Show resolved Hide resolved

sangwoo108 commented Aug 24, 2022

View reviewed changes

components/brave_new_tab_ui/data/colors.ts Show resolved Hide resolved

Rename OnCustomBackgroundImageUpdated() => OnCustomBackgroundUpdated()

83e59bf

Merge two separate path into one

d7aa2fd

We don't have to keep both of them. They're not that diffrent.

sangwoo108 requested review from simonhong and fallaciousreasoning August 24, 2022 10:27

fallaciousreasoning reviewed Aug 24, 2022

View reviewed changes

fallaciousreasoning approved these changes Aug 24, 2022

View reviewed changes

Clean ups

c131d6e

simonhong reviewed Aug 25, 2022

View reviewed changes

components/brave_new_tab_ui/brave_new_tab_page.mojom Outdated Show resolved Hide resolved

Remove unused mojo constant

8ca0f4c

simonhong approved these changes Aug 25, 2022

View reviewed changes

Remove unnecessary assertion

66bd266

sangwoo108 merged commit 31f1b86 into master Aug 25, 2022

sangwoo108 deleted the sko/gradient branch August 25, 2022 08:34

github-actions bot added this to the 1.45.x - Nightly milestone Aug 25, 2022

sangwoo108 restored the sko/gradient branch September 2, 2022 05:21

sangwoo108 mentioned this pull request Sep 2, 2022

Implement Gradient background for NTP (uplift to 1.44.x) #14960

Closed

25 tasks

This was referenced Sep 14, 2022

Bandwidth stats is not shown clearly when blue Gradients/Solid background color is selected brave/brave-browser#25392

Closed

Trackers & ads blocked stats is not shown clearly in NTP when a Gradient/Solid color is selected brave/brave-browser#25393

Closed

sangwoo108 deleted the sko/gradient branch December 14, 2023 01:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Gradient background for NTP #14783

Implement Gradient background for NTP #14783

sangwoo108 commented Aug 23, 2022

fallaciousreasoning left a comment

sangwoo108 left a comment

simonhong left a comment

sangwoo108 commented Aug 24, 2022 •

edited

Loading

fallaciousreasoning commented Aug 24, 2022

petemill commented Aug 24, 2022 •

edited

Loading

sangwoo108 commented Aug 24, 2022

sangwoo108 commented Aug 24, 2022

brave-builds commented Aug 24, 2022

sangwoo108 commented Aug 24, 2022

brave-builds commented Aug 24, 2022

fallaciousreasoning left a comment

simonhong Aug 25, 2022

sangwoo108 Aug 25, 2022

brave-builds commented Aug 25, 2022

sangwoo108 commented Aug 25, 2022 •

edited

Loading

GeetaSarvadnya commented Sep 14, 2022 •

edited

Loading

		@@ -20,15 +20,18 @@ class GURL;

		namespace ntp_background_images {

		// TODO(sko) Rename this to NTPCustomBackgroundService. It's dealing with

Implement Gradient background for NTP #14783

Implement Gradient background for NTP #14783

Conversation

sangwoo108 commented Aug 23, 2022

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

fallaciousreasoning left a comment

Choose a reason for hiding this comment

sangwoo108 left a comment

Choose a reason for hiding this comment

simonhong left a comment

Choose a reason for hiding this comment

sangwoo108 commented Aug 24, 2022 • edited Loading

fallaciousreasoning commented Aug 24, 2022

petemill commented Aug 24, 2022 • edited Loading

sangwoo108 commented Aug 24, 2022

sangwoo108 commented Aug 24, 2022

brave-builds commented Aug 24, 2022

sangwoo108 commented Aug 24, 2022

brave-builds commented Aug 24, 2022

fallaciousreasoning left a comment

Choose a reason for hiding this comment

simonhong Aug 25, 2022

Choose a reason for hiding this comment

sangwoo108 Aug 25, 2022

Choose a reason for hiding this comment

brave-builds commented Aug 25, 2022

sangwoo108 commented Aug 25, 2022 • edited Loading

GeetaSarvadnya commented Sep 14, 2022 • edited Loading

sangwoo108 commented Aug 24, 2022 •

edited

Loading

petemill commented Aug 24, 2022 •

edited

Loading

sangwoo108 commented Aug 25, 2022 •

edited

Loading

GeetaSarvadnya commented Sep 14, 2022 •

edited

Loading