Improve tab contrast #6912

simonhong · 2020-10-21T09:09:12Z

Resolves brave/brave-browser#2735
Resolves brave/brave-browser#8807

dark mode window

private window

tor window

Profile menu bubbles

Submitter Checklist:

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

simonhong · 2020-11-16T08:43:50Z

@karenkliu PTAL updated colors with below dmg (nightly).
All colors are updated except custom graphic on title bar bg. (custom graphic is in-progress)
I didn't change hover colors of omnibox because brave/brave-browser#2735 doesn't have new hover colors.
Please check hover colors.
https://drive.google.com/file/d/1Ycapkk2pfsApvl_41UUBLevrSLGDQCGA/view?usp=sharing

karenkliu · 2020-11-16T19:53:35Z

@simonhong Thanks for the update! I found a few minor issues -

The browser profile dropdown menu should have some updated text; it's not using the correct name for our windows:

The hover colors of omnibox for Private Window and Private Window with Tor look fine to me. But let's change the normal window (dark theme) omnibox hover color to #23252F so it matches the other windows better:

karenkliu · 2020-11-17T17:50:18Z

@simonhong Sorry - I noticed there was a typo in my earlier comment. For the Private Window browser profile menu, it should say "Exit Private Window" as the option. I've updated it the image.

simonhong · 2020-11-18T01:53:48Z

@karenkliu All specs are applied. Will share new dmg after building.

simonhong · 2020-11-18T02:48:39Z

@karenkliu Uploaded new dmg file here https://drive.google.com/file/d/1F5PNexZJ5VQxoixSqdLLaF8hoLHKpPr6/view?usp=sharing PTAL again :)

karenkliu

Looks great! 👏 👏 👏 Nice job!

petemill

Code looks good. One minor text change from reading it. Just waiting for a build and then will review further, but for now this is only feedback. Nice work!

petemill · 2020-11-18T20:09:59Z

app/brave_generated_resources.grd

+        Private with Tor
+      </message>
+      <message name="IDS_TOR_AVATAR_BUTTON_TOOLTIP_TEXT" desc="The tooltip text of tor avatar button.">
+        This is private window with tor


Missing " a" and capitalization of "Tor":

This is a private window with Tor

Looks like it still has the old spelling?

@bsclifton Oops, something happened while rebasing. fixed again. Thanks for checking!

petemill · 2020-11-18T20:20:40Z

app/theme/brave_theme_resources.grd

@@ -22,6 +22,7 @@
      <structure type="chrome_scaled_image" name="IDR_PRODUCT_LOGO_32_DEV" file="brave/product_logo_32_dev.png" />
      <structure type="chrome_scaled_image" name="IDR_PRODUCT_LOGO_32_DEVELOPMENT" file="brave/product_logo_32_development.png" />
      <if expr="not is_android">
+        <structure type="chrome_scaled_image" name="IDR_TOR_WINDOW_FRAME_GRAPHIC" file="brave/tor_window_frame_graphic.png" />


Out of interest - did you try a scalable .icon instead of PNG?

I didn't try because .icon only works well for icon type(square) and mono color.

bsclifton · 2020-11-19T19:29:57Z

Might be Windows only - but it looks like Private Windows and Private Window with Tor are the same color? (for the window chrome; not the tab contents) Definitely doesn't show the contrast shown in your screenshots above

Easier to see screenshot

simonhong · 2020-11-19T23:13:57Z

@bsclifton Hmm, it's strange. coloring is cross-platform implementation and it seems fine on my local.
I assume it depends on monitor specification?

simonhong · 2020-11-19T23:52:54Z

@bsclifton I'll try rebuild and double check again. I rebased but maybe recent IsTorProfile() deprecation could affect.
My above screenshot is from the image before rebasing. (~~it was ok on macOS after rebasing~~)

simonhong · 2020-11-20T06:34:11Z

@bsclifton Tor can have different theme colors again. PTAL :)

bsclifton · 2020-11-20T16:57:49Z

There we go - much better! Confirmed colors are working as expected now 😄👍 Will check out the rest now...

bsclifton · 2020-11-20T17:12:10Z

browser/ui/views/frame/brave_glass_browser_frame_view.cc

+    bounds_to_frame_graphic.Inset(0, kFrameBorderOutlineThickness);
+    canvas->ClipRect(bounds_to_frame_graphic);
+  }
+  frame_graphic_->Paint(canvas, bounds_to_frame_graphic);


This is cool! I had no idea it would be this easy to add the graphic up there 😄 Nice work!

bsclifton · 2020-11-20T17:15:12Z

browser/ui/views/location_bar/brave_location_bar_view.cc

@@ -41,6 +41,20 @@ class BraveLocationBarViewFocusRingHighlightPathGenerator
  DISALLOW_COPY_AND_ASSIGN(BraveLocationBarViewFocusRingHighlightPathGenerator);
 };

+base::Optional<SkColor> GetFocusRingColor(Profile* profile) {


Looks perfect! nice and clean code too 😄👌

bsclifton

Tested it out- works great! Code also looks good too- minus one spelling problem. Hit me up once you can address that and I'll re-review / approve 😄 Great work here @simonhong and @karenkliu!

simonhong · 2020-11-23T07:10:24Z

Kindly ping to @brave/patch-reviewers ..

mkarolin · 2020-11-23T22:53:39Z

patches/chrome-browser-themes-theme_service_factory.cc.patch

+
+ content::BrowserContext* ThemeServiceFactory::GetBrowserContextToUse(
+     content::BrowserContext* context) const {
+  BRAVE_THEMESERVICEFACTORY_GETBROWSERCONTEXTTOUSE


Can we redefine the method in chromium_src instead?

@mkarolin Yes. I did :) PTAL.

mkarolin

chromium_src and patches changes LGTM!

As we use different theme colors for tor window(previously, same with private), we should differentiate tor and private profile from ThemeHelper. However, upstream chromium uses only two kinds of theme provider. One for orignal theme provider and the other is incognito theme provider. If we want to add another theme provider for tor, it will add many patch files. So, I think using different theme service for tor profile is simplest way to have tor specific theme colors.

simonhong · 2020-11-25T10:25:11Z

Got all green. 🎉 Merged.

bsclifton assigned simonhong Oct 23, 2020

simonhong force-pushed the tab_contrast branch from f58f0d1 to 31a449a Compare November 2, 2020 08:40

simonhong force-pushed the tab_contrast branch 3 times, most recently from 63f458e to c12d880 Compare November 16, 2020 05:56

simonhong force-pushed the tab_contrast branch 3 times, most recently from 52318a3 to 146ad12 Compare November 17, 2020 16:15

simonhong force-pushed the tab_contrast branch 3 times, most recently from 6c7b387 to 30f7399 Compare November 18, 2020 01:04

simonhong marked this pull request as ready for review November 18, 2020 01:32

simonhong requested a review from a team as a code owner November 18, 2020 01:32

simonhong force-pushed the tab_contrast branch from 81875b6 to 0824a0c Compare November 18, 2020 01:32

simonhong requested review from bsclifton, karenkliu and petemill November 18, 2020 01:33

simonhong changed the title ~~Tab contrast~~ Improve tab contrast Nov 18, 2020

karenkliu approved these changes Nov 18, 2020

View reviewed changes

petemill reviewed Nov 18, 2020

View reviewed changes

simonhong force-pushed the tab_contrast branch 2 times, most recently from 7317f5c to 622195b Compare November 19, 2020 08:41

bsclifton reviewed Nov 20, 2020

View reviewed changes

bsclifton approved these changes Nov 20, 2020

View reviewed changes

mkarolin reviewed Nov 23, 2020

View reviewed changes

simonhong requested a review from mkarolin November 24, 2020 02:20

mkarolin approved these changes Nov 24, 2020

View reviewed changes

simonhong added 13 commits November 25, 2020 09:06

Change toolbar/tab color of dark theme, private and tor window

62309ca

Adjust tab separator color of private/tor/guest

ebca566

Cleanup AvatarToolbarButton patches

58ac2d6

Re-style brave avatar button

eda9d59

Update location bar focus ring color for non regular

306be6e

Update location/omnibox color

87fe560

Paint graphic only on tor window frame

a71c0a6

Update omnibox hover color on dark mode

ad0982e

Update private/tor label in profile menu view

0bf37bc

Remove using IsTorProfile() function after rebasing

721c604

Update tooltip of tor avatar button

82e5458

Avoid patching for theme_service_factory.cc

7a7c9be

simonhong force-pushed the tab_contrast branch from f9bce3e to 7a7c9be Compare November 25, 2020 00:14

Fix lint error

d589649

simonhong force-pushed the tab_contrast branch from ff0eb13 to d589649 Compare November 25, 2020 01:26

simonhong merged commit ea2184f into master Nov 25, 2020

simonhong deleted the tab_contrast branch November 25, 2020 10:25

simonhong added this to the 1.19.x - Nightly milestone Nov 25, 2020

LaurenWags mentioned this pull request Jan 8, 2021

Improve ability to recognize the difference between a private window and private window with Tor brave/brave-browser#2735

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve tab contrast #6912

Improve tab contrast #6912

simonhong commented Oct 21, 2020 •

edited

Loading

simonhong commented Nov 16, 2020 •

edited

Loading

karenkliu commented Nov 16, 2020 •

edited

Loading

karenkliu commented Nov 17, 2020

simonhong commented Nov 18, 2020

simonhong commented Nov 18, 2020

karenkliu left a comment

petemill left a comment

petemill Nov 18, 2020

simonhong Nov 19, 2020

bsclifton Nov 20, 2020

simonhong Nov 21, 2020

petemill Nov 18, 2020

simonhong Nov 18, 2020

bsclifton commented Nov 19, 2020 •

edited

Loading

simonhong commented Nov 19, 2020

simonhong commented Nov 19, 2020 •

edited

Loading

simonhong commented Nov 20, 2020 •

edited

Loading

bsclifton commented Nov 20, 2020 •

edited

Loading

bsclifton Nov 20, 2020

bsclifton Nov 20, 2020

bsclifton left a comment

simonhong commented Nov 23, 2020

mkarolin Nov 23, 2020

simonhong Nov 24, 2020

mkarolin left a comment

simonhong commented Nov 25, 2020

Improve tab contrast #6912

Improve tab contrast #6912

Conversation

simonhong commented Oct 21, 2020 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

simonhong commented Nov 16, 2020 • edited Loading

karenkliu commented Nov 16, 2020 • edited Loading

karenkliu commented Nov 17, 2020

simonhong commented Nov 18, 2020

simonhong commented Nov 18, 2020

karenkliu left a comment

Choose a reason for hiding this comment

petemill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Nov 19, 2020 • edited Loading

simonhong commented Nov 19, 2020

simonhong commented Nov 19, 2020 • edited Loading

simonhong commented Nov 20, 2020 • edited Loading

bsclifton commented Nov 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

simonhong commented Nov 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

simonhong commented Nov 25, 2020

simonhong commented Oct 21, 2020 •

edited

Loading

simonhong commented Nov 16, 2020 •

edited

Loading

karenkliu commented Nov 16, 2020 •

edited

Loading

bsclifton commented Nov 19, 2020 •

edited

Loading

simonhong commented Nov 19, 2020 •

edited

Loading

simonhong commented Nov 20, 2020 •

edited

Loading

bsclifton commented Nov 20, 2020 •

edited

Loading