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.

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

Cleanup how the MdTextButton works #16124

Merged
merged 1 commit into from
Feb 28, 2023
Merged

Conversation

fallaciousreasoning
Copy link
Contributor

@fallaciousreasoning fallaciousreasoning commented Nov 28, 2022

Resolves brave/brave-browser#27324

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:

Buttons in the Brave News Subscribe dialog should not change
Buttons in the permissions prompts (allow/deny) (i.e. for location) should not change

@simonhong
Copy link
Member

After loading https://www.theatlantic.com/world/ and click feed icon from location bar, I got below crash.
It's not followed yet.

[21469:259:1214/152132.545319:FATAL:md_text_button.cc(331)] Check failed: false. No style found for ButtonKind: 1, ColorScheme: light, ButtonState: 3
0   libbase.dylib                       0x000000010e35dd52 base::debug::CollectStackTrace(void**, unsigned long) + 18
1   libbase.dylib                       0x000000010e236863 base::debug::StackTrace::StackTrace() + 19
2   libbase.dylib                       0x000000010e258040 logging::LogMessage::~LogMessage() + 176
3   libbase.dylib                       0x000000010e2590fe logging::LogMessage::~LogMessage() + 14
4   libviews.dylib                      0x0000000125e4292b views::MdTextButton::GetButtonColors() + 555
5   libviews.dylib                      0x0000000125e42adb views::MdTextButton::UpdateBackgroundColor() + 27
6   libviews.dylib                      0x0000000125e40059 views::LabelButton::VisualStateChanged() + 41
7   libviews.dylib                      0x0000000125e0384a base::RepeatingCallback<void ()>::Run() const & + 90
8   libviews.dylib                      0x0000000125e12637 void base::internal::CallbackListBase<base::RepeatingCallbackList<void ()>>::Notify<>() + 199
9   libviews.dylib                      0x0000000125e0384a base::RepeatingCallback<void ()>::Run() const & + 90
10  libviews.dylib                      0x0000000125e12637 void base::internal::CallbackListBase<base::RepeatingCallbackList<void ()>>::Notify<>() + 199
11  libviews.dylib                      0x0000000125f173e4 views::Widget::OnNativeWidgetActivationChanged(bool) + 1044
12  libviews.dylib                      0x0000000125f42039 views::NativeWidgetMac::OnWindowKeyStatusChanged(bool, bool) + 41
13  libapp_shim.dylib                   0x000000012656ccd7 remote_cocoa::NativeWidgetNSWindowBridge::OnWindowKeyStatusChangedTo(bool) + 119
14  CoreFoundation                      0x00007ff814b4e3f4 __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 137
15  CoreFoundation                      0x00007ff814be843a ___CFXRegistrationPost_block_invoke + 88
16  CoreFoundation                      0x00007ff814be8389 _CFXRegistrationPost + 536
17  CoreFoundation                      0x00007ff814b21919 _CFXNotificationPost + 735
18  Foundation                          0x00007ff81595bf2c -[NSNotificationCenter postNotificationName:object:userInfo:] + 82
19  AppKit                              0x00007ff817e3f511 -[NSWindow resignKeyWindow] + 758
20  AppKit                              0x00007ff817dcc67c -[NSWindow _changeKeyAndMainLimitedOK:] + 964
21  AppKit                              0x00007ff817dcc0f8 -[NSWindow _makeKeyRegardlessOfVisibility] + 76
22  AppKit                              0x00007ff817dc3156 -[NSWindow makeKeyAndOrderFront:] + 27
23  libapp_shim.dylib                   0x000000012656b6dd remote_cocoa::NativeWidgetNSWindowBridge::SetVisibilityState(remote_cocoa::mojom::WindowVisibilityState) + 685
24  libviews.dylib                      0x0000000125f435e4 views::NativeWidgetMac::Show(ui::WindowShowState, gfx::Rect const&) + 244
25  libviews.dylib                      0x0000000125f15265 views::Widget::Show() + 277
26  libchrome_dll.dylib                 0x00000001192f881e BraveNewsBubbleView::Show(views::View*, content::WebContents*) + 62
27  libchrome_dll.dylib                 0x0000000119334c51 (anonymous namespace)::BraveNewsButtonView::ButtonPressed() + 65
28  libviews.dylib                      0x0000000125e0384a base::RepeatingCallback<void ()>::Run() const & + 90
29  libviews.dylib                      0x0000000125e3772e _ZN4base8internal7InvokerINS0_9BindStateIZN5views6Button15PressedCallbackC1ENS_17RepeatingCallbackIFvvEEEE3$_0JS8_EEEFvRKN2ui5EventEEE3RunEPNS0_13BindStateBaseESE_ + 30
30  libviews.dylib                      0x0000000125e37505 base::RepeatingCallback<void (ui::Event const&)>::Run(ui::Event const&) const & + 101
31  libviews.dylib                      0x0000000125e453a5 views::MenuButtonController::Activate(ui::Event const*) + 373
32  libviews.dylib                      0x0000000125e451f7 views::MenuButtonController::OnMousePressed(ui::MouseEvent const&) + 167
33  libviews.dylib                      0x0000000125efca2d views::View::ProcessMousePressed(ui::MouseEvent const&) + 301
34  libviews.dylib                      0x0000000125efc842 views::View::OnMouseEvent(ui::MouseEvent*) + 50
35  libevents.dylib                     0x0000000111215568 ui::ScopedTargetHandler::OnEvent(ui::Event*) + 88
36  libevents.dylib                     0x000000011120f16d ui::EventDispatcher::ProcessEvent(ui::EventTarget*, ui::Event*) + 301
37  libevents.dylib                     0x000000011120ef72 ui::EventDispatcherDelegate::DispatchEventToTarget(ui::EventTarget*, ui::Event*) + 66
38  libevents.dylib                     0x000000011120eed4 ui::EventDispatcherDelegate::DispatchEvent(ui::EventTarget*, ui::Event*) + 148
39  libviews.dylib                      0x0000000125f0d0f9 views::internal::RootView::OnMousePressed(ui::MouseEvent const&) + 681
40  libviews.dylib                      0x0000000125f18b7a views::Widget::OnMouseEvent(ui::MouseEvent*) + 362
41  libviews.dylib                      0x0000000125f349b6 non-virtual thunk to views::NativeWidgetMacNSWindowHost::OnMouseEvent(std::Cr::unique_ptr<ui::Event, std::Cr::default_delete<ui::Event>>) + 86
42  libapp_shim.dylib                   0x000000012655fa77 -[BridgedContentView mouseEvent:] + 215
43  AppKit                              0x00007ff817e91642 -[NSWindow(NSEventRouting) _handleMouseDownEvent:isDelayedEvent:] + 4283
44  AppKit                              0x00007ff817e076ae -[NSWindow(NSEventRouting) _reallySendEvent:isDelayedEvent:] + 2595
45  AppKit                              0x00007ff817e06a6f -[NSWindow(NSEventRouting) sendEvent:] + 345
46  libapp_shim.dylib                   0x000000012656601c -[NativeWidgetMacNSWindow sendEvent:] + 476
47  AppKit                              0x00007ff817e04dde -[NSApplication(NSEvent) sendEvent:] + 358
48  libchrome_dll.dylib                 0x0000000119cc03c2 __34-[BrowserCrApplication sendEvent:]_block_invoke + 322
49  libbase.dylib                       0x000000010e375682 base::mac::CallWithEHFrame(void () block_pointer) + 10
50  libchrome_dll.dylib                 0x0000000119cbff8f -[BrowserCrApplication sendEvent:] + 1023
51  AppKit                              0x00007ff8180c3885 -[NSApplication _handleEvent:] + 65
52  AppKit                              0x00007ff817c9441c -[NSApplication run] + 623
53  libbase.dylib                       0x000000010e38121c base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) + 348
54  libbase.dylib                       0x000000010e37f385 base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) + 165
55  libbase.dylib                       0x000000010e3080d5 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run(bool, base::TimeDelta) + 677
56  libbase.dylib                       0x000000010e2a864d base::RunLoop::Run(base::Location const&) + 685
57  libcontent.dylib                    0x0000000112e901c3 content::BrowserMainLoop::RunMainMessageLoop() + 243
58  libcontent.dylib                    0x0000000112e92162 content::BrowserMainRunnerImpl::Run() + 82
59  libcontent.dylib                    0x0000000112e8d5f7 content::BrowserMain(content::MainFunctionParams) + 199

@fallaciousreasoning
Copy link
Contributor Author

fallaciousreasoning commented Dec 15, 2022

Oh nice catch! Fixed! 😄 It's interesting, I didn't think that code path was being executed at all.

Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++ 👍🏼

@fallaciousreasoning fallaciousreasoning merged commit 1eb2b68 into master Feb 28, 2023
@fallaciousreasoning fallaciousreasoning deleted the md-text-cleanup branch February 28, 2023 00:33
@github-actions github-actions bot added this to the 1.50.x - Nightly milestone Feb 28, 2023
return;
}

// Override border color for hover on non-prominent
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused about these changes, why aren't we just overriding the ColorProvider?

Copy link
Member

@simonhong simonhong May 21, 2024

Choose a reason for hiding this comment

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

I assume that we had to copy the same code and use same bg color(kColorDialogBackground) so used kBraveBrandedColor directly instead of updating kColorButtonBorder.

As we'll use ui::ButtonStyle instead of Kind from cr126, I think we could try to override this button style's color via color provider instead of using GetButtonThemes().

Filed f/u issue for this - brave/brave-browser#38438

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MdTextButton should be refactored to use the same mechanisms as the Chromium impl of MdTextButton
4 participants