-
Notifications
You must be signed in to change notification settings - Fork 885
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.
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
[Brave News]: Fix for incorrectly styled buttons on OSX #16085
Conversation
be23dee
to
e4e84b8
Compare
e4e84b8
to
cf40158
Compare
@@ -207,6 +207,15 @@ void MdTextButton::SetLoading(bool loading) { | |||
UpdateColors(); | |||
} | |||
|
|||
void MdTextButton::UpdateBackgroundColor() { | |||
// Handled via |UpdateColorsForBrave|. | |||
if (kind_ != kOld) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about splitting UpdateColorsForBrave()
into two parts? Background part and other parts.
and moving Background parts into here to sync with upstream structure.
If upsteam changes to call UpdateBackgroundColors()
from another places in the future, it could make regression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is on my list, but it's a bit of a bigger change and I'd like to keep this as minimal as possible so we can get it uplifted. I don't think, upstream to calling UpdateBackgroundColors
will break anything because it will use our colors (either the existing ones if kind_ == kOld
or the new ones, otherwise).
I think it'll also require a bit more testing if we do this change here, as we could break the existing buttons, rather than just changing the brave news ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start of a follow up PR is here: #16124
// that define their own UpdateColors() method (OmniboxChipButton) now, which | ||
// would not work with the virtual + override approach. | ||
// Note: We redefine UpdateBackgroundColor because we want it to be protected. | ||
#define UpdateBackgroundColor \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and how about removing md_text_button.cc.patch
while you're here?
If making UpdateColors()
as virtual in protected, it could be overridden by ours also like you did for UpdateBackgroundColor()
? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to make a followup PR, where do this + the background split.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we can't remove this, because the UpdateIconForBrave
method still needs the patch :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fallaciousreasoning I'm not following. The only patch is for BRAVE_MD_TEXT_BUTTON_UPDATE_COLORS
which can be removed by making UpdateColors
virtual as @simonhong is suggesting. You can call the superclass method and then do whatever BRAVE_MD_TEXT_BUTTON_UPDATE_COLORS
does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got scared off by the comment saying to not make UpdateColors
virtual. However, I just tested and the OmniboxChipButtons
look fine, so I guess that issue has been resolved in the 2+ years since the comment. Adding it to my follow up PR #16124
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ with f/u issue. This PR works great! 👍🏼
Verified
|
Brave | 1.48.13 Chromium: 108.0.5359.62 (Official Build) nightly (x86_64) |
---|---|
Revision | 041930a89a990cfab0315a2d9f20d6429a4a67cf-refs/branch-heads/5359@{#938} |
OS | macOS Version 11.7.1 (Build 20G918) |
Steps:
- installed
1.48.13
- launched Brave
- opened
brave://flags
- set
brave://flags/#brave-news-subscribe-button
toEnabled
- clicked
Relaunch
- loaded
https://www.pravda.com.ua/
- set mode to
Dark
andLight
, and tested button default, hover, and click/followed states
Confirmed initial/default, hover, and click/followed states are rendered correctly
Dark
mode:
default | hover | click/followed |
---|---|---|
Light
mode:
default | hover | click/followed |
---|---|---|
Resolves brave/brave-browser#26262
Before
After
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-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 tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: