Replaced folder image from bookmarks bar #9424
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mariospr
approved these changes
Jul 14, 2021
| @@ -21,4 +34,17 @@ bool ShouldShowAppsShortcutInBookmarkBar(Profile* profile) { | |||
| return false; | |||
| } | |||
|
|
|||
| #if defined(TOOLKIT_VIEWS) | |||
| ui::ImageModel GetBookmarkFolderIcon(SkColor text_color) { | |||
There was a problem hiding this comment.
I wonder if we should consider more cases in this overriden function like upstream does (e.g. BookmarkFolderIconType::kManaged for Win/Mac), but since the goal is to provide Brave-specific assets for folders, I think this is probably fine as is.
There was a problem hiding this comment.
It looks like two methods (for normal and for managed) are unified.
I think we can go with this.
In the future, we could consider different folder images.
mariospr
added a commit
that referenced
this pull request
Jul 16, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mariospr
added a commit
that referenced
this pull request
Jul 16, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mariospr
added a commit
that referenced
this pull request
Jul 19, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mariospr
added a commit
that referenced
this pull request
Jul 20, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mariospr
added a commit
that referenced
this pull request
Jul 22, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mariospr
added a commit
that referenced
this pull request
Jul 27, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mariospr
added a commit
that referenced
this pull request
Jul 28, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mkarolin
pushed a commit
that referenced
this pull request
Jul 29, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mariospr
added a commit
that referenced
this pull request
Jul 30, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mkarolin
pushed a commit
that referenced
this pull request
Aug 2, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mariospr
added a commit
that referenced
this pull request
Aug 4, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mkarolin
pushed a commit
that referenced
this pull request
Aug 5, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mkarolin
pushed a commit
that referenced
this pull request
Aug 7, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mariospr
added a commit
that referenced
this pull request
Aug 9, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mariospr
added a commit
that referenced
this pull request
Aug 10, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
|
When will there be customization to pick bookmarks folder icon we like better? Why even change it without offering a setting for it? EDIT: it is yellow in bookmarks manager, but in browser this plain gray thingy. Windows 10. |
|
Thank You NipLars! I was going to sign up just to ask these questions! ;) Seriously, WHY make this change without offering a setting for it? The 'new' folders are UGLY. I'd like the default/yellow folders restored OR a way to restore/change them. If the option to restore/change the folders can't be given to users, then change the folders back to the default/yellow folders. Thank You. |
|
It would indeed be nice to switch these back. My GF likes them, but I find they "blend in" a bit too much with the background, meaning when I'm looking at a mix of normal shortcuts and folders, my eyes can barely register the folders are there. Is there a setting to switch them back? It's really a personal preference thing. This sort of stuff should ABSOLUTELY be customisable in a web browser.. |
mkarolin
pushed a commit
that referenced
this pull request
Aug 12, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
mkarolin
pushed a commit
that referenced
this pull request
Aug 16, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
|
I thought I saw a setting in v129 to opt out of the new grey folder icons on the bar and go back to yellow. I am not a fan of the way the greay icons appeared and would like to go back. My current and up to date version of Google Chrome has the yellow icons. |
mkarolin
pushed a commit
that referenced
this pull request
Aug 18, 2021
Tested on Linux and it preserves the original behaviour from PR #9424. [1] #9424 Chromium change: https://source.chromium.org/chromium/chromium/src/+/2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 commit 2a4b9bfb525d77dfeec91be45068f9c95b00a8e9 Author: Peter Kasting <[email protected]> Date: Sat Jul 10 01:41:59 2021 +0000 Avoid accessing NativeTheme too early from bookmarks-related functions. This must only be accessed when the caller is in a Widget. Fixing this required significantly reworking how bookmark folder images are handled. This also modifies the color of bookmark folders on non-Win, non-Mac platforms: they are grey 700 by default (in light mode), like other icons, instead of being something closer to grey 600; and when a custom theme modifies the bookmark bar text color, folder icons in the overflow menu do not change similarly (but keep their standard menu colors). The menu does not necessarily have the same background color as the bookmark bar, so matching the bar's foreground color doesn't make sense, doubly so when done just for the folder icons and nothing else. Bug: 1211091
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves brave/brave-browser#16940
Windows:
macOS:
Linux:
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: