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.

Sign up for GitHub

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

Jump to bottom

Fixed navigation bar color at private tab #25923

Merged
merged 2 commits into from
Oct 11, 2024
Merged

Fixed navigation bar color at private tab #25923

merged 2 commits into from
Oct 11, 2024

Conversation

AlexeyBarabash
Copy link
Contributor

Resolves brave/brave-browser#41545

This PR fixes issue with the navigation bar color for private tab.

Color is used from @simoarpe 's upcoming PR c972bdd

                        Screenshot                                                              image

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:

  1. Take Nightly 1.72.66
  2. Open private tab

@AlexeyBarabash
Fixed navigation bar color at private tab; fixes brave/brave-browser#…
abee4f1 
…41545

Chromium change:
https://source.chromium.org/chromium/chromium/src/+/52962da478ed750038f966c386c4ee0ff2d73837

Always apply luminance calculation to navigation bar icons

Currently, the navigation bar icons only respond to the color of the navigation bar during animations. Since animations are being disabled for the navbar coloring experiment, it's important that the luminance calculation to properly color the navigation bar icons is applied all the time, not just during animations.

Bug: 364228106, 40925025
@AlexeyBarabash AlexeyBarabash self-assigned this Oct 10, 2024
samartnik
samartnik approved these changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@samartnik samartnik left a comment

Choose a reason for hiding this comment

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

lgtm

...ava/org/chromium/chrome/browser/tabbed_mode/BraveTabbedNavigationBarColorControllerBase.java
@@ -34,7 +37,11 @@ public int getNavigationBarColor(boolean forceDarkNavigationBar) {
if (BottomToolbarConfiguration.isBottomToolbarEnabled()
&& BraveMenuButtonCoordinator.isMenuFromBottom()
&& mContext != null) {
return mContext.getColor(R.color.default_bg_color_baseline);
if (mTabModelSelector.isIncognitoSelected()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should think of how to detect whether they deleted/renamed this variable in the upstream. Same applies to mContext. In any case we should treat variable that we deleted in the bytecode from the upstream's code as Nullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samartnik
by my understanding the bytecode test does it

assert.assertFalse(fieldExists("org/chromium/chrome/browser/tabbed_mode/TabbedNavigationBarColorController","mTabModelSelector"));

oh, that's assertFalse(fieldExists()), so it is applied after asm patching and we indeed can miss the upstream change then.

I think it is possible to add some degree of robust if we will change BraveClassVisitor so it will warn if the specified modification hadn't triggered.
I remember the cases from the past when build succeeded but my bytecode patch wasn't applied because I didn't specified the correct .jar file.

Created an issue brave/brave-browser#41548

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and I need to check mTabModelSelector for null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks fixed at d76aa8a

We should think of how to detect whether they deleted/renamed this variable in the upstream.

maybe we could check for null and if it is, then call DumpWithoutCrashing / reportException

simoarpe
simoarpe approved these changes Oct 10, 2024
View reviewed changes
Copy link
Collaborator

@simoarpe simoarpe left a comment

Choose a reason for hiding this comment

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

👍 Changes look good!
Thank you for fixing this!

@AlexeyBarabash AlexeyBarabash mentioned this pull request Oct 10, 2024
Open
@AlexeyBarabash AlexeyBarabash force-pushed the android_navigation_bar_color_private_tab branch from a15ac1b to d76aa8a Compare October 10, 2024 12:46
deeppandya
deeppandya approved these changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@deeppandya deeppandya left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @AlexeyBarabash

@AlexeyBarabash AlexeyBarabash merged commit 0939827 into master Oct 11, 2024
17 checks passed
@AlexeyBarabash AlexeyBarabash deleted the android_navigation_bar_color_private_tab branch October 11, 2024 14:17
@github-actions github-actions bot added this to the 1.73.x - Nightly milestone Oct 11, 2024
brave-builds added a commit that referenced this pull request Oct 11, 2024
@brave-builds
Uplift of #25923 (squashed) to beta
27937e5
brave-builds added a commit that referenced this pull request Oct 11, 2024
@brave-builds
Uplift of #25923 (squashed) to release
f762ece
This was referenced Oct 11, 2024
Merged
Merged
@brave-builds
Copy link
Collaborator

Released in v1.73.7

@hffvld
Copy link
Collaborator

hffvld commented Oct 11, 2024

Verified on Pixel 7 using version(s):

Device/OS: Pixel 7 / panther_beta-user 15 AP41.240823.009 release-keys
Brave build: 1.73.7 
Chromium: 130.0.6723.44 (Official Build) canary (64-bit)

STEPS:

  1. Make sure that the device's Dark mode is OFF
  2. Launch Brave > Confirm that the Light mode UI is shown
  3. Open Private tab > Verify

ACTUAL RESULTS:

  • Verified that the OS navigation bar in the Private tab has the correct color.
  • Verified the same when the device theme is set to Light and Dark mode.
  • Verified the same for the Standard tab as well.
  • Verified the same when Brave theme in Settings -> Appearance is set to System default, Light and Dark mode.
  • Verified the same for Gesture navigation and 3-button navigation.

Reproduced in Nightly 1.73.2

1 2
1 2

Verified in Nightly 1.73.7

1 2 3 4
1 2 3 4
1 2 3 4

@hffvld hffvld mentioned this pull request Oct 16, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simoarpe simoarpe simoarpe approved these changes

@deeppandya deeppandya deeppandya approved these changes

@samartnik samartnik samartnik approved these changes

Assignees

@AlexeyBarabash AlexeyBarabash

Labels
None yet
Projects
None yet
Milestone
1.73.x - Nightly
Development

Successfully merging this pull request may close these issues.

[CR130] Navigation bar color at private tab
6 participants
@AlexeyBarabash @brave-builds @hffvld @simoarpe @deeppandya @samartnik