-
Notifications
You must be signed in to change notification settings - Fork 925
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
Remove duplicated menu separator after remove translate menu item #8914
Conversation
menu_model_.RemoveItemAt(index); | ||
|
||
// Separator always precedes translate item, |
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.
is this helpful here? It was recently added https://chromium.googlesource.com/chromium/src.git/+/3ac3f99cb82b34690d800a4dfdbefe2630060e39%5E%21/
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.
@bridiver ,
42c0333
yes, there are two useful methods: RenderViewContextMenuBase::RemoveSeparatorBeforeMenuItem
and RenderViewContextMenuBase::RemoveAdjacentSeparators
.
RemoveAdjacentSeparators
may be used in the end of BraveRenderViewContextMenu::InitMenu()
to exclude any possibility of duplicating the separators.
Both RemoveSeparatorBeforeMenuItem
and RemoveAdjacentSeparators
have the code in the end
if (toolkit_delegate_)
toolkit_delegate_->RebuildMenu();
But toolkit_delegate_
is initialized after BraveRenderViewContextMenu::InitMenu()
, so I had to clear and restore toolkit_delegate_
to avoid the crash.
CI failed, needs rebase |
f507f7b
to
495556b
Compare
CI failed on gn_check, related to the PR |
27e5a48
to
42c0333
Compare
42c0333
to
d570210
Compare
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.
LGTM as long as we pass the test plan in #3103.
d570210
to
bbf834e
Compare
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.
LGTM
8b7394b
to
a4a5eeb
Compare
a4a5eeb
to
b76bb5a
Compare
fixes brave/brave-browser#15714 Override of RenderViewContextMenuViews::Show is used
b76bb5a
to
702d950
Compare
@@ -161,6 +161,13 @@ source_set("ui") { | |||
"views/toolbar/bookmark_button.h", | |||
] | |||
|
|||
if (use_aura) { |
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.
@AlexeyBarabash Can you check this condition again?
I think below views related sources should be included with toolkit_views
condition above.
|use_aura| is false for macOS.
Resolves brave/brave-browser#15714
Sometimes after removing the Translate menu item, separator item is duplicated.
This PR removes the duplicated separator, if required.
Not related to the sync options on Nightly
1.27.4
at least.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: